Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protocol: support query attribute since mysql 8.0.23 #55175

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kafka1991
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #55174

Problem Summary:

  1. Supports parse query attributes in protocol layer
  2. support mysql_query_attribute_string builtin function.

What changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/invalid-title do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 4, 2024
@sre-bot
Copy link
Contributor

sre-bot commented Aug 4, 2024

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2024
Copy link

ti-chi-bot bot commented Aug 4, 2024

Welcome @kafka1991!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to pingcap/tidb. 😃

Copy link

ti-chi-bot bot commented Aug 4, 2024

Hi @kafka1991. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 4, 2024
Copy link

tiprow bot commented Aug 4, 2024

Hi @kafka1991. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@lance6716
Copy link
Contributor

/ok-to-test

@ti-chi-bot ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 4, 2024
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 5.26316% with 108 lines in your changes missing coverage. Please review.

Project coverage is 56.2613%. Comparing base (fdf550b) to head (6eed1cd).
Report is 1 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #55175         +/-   ##
=================================================
- Coverage   73.3185%   56.2613%   -17.0572%     
=================================================
  Files          1631       1756        +125     
  Lines        451092     630095     +179003     
=================================================
+ Hits         330734     354500      +23766     
- Misses       100031     251488     +151457     
- Partials      20327      24107       +3780     
Flag Coverage Δ
integration 36.7746% <5.2631%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.9478% <ø> (ø)
parser ∅ <ø> (∅)
br 52.6409% <ø> (+6.6242%) ⬆️

@kafka1991
Copy link
Contributor Author

i don't have much background about bazel, can anyone help me with bazel compilation error ?

@lance6716
Copy link
Contributor

lance6716 commented Aug 6, 2024

i don't have much background about bazel, can anyone help me with bazel compilation error ?

For the errors of https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fghpr_check/detail/ghpr_check/14289/pipeline

linting
  ✘  https://revive.run/r#exported  exported var ErrUnknownFieldType should have comment or be unexported  
  pkg/param/binary_params.go:22:5

please check this line and fix it. It means public (exported) variables should have comment which is expected at the place 1 line above the definition.

Also, please sign the CLA in the first comment.

@kafka1991
Copy link
Contributor Author

kafka1991 commented Aug 6, 2024

i don't have much background about bazel, can anyone help me with bazel compilation error ?

For the errors of https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fghpr_check/detail/ghpr_check/14289/pipeline

linting
  ✘  https://revive.run/r#exported  exported var ErrUnknownFieldType should have comment or be unexported  
  pkg/param/binary_params.go:22:5

please check this line and fix it. It means public (exported) variables should have comment which is expected at the place 1 line above the definition.

Thanks for your help, I have solved the problem.

Also, please sign the CLA in the first comment.

The CLA had been signed.

@lance6716
Copy link
Contributor

vicgao seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Maybe your GitHub account email is not the same as the email in this PR's commit?

@kafka1991
Copy link
Contributor Author

vicgao seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

Maybe your GitHub account email is not the same as the email in this PR's commit?

Had add the email in this PR's commit to github accounts in setting.

@lance6716
Copy link
Contributor

Files errors.toml and /tmp/errors.toml.before differ
make: *** [errdoc] Error 1

you can use make errdoc and then commit the changed files

@YangKeao
Copy link
Member

YangKeao commented Aug 8, 2024

The failed pull-mysql-client-test is the test from https://github.com/mysql/mysql-server/tree/trunk/testclients. We only run a subset of the tests in CI (because many of these tests are known to fail), so you can focus on the failed one in CI test_bug5194. It seems that the params protocol implementation has some bugs.

If you need any help to reproduce the test result or debug the protocol related codes, feel free to @ me here.

@kafka1991
Copy link
Contributor Author

The failed pull-mysql-client-test is the test from https://github.com/mysql/mysql-server/tree/trunk/testclients. We only run a subset of the tests in CI (because many of these tests are known to fail), so you can focus on the failed one in CI test_bug5194. It seems that the params protocol implementation has some bugs.

