vtexplain support different modes #3532
Conversation
demmer
force-pushed
the
vtexplain-test-other-modes
branch
from
aec67fb
to
1177203
Compare
|
As discussed offline, can you remove legacyAutocommit support? It's a soon-to-be deprecated feature. |
This reverts commit 1f3a930. Sorting in fact makes the test output illogical for sequences where statements occur in the same time quantum since it inverts the causal ordering.
Include an execution mode in the vtexplain options and refactor the tests to support different outputs for each execution mode. The mode doesn't yet change the behavior -- support for legacy autocommit and TwoPC to come in subsequent commits.
demmer
force-pushed
the
vtexplain-test-other-modes
branch
from
9ded85c
to
0a82a1f
Compare
|
@sougou I removed the legacy autocommit support as discussed and rebased onto the latest master. It would be great to merge this before changes like #3559 or #3564 since it would be great to see the effect of either change on the vtexplain output as another demonstration about what will change, and it is hard to reason about properly until we revert the sorting (2c78fe0e in this PR). As an aside, I wonder if we should stop including the json output mode from vtexplain as part of the travis testing and just include the text mode. Although the json mode does include much more detail, as a result it is also prone to test conflicts since it dumps the full Plan. |
|
I'm wondering if a single test to sanity check the json output would be sufficient. This will save us from making huge test changes for every small change in plans. |
The json mode ends up being very verbose and prone to changes when the internals of the vtgate plan object change, so remove them from the vtexplain tests.
Check the structure of the vtexplain output without depending on the details of the Plan so that any future changes to the plan structure don't cause the vtexplain tests to need updating.
|
Yep -- my thoughts exactly. I added a test that verifies the structure of the JSON output matches what we expect without depending on the details of the Plan object. This way any future changes to the vtgate planner that don't actually affect the tablet execution shouldn't require the vtexplain test output to change at all. |
|
LGTM |
…ip vindex keyrange filtering when we can (vitessio#3532) * backport of 3529 * Adjust for v17 Signed-off-by: Matt Lord <mattalord@gmail.com> --------- Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com>
Add an optional execution mode option to vtexplain with support for "multi" (default), "legacy_autocommit", and "twopc" to provide more insight into how vitess will handle queries in these other non-standard modes.
In doing this work I noticed that delete queries were failing on tables with an owned lookup vindex, because vtgate tries to run a
select ... for updatequery on the primary table to find out the values needed to delete the corresponding rows in the lookup table.As an attempt to make this work, I updated the vtgate query planner with an option to run the lookup queries in an unlocked mode and use this mode when legacy autocommit is enable and there is no active transaction (see 3edb1bfafd7210040b65b25ae05f25880984338f for the implementation). Although this is clearly not transactionally safe, it seems preferable to simply failing to do the deletes altogether in legacy autocommit mode.
For TwoPC mode, all the vtexplain test cases do work and I've included the output from a test run as part of this commit, but the output isn't stable across test runs since the values put into the twopc tracking tables include the timestamp. I still included the test output files in the PR since I found it informative to compare the output for the various modes (and to see exactly what TwoPC does under the covers).
Finally, I ended up reverting 1f3a930 since the sorting turned out to not actually be needed for stable test output and in fact it removed logical sequencing from the statements within a single time epoch which turns out to be important.