Partial scatter with warnings#4293
Conversation
This includes adding a new RecordWarnings() method to the vcursor interface so that primitives can store warnings in the session. Also add support to the route primitive to record warnings when the SCATTER_ERRORS_AS_WARNINGS query directive is set. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
To support the semantics of mysql warnings, clear them for all "non-diagnostic" statements, which in this case can be reasoned to be every statement other than SHOW. 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>
Remove the hard-coded vitess-specific error code for a shard error and instead extract the error code from the underlying error object like we do for regular mysql protocol errors. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
demmer
force-pushed
the
partial-scatter-with-warnings
branch
from
7f2cdd4 to
c1a5a3c
Compare
|
@sougou Now that the other three PRs are merged, I rebased this on the latest master and updated the description. This is now ready for review. |
|
Change is good. Tests need fixing. |
Need to call NewSQLErrorFromError to parse the errno out of the message since it isn't necessarily a SQLError. Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
demmer
force-pushed
the
partial-scatter-with-warnings
branch
from
882c12f to
9ca51ff
Compare
|
@sougou Updated with fixes for the end to end tests, which incidentally caught an oversight since I wasn't properly parsing into a SQLError to get the right mysql error code. |
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
Found one potential race (that I missed before). Looks good otherwise.
go/vt/vtgate/executor.go
Outdated
| // for all statements _except_ SHOW, so that SHOW WARNINGS | ||
| // can actually return them. | ||
| if stmtType != sqlparser.StmtShow { | ||
| safeSession.Warnings = nil |
There was a problem hiding this comment.
It just occurred to me that this assignment can race with itself because of recursive calls through engine and vindex. It will be safer to mutex this.
There was a problem hiding this comment.
Good point.
But it’s not simply solved with a mutex. It also seems like the recursive vindex execute could clear warnings from the current session.
In the current use case I don’t think this is a problem because warnings get applied at the end of the route phase which comes after the vindex statements.
But this also means the partial scatter option doesn’t get propagated to the vindex statements themselves which it probably should.
That’s a long way of saying that this feature is still not totally defined for lookup-dependent plans.
IMO we should add the mutex to be race free and merge as is. Then we can fix this later.
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
2 participants
Description
Final stage of the SCATTER_ERRORS_AS_WARNINGS query directive support. (Part 4 of #4279 )
Details
This last step ties the three previous changes together.
RecordWarnings()method on the vcursor.SHOWstatements so thatSHOW WARNINGSstill works.