If you need any help to reproduce the test result or debug the protocol related codes, feel free to @ me here.

Thanks for your remind, i will try to reproduce it by myself.

@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2024
@kafka1991
Copy link
Contributor Author

The failed pull-mysql-client-test is the test from https://github.com/mysql/mysql-server/tree/trunk/testclients. We only run a subset of the tests in CI (because many of these tests are known to fail), so you can focus on the failed one in CI test_bug5194. It seems that the params protocol implementation has some bugs.

If you need any help to reproduce the test result or debug the protocol related codes, feel free to @ me here.

You are right, params protocol implementation for execute statement did has some bugs and they had been solved.

By the way, server has a minor compatibility issue when tested by higher version of mysql_client_test:

All lowercase letters in 'innodb'
https://github.com/mysql/mysql-server/blob/trunk/testclients/mysql_client_fw.cc

image

Not all lowercase letters in 'innodb'
https://github.com/pingcap/tidb/blob/master/pkg/executor/infoschema_reader.go

image

@kafka1991 kafka1991 changed the title [WIP] support query attribute since mysql 8.0.23 support query attribute since mysql 8.0.23 Aug 10, 2024
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2024
@YangKeao
Copy link
Member

By the way, server has a minor compatibility issue when tested by higher version of mysql_client_test:

MySQL also shows InnoDB:

+--------------------+---------+----------------------------------------------------------------+--------------+------+------------+
| ENGINE             | SUPPORT | COMMENT                                                        | TRANSACTIONS | XA   | SAVEPOINTS |
+--------------------+---------+----------------------------------------------------------------+--------------+------+------------+
| FEDERATED          | NO      | Federated MySQL storage engine                                 | NULL         | NULL | NULL       |
| MEMORY             | YES     | Hash based, stored in memory, useful for temporary tables      | NO           | NO   | NO         |
| InnoDB             | DEFAULT | Supports transactions, row-level locking, and foreign keys     | YES          | YES  | YES        |
| PERFORMANCE_SCHEMA | YES     | Performance Schema                                             | NO           | NO   | NO         |
| MyISAM             | YES     | MyISAM storage engine                                          | NO           | NO   | NO         |
| MRG_MYISAM         | YES     | Collection of identical MyISAM tables                          | NO           | NO   | NO         |
| BLACKHOLE          | YES     | /dev/null storage engine (anything you write to it disappears) | NO           | NO   | NO         |
| CSV                | YES     | CSV storage engine                                             | NO           | NO   | NO         |
| ARCHIVE            | YES     | Archive storage engine                                         | NO           | NO   | NO         |
+--------------------+---------+----------------------------------------------------------------+--------------+------+------------+

I think the result of the query select * from information_schema.engines where engine = 'innodb' differs because of the collation. It seems that MySQL is using a case insensitive collation for information_schema tables, but TiDB is using utf8mb4_bin. I remembered it's a known issue but I didn't find it now 🤦.

@YangKeao YangKeao changed the title support query attribute since mysql 8.0.23 protocol: support query attribute since mysql 8.0.23 Aug 12, 2024
@ti-chi-bot ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 15, 2024
@kafka1991
Copy link
Contributor Author

@lance6716 Can you help me with the failed tests?

@lance6716
Copy link
Contributor

/retest

@lance6716
Copy link
Contributor

@you06 can you take a look at failed UT TestInitStatsSessionBlockGC?

@lance6716
Copy link
Contributor

@hawkingrei Can you take a look at the failed case TestInitStatsSessionBlockGC?

@hawkingrei
Copy link
Member

/retest

@kafka1991
Copy link
Contributor Author

kafka1991 commented Oct 8, 2024

/retest

@hawkingrei @YangKeao @lance6716
It seems I reproduced the same test fails of the TestInitStatsSessionBlockGC case in master branch.
I try to debug the code and found there is no 'trx' for all inner sessions.

