Skip to content

mysql: support warning counts#4272

Merged
sougou merged 4 commits intovitessio:masterfrom
tinyspeck:mysql-server-support-warning-counts
Oct 28, 2018
Merged

mysql: support warning counts#4272
sougou merged 4 commits intovitessio:masterfrom
tinyspeck:mysql-server-support-warning-counts

Conversation

@demmer
Copy link
Copy Markdown
Member

@demmer demmer commented Oct 15, 2018

Description

Add support for warning counts to the mysql protocol implementation. (Part 2 of #4279)

Details

This includes both client and server protocol changes to propagate the warning counts into the EOF or OK packets, based on the CapabilityClientDeprecateEOF flag.

This also changes the mysql server handler interface to include a WarningCount() API which is intended to get the warning counts from the previously executed query.

As part of this change I refactored the server implementation slightly to move the command handling into the Conn class instead of the Listenener, which enables it to be used more easily in the unit tests.

Bug

Along the way I discovered a subtle bug in the client protocol handling which turns out to have no actual ramifications. But for completeness, when the CapabilityClientDeprecateEOF flag is set, then the last packet at the end of a query is actually an OK packet with the EOF type, and not an EOF packet.

It turns out though that our client was parsing the packet as an EOF regardless of the mode:

vitess/go/mysql/query.go

Lines 380 to 390 in 58ea83c

if isEOFPacket(data) {
// Strip the partial Fields before returning.
if !wantfields {
result.Fields = nil
}
result.RowsAffected = uint64(len(result.Rows))
more, err := parseEOFPacket(data)
if err != nil {
return nil, false, err
}
return result, more, nil

The only reason this works is that it turns out that the status flags happen to be in the same position in either an EOF or an OK packet IF affected_rows and last_insert_id are both 0:
https://dev.mysql.com/doc/internals/en/packet-OK_Packet.html
https://dev.mysql.com/doc/internals/en/packet-EOF_Packet.html

As such, the code didn't need to distinguish between the two cases since DMLs go through a different code path, so this turned out to be a theoretical problem that couldn't actually affect anything.

Of course now that we want the warning counts, the distinction does matter so I've fixed the handling accordingly.

@demmer demmer requested review from alainjobart and sougou October 15, 2018 18:56
@demmer demmer force-pushed the mysql-server-support-warning-counts branch 2 times, most recently from 4e1a1d5 to d29c491 Compare October 17, 2018 04:25
@demmer demmer force-pushed the mysql-server-support-warning-counts branch from d29c491 to 835a679 Compare October 19, 2018 23:05
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Let's see how the merge conflict gets resolved.

go/mysql/conn.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We'll probably have to think about this when resolving the merge conflict. When the change was made to ComPing, the listener was accessible, but now it's not.

Copy link
Copy Markdown
Member Author

@demmer demmer Oct 27, 2018

Choose a reason for hiding this comment

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

I just rebased the branch and handled this by stashing a reference to the listener in the Conn when used for server-side connections.

I think it's a reasonably safe invariant that Conn.handleNextCommand is only called for server-side connections for which newServerConn was used to create it.

In order to facilitate better unit testing of the server-side
command handlers, refactor the main server loop so that the
inner operation to handle an incoming command is part of the
Conn and not the Listener.

Take advantage of this in query_test.go to remove the ad-hoc
packet handling logic and instead use conn.handleNextCommand.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
The wire protocol supports sending a warning count at the end of each
query to signal to the client if there were any non-fatal warnings.

Add the approrpriate support to the mysql server and client implementations
to convey a warning count, and add a WarningCount method to the handler
interface so that specific server implementations can track warnings.

Along the way fix a bug in the client implementation -- when the
ClientDeprecateEOF option is enabled, it parsed the final EOF packet
incorrectly as an EOF packet but it should really have been parsed as
an OK packet with the EOF type. This could result in misinterpreting
the statusFlags, which could confuse the multi-query support.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
@demmer demmer force-pushed the mysql-server-support-warning-counts branch from ea13b0e to 99c600a Compare October 27, 2018 23:18
@sougou sougou merged commit f969b20 into vitessio:master Oct 28, 2018
@rafael rafael deleted the mysql-server-support-warning-counts branch October 29, 2018 18:21
@dweitzman
Copy link
Copy Markdown
Collaborator

dweitzman commented Oct 29, 2018

It seems possible that one of the tests for this doesn't pass with vitess/base:percona. Here's what I'm observing:

--- FAIL: TestMultiResultDeprecateEOF (0.00s)
	client_test.go:155: Negotiated ClientDeprecateEOF flag: false, want: true
--- FAIL: TestWarningsDeprecateEOF (0.00s)
	query_test.go:252: Negotiated ClientDeprecateEOF flag: false, want: true
FAIL
FAIL	vitess.io/vitess/go/mysql/endtoend	33.617s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants