-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
server: fix COM_FIELD_LIST response bug that make mariadb-client crash during use db
#6918
Conversation
use db
Well done! |
server/column.go
Outdated
// Dump dumps ColumnInfo to bytes. | ||
func (column *ColumnInfo) Dump(buffer []byte) []byte { | ||
func (column *ColumnInfo) Dump(buffer []byte, flags DumpOpt) []byte { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a bool argument to control whether output default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done~
server/column.go
Outdated
if flags&WithDefaultValue > 0 { | ||
// Current we doesn't output defaultValue but reserve defaultValue length bit to make mariadb client happy. | ||
// https://dev.mysql.com/doc/internals/en/com-query-response.html#column-definition | ||
buffer = dumpUint64(buffer, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because in protocol definition https://dev.mysql.com/doc/internals/en/com-query-response.html#column-definition said.
if command was COM_FIELD_LIST {
lenenc_int length of default-values
string[$len] default values
}
So, if it's a response of COM_FIELD_LIST, we should actually add default values part use just like this.
+----------------------+-------------------+------------+--------------+--------------+-----------+--------------------+
| | | | | | | |
| | | | | | | |
| | | | | | | |
| | | | | | | |
| ....sth we | length byte of | default | default | default | default | .............. |
| outputed | default values | value 1 | value 2 | value 3 | value 4 | |
| | | | | | | |
| | | | | | | |
| | | | | | | |
+----------------------+-------------------+------------+--------------+--------------+-----------+--------------------+
but In tidb we omit default values(It seems not easy to output that....), and omit all default values bytes...but mariadb will read length byte of default values
without condition, after send COM_FIELD_LIST
. see: https://github.com/MariaDB/server/blob/72b6d01848e56a75349d663bc61bbe71f97a280b/sql-common/client.c#L1519
So at this PR, we don't complete fill all default values but follow the protocol give client an empty default values
+-------------+--------------+
| | |
| | |
| | |
| sth we | length byte |
| outputed | of default |
| | values |
| | |
| | (0) |
| | |
+-------------+--------------+
so mariadb will read length byte without out of index crash.
Review status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @shenli, @tiancaiamao, and @coocood) Comments from Reviewable |
Review status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @shenli, @lysu, @tiancaiamao, and @coocood) server/column.go, line 50 at r3 (raw file):
Comments from Reviewable |
Reviewed 1 of 4 files at r1, 2 of 3 files at r2, 1 of 1 files at r3. Comments from Reviewable |
LGTM |
/run-all-tests |
@tiancaiamao PTAL |
1 similar comment
@tiancaiamao PTAL |
/run-all-tests |
LGTM @shenli |
LGTM |
@lysu Could you add some integration test for this? |
/run-mybatis-test |
[column-definitions](https://dev.mysql.com/doc/internals/en/com-query-response.html#column-definition) in COM_FIELD_LIST response need write addition default-values part even though we omit default-values. and some client like MariaDB-cli will read default-values length bit and meet fault if we haven't give it. https://github.com/MariaDB/server/blob/72b6d01848e56a75349d663bc61bbe71f97a280b/sql-common/client.c#L1519 fix #6622
we should open for continue edition that fill default value.
PTAL @coocood |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What have you changed? (mandatory)
column-definitions in COM_FIELD_LIST response need write addition default-values part even though we omit default-values.
and some client like MariaDB-cli(without
-A
)'suse db
will trigger COM_FEILD_LIST and read default-values length bit in response, it will meet fault if we haven't give it.https://github.com/MariaDB/server/blob/72b6d01848e56a75349d663bc61bbe71f97a280b/sql-common/client.c#L1519
dump()
add option parameter and passWithDefaultValues
option inhandle_field_list()
DefaultValue
DefaultValueLength
field fromserver.ColumnInfo
fix #6622
What are the type of the changes (mandatory)?
The currently defined types are listed below, please pick one of the types for this PR by removing the others:
How has this PR been tested (mandatory)?
This change is