GetStartTSFromSession(se any) (startTS, processInfoID uint64) in tidb/pkg/sessiom/session.go

	txnInfo := tmp.TxnInfo()
        /// always false in test cases
	if txnInfo != nil {
		startTS = txnInfo.StartTS
		processInfoID = txnInfo.ConnectionID
	}

@lance6716
Copy link
Contributor

Sorry for the later reply @kafka1991 , I'll take a look in this week.

@lance6716
Copy link
Contributor

/retest

@lance6716
Copy link
Contributor

lance6716 commented Oct 22, 2024

Seems it's a flaky test, not the problem of your PR Maybe merging master will eliminate this problem. I can pass this test with your PR in my local environment.

Please continue review @YangKeao

Copy link

ti-chi-bot bot commented Oct 22, 2024

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-tests-checked label, please finished the tests then check the finished items in description.

For example:

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

📖 For more info, you can check the "Contribute Code" section in the development guide.

Copy link

tiprow bot commented Oct 22, 2024

@kafka1991: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
fast_test_tiprow 6eed1cd link true /test fast_test_tiprow

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link

ti-chi-bot bot commented Oct 22, 2024

@kafka1991: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/unit-test 6eed1cd link true /test unit-test
idc-jenkins-ci-tidb/build 6eed1cd link true /test build

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dveeden
Copy link
Contributor

dveeden commented Oct 23, 2024

From the CI build job:

pkg/server/conn_stmt_params_test.go:322:93: param min has same name as predeclared identifier (predeclared)

Maybe change min to _min or minute ?

@@ -23,7 +23,7 @@ import (
var ErrUnknownFieldType = dbterror.ClassServer.NewStd(errno.ErrUnknownFieldType)

// BinaryParam stores the information decoded from the binary protocol
// It can be further parsed into `expression.Expression` through the `ExecArgs` function in this package
// It can be further parsed into `expression.Expression` through the expression.ExecBinaryParam function in expression package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// It can be further parsed into `expression.Expression` through the expression.ExecBinaryParam function in expression package
// It can be further parsed into `expression.Expression` through the expression.ExecBinaryParam function in the expression package

@@ -1698,6 +1706,61 @@ func (cc *clientConn) audit(eventType plugin.GeneralEvent) {
}
}

// parseQueryAttributes support query attributes since mysql 8.0.23
// see https://dev.mysql.com/doc/refman/8.0/en/query-attributes.html
// https://archive.fosdem.org/2021/schedule/event/mysql_protocl/attachments/slides/4274/export/events/attachments/mysql_protocl/slides/4274/FOSDEM21_MySQL_Protocols_Query_Attributes.pdf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// https://archive.fosdem.org/2021/schedule/event/mysql_protocl/attachments/slides/4274/export/events/attachments/mysql_protocl/slides/4274/FOSDEM21_MySQL_Protocols_Query_Attributes.pdf
// https://archive.fosdem.org/2021/schedule/event/mysql_protocl/attachments/slides/4274/export/events/attachments/mysql_protocl/slides/4274/FOSDEM21_MySQL_Protocols_Query_Attributes.pdf
// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_query.html
// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_stmt_execute.html

@@ -319,6 +319,20 @@ func buildDatetimeParam(year uint16, month uint8, day uint8, hour uint8, minute
return result
}

func buildDatetimeParamWithClientQueryAttr(year uint16, month uint8, day uint8, hour uint8, min uint8, sec uint8, msec uint32) []byte {
Copy link
Contributor

@dveeden dveeden Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func buildDatetimeParamWithClientQueryAttr(year uint16, month uint8, day uint8, hour uint8, min uint8, sec uint8, msec uint32) []byte {
func buildDatetimeParamWithClientQueryAttr(year uint16, month uint8, day uint8, hour uint8, _min uint8, sec uint8, msec uint32) []byte {

Maybe _min or minute because min is a predeclared identifier

ast.NextVal: &nextValFunctionClass{baseFunctionClass{ast.NextVal, 1, 1}},
ast.LastVal: &lastValFunctionClass{baseFunctionClass{ast.LastVal, 1, 1}},
ast.SetVal: &setValFunctionClass{baseFunctionClass{ast.SetVal, 2, 2}},
ast.QueryAttrString: &getQueryAttrFunctionClass{baseFunctionClass{ast.QueryAttrString, 1, 1}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ast.QueryAttrString: &getQueryAttrFunctionClass{baseFunctionClass{ast.QueryAttrString, 1, 1}},
// Query attributes and ...
ast.QueryAttrString: &getQueryAttrFunctionClass{baseFunctionClass{ast.QueryAttrString, 1, 1}},

I don't think this belongs under "TiDB Sequence function." Maybe think of a good name or leave it out if it is obvious.

return b.SessionVarsPropReader.RequiredOptionalEvalProps()
}

func (b *builtinGetQueryAttrStringSig) evalString(ctx EvalContext, row chunk.Row) (string, bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (b *builtinGetQueryAttrStringSig) evalString(ctx EvalContext, row chunk.Row) (string, bool, error) {
// This implements `mysql_query_attribute_string(str)`
func (b *builtinGetQueryAttrStringSig) evalString(ctx EvalContext, row chunk.Row) (string, bool, error) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often people know the function name and then have to fine the location of the code. This might help to make it easier to find.

@dveeden
Copy link
Contributor

dveeden commented Oct 23, 2024

Please review the description.

  • It should have a release note
  • It should tick some of the documentation boxes as it affects MySQL compatibility (in a good way) etc
  • It should tick some of the testing boxes

@dveeden
Copy link
Contributor

dveeden commented Oct 23, 2024

When testing MySQL 9.1.0 I noticed that mysql_query_attribute_string doesn't exist by default. You need to run INSTALL COMPONENT "file://component_query_attributes" first. I don't think the possible naming clashing is not a problem because of the naming and the fact that TiDB doesn't support stored functions etc.

Copy link
Contributor

@dveeden dveeden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#!/bin/python3
import mysql.connector

c = mysql.connector.connect(
    host="127.0.0.1", port=4000, user="root", database="test", ssl_disabled=True
)

cur = c.cursor(prepared=True)
cur.add_attribute("testattr", "test value")

cur.execute("SELECT mysql_query_attribute_string(%s)", ("testattr",))
for row in cur:
    print(row)

print(cur.get_attributes())

cur.clear_attributes()
print(cur.get_attributes())
cur.close()
c.close()

This script succeeds against MySQL 9.1.0 but fails against TiDB (master_query_attr branch).

dvaneeden@dve-carbon:~$ ./query_attributes.py 
Traceback (most recent call last):
  File "/usr/lib64/python3.12/site-packages/mysql/connector/connection_cext.py", line 651, in cmd_stmt_execute
    statement_id.stmt_execute(*args, query_attrs=self.query_attrs)
_mysql_connector.MySQLInterfaceError: Error while executing statement: runtime error: index out of range [1] with length 1

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/dvaneeden/./query_attributes.py", line 9, in <module>
    cur.execute("SELECT mysql_query_attribute_string(%s)", ('testattr',))
  File "/usr/lib64/python3.12/site-packages/mysql/connector/cursor_cext.py", line 1229, in execute
    res = self._cnx.cmd_stmt_execute(self._stmt, *params)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/mysql/connector/opentelemetry/context_propagation.py", line 102, in wrapper
    return method(cnx, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/site-packages/mysql/connector/connection_cext.py", line 653, in cmd_stmt_execute
    raise InterfaceError(str(err)) from err
mysql.connector.errors.InterfaceError: Error while executing statement: runtime error: index out of range [1] with length 1

Wireshark also fails to decode it:
image

This might be because the flags are set to 0x0 instead of 0x8 (PARAMETER_COUNT_AVAILABLE). See also https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_com_stmt_execute.html

Copy link

ti-chi-bot bot commented Oct 23, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-10-23 07:52:04.499181605 +0000 UTC m=+423925.195972215: ✖️🔁 reset by dveeden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/needs-tests-checked ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support query attributed in client protocol since 8.0.23
6 participants