Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12641

shlomi-noach · 2023-03-16T12:30:27Z

Description

Identify the two scenarios where a table may be dropped while we're inspecting the schema. Either we get a (errno 1146) error code, or the table does not appear in information_schema even after it did, before.

The fix is to silently ignore tables hitting this scenario.

Related Issue(s)

Fixes: Bug Report: Mysqld.GetSchema() race condition when table is dropped throughout inspection #12640

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-03-16T13:31:53Z

hmmmm got this relevant error in Unit_test:

2023-03-16T13:25:15.6015797Z === RUN   TestDeferSecondaryKeys/3FPK1SK_ShardMerge
2023-03-16T13:25:15.6015939Z === RUN   TestDeferSecondaryKeys/0FPK2tSK
2023-03-16T13:25:15.6016109Z     vreplicator_test.go:443: 
2023-03-16T13:25:15.6016645Z         	Error Trace:	/home/runner/work/vitess/vitess/go/vt/vttablet/tabletmanager/vreplication/vreplicator_test.go:443
2023-03-16T13:25:15.6016885Z         	Error:      	Received unexpected error:
2023-03-16T13:25:15.6017622Z         	            	unexpected number of table definitions returned from GetSchema call for table "t1": 0
2023-03-16T13:25:15.6017881Z         	Test:       	TestDeferSecondaryKeys/0FPK2tSK
2023-03-16T13:25:15.6018020Z === RUN   TestDeferSecondaryKeys/2SKRetryNoErr
2023-03-16T13:25:15.6018181Z === RUN   TestDeferSecondaryKeys/2SKRetryNoErr2
2023-03-16T13:25:15.6018338Z === RUN   TestDeferSecondaryKeys/SKSuperSetNoErr

shlomi-noach · 2023-03-16T13:35:59Z

I'm not sure what the situation is in vreplicator_test.go calling stashSecondaryKeys(), that results with an empty schema. That is, I'm not sure if the new logic is doing the right thing, and this is unexpected to stashSecondaryKeys(), or whether the new logic is doing something wrong. I'm not sure if a table is being dropped here.

shlomi-noach · 2023-03-16T13:37:10Z

Ah, I get it. A table without PRIMARY KEY. Converting this PR to draft while I fix it.

…ped. It can also mean the table does not have PRIMARY KEY Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-03-16T13:41:40Z

OK, the assumption about the 2nd race condition was incorrect. A table that does not appear in INFORMATION_SCHEMA.STATISTICS isn't necessarily dropped. It may also be a table that does not have any keys.

Removed the 2nd race condition handling altogether. It a table does end up being dropped before calculating primary key, it will just have an implicit nil primary key.

mattlord

Thank you for investigating this! Nice debugging work. I only had some minor comments.

mattlord · 2023-03-16T15:06:56Z

go/vt/mysqlctl/schema.go

+ // This is fine. We identify the situation and continue to remove the table from our records
+ sqlErr, isSQLErr := mysql.NewSQLErrorFromError(err).(*mysql.SQLError)
+ if isSQLErr && sqlErr != nil && sqlErr.Number() == mysql.ERNoSuchTable {
+ td.Fields = nil


Should we instead remove td from tds here, or set td = nil? I feel like the former would be the right thing to do so that our memory representation tracks the actual live mysqld state (which is the point of that component AFAIUI).

OK, I rewrote this in a different way. Problem is that we're iterating tds concurrently. Any change to tds itself during iteration is less than ideal. So instead I'm building a validTds slice which in turn is the final list of table definitions.

go/vt/mysqlctl/schema.go

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

mattlord

Approving so that you can merge whenever all the tests are passing.

IMO, we should re-run the affected test(s) 5-10 times before merging to be sure that they now pass each time — or at least don't have the same sub-test failure which led to the investigation — and we've fully addressed the issue.

Thanks again for great investigative work!

mattlord · 2023-03-16T15:48:28Z

go/vt/mysqlctl/schema.go

@@ -109,6 +112,15 @@ func (mysqld *Mysqld) GetSchema(ctx context.Context, dbName string, request *tab

 fields, columns, schema, err := mysqld.collectSchema(ctx, dbName, td.Name, td.Type, request.TableSchemaOnly)
 if err != nil {
+ // there's a possible race condition: it could happen that a table was dropped in between reading


IF you have to do another push for some reason, we can capitalize There's here. It's annoyingly nitty, I know, so no need unless we have to do another commit/push for another reason.

shlomi-noach · 2023-03-16T16:12:55Z

IMO, we should re-run the affected test(s) 5-10 times before merging to be sure

I've ran this on local env some 20 times and it passed all runs. I'm signing out for the week and won't be watching CI to kick restarts. If this can wait until Sunday, I can verify and merge then. If not, anyone should feel free to test/retry and merge.

mattlord · 2023-03-16T16:17:04Z

IMO, we should re-run the affected test(s) 5-10 times before merging to be sure

I've ran this on local env some 20 times and it passed all runs. I'm signing out for the week and won't be watching CI to kick restarts. If this can wait until Sunday, I can verify and merge then. If not, anyone should feel free to test/retry and merge.

Sounds good. You deserve a clean break for the weekend! 🙂 I can periodically re-run the test, as I'm doing that on another PR too.

shlomi-noach · 2023-03-19T05:59:39Z

Multiple tests are consistently failing on this PR. Example: vreplication_basic: https://github.com/vitessio/vitess/actions/runs/4438816245/jobs/7831448081?pr=12641 with error:

2023-03-19T05:54:35.2013877Z Reshard Error: rpc error: code = Unknown desc = copySchema: CopySchemaShard was not successful because the schemas between the two tablets cell:"zone1" uid:200 and cell:"zone1" uid:600 differ: [dest has an extra table named cproduct dest has an extra table named customer dest has an extra table named db_order_test source has an extra table named db_order_test source has an extra table named cproduct source has an extra table named customer]

vtgate_schema has a similar error:

Mar 19 05:54:00         	Error Trace:	go/test/endtoend/vtgate/schema/schema_test.go:249
Mar 19 05:54:00         	            				go/test/endtoend/vtgate/schema/schema_test.go:110
Mar 19 05:54:00         	Error:      	Expected nil, but got: &exec.ExitError{ProcessState:(*os.ProcessState)(0xc0006de408), Stderr:[]uint8(nil)}
Mar 19 05:54:00         	Test:       	TestSchemaChange

I'm going to try a different angle to this PR. Instead of removing the dropped table from the result set, I'm going to keep it there, with empty table definition, ie empty keys, empty columns etc.

…we keep the table, but with empty column/key/fields info Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-03-19T07:11:56Z

@mattlord @rohit-nayak-ps could you give this another look? I've simplified the change to just returning an incomplete table description. Previously, when I excluded the table description from the result table list, many tests failed, consistently, see above. Now, everything seems to pass.

shlomi-noach · 2023-03-19T07:16:28Z

PS I'm re-running onlineddl_vrepl in CI just to ensure it passes consistently.

shlomi-noach · 2023-03-20T07:09:17Z

So with the current implementation Columns and Fields remain nil. I reviewed all usage and nothing is susceptible to nil pointer access; everything just properly ranges over the slices.

Tests are super happy. I think this should be good.

rohit-nayak-ps · 2023-03-20T09:35:37Z

@mattlord @rohit-nayak-ps could you give this another look? I've simplified the change to just returning an incomplete table description. Previously, when I excluded the table description from the result table list, many tests failed, consistently, see above. Now, everything seems to pass.

Looks good @shlomi-noach, thanks!

shlomi-noach · 2023-03-20T09:43:59Z

waiting to see if the bot is able to cherry pick this

…ma (vitessio#12641) * Mysqld.GetSchema: tolerate tables being dropped while inspecting schema Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * lack of primary key columns in STATISTICS does not mean table is dropped. It can also mean the table does not have PRIMARY KEY Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * populate validTds rather than rely on nil hints Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * re-introdce earlier check Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * use validTds, sync Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * due to many tests consistently failing, trying a different approach: we keep the table, but with empty column/key/fields info Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * grammar Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ma (#12641) (#12666) * Mysqld.GetSchema: tolerate tables being dropped while inspecting schema * lack of primary key columns in STATISTICS does not mean table is dropped. It can also mean the table does not have PRIMARY KEY * populate validTds rather than rely on nil hints * re-introdce earlier check * use validTds, sync * due to many tests consistently failing, trying a different approach: we keep the table, but with empty column/key/fields info * grammar --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ma (#12641) (#12665) * Mysqld.GetSchema: tolerate tables being dropped while inspecting schema * lack of primary key columns in STATISTICS does not mean table is dropped. It can also mean the table does not have PRIMARY KEY * populate validTds rather than rely on nil hints * re-introdce earlier check * use validTds, sync * due to many tests consistently failing, trying a different approach: we keep the table, but with empty column/key/fields info * grammar --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…ma (#12641) (#12664) * Mysqld.GetSchema: tolerate tables being dropped while inspecting schema * lack of primary key columns in STATISTICS does not mean table is dropped. It can also mean the table does not have PRIMARY KEY * populate validTds rather than rely on nil hints * re-introdce earlier check * use validTds, sync * due to many tests consistently failing, trying a different approach: we keep the table, but with empty column/key/fields info * grammar --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Mysqld.GetSchema: tolerate tables being dropped while inspecting schema

be883bc

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Bug Component: VReplication Component: Schema Tracker Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) labels Mar 16, 2023

shlomi-noach requested review from deepthi, mattlord and rsajwani as code owners March 16, 2023 12:30

shlomi-noach requested a review from rohit-nayak-ps March 16, 2023 12:32

shlomi-noach added Backport to: release-14.0 labels Mar 16, 2023

dbussink approved these changes Mar 16, 2023

View reviewed changes

shlomi-noach marked this pull request as draft March 16, 2023 13:37

lack of primary key columns in STATISTICS does not mean table is drop…

eed2706

…ped. It can also mean the table does not have PRIMARY KEY Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach marked this pull request as ready for review March 16, 2023 13:41

mattlord reviewed Mar 16, 2023

View reviewed changes

shlomi-noach added 3 commits March 16, 2023 17:26

populate validTds rather than rely on nil hints

85f0273

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

re-introdce earlier check

79cfdc2

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

use validTds, sync

6ebebd8

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

mattlord approved these changes Mar 16, 2023

View reviewed changes

shlomi-noach added 2 commits March 19, 2023 08:15

due to many tests consistently failing, trying a different approach: …

39cdc58

…we keep the table, but with empty column/key/fields info Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

grammar

6d866c8

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit 0857b7d into vitessio:main Mar 20, 2023

shlomi-noach deleted the get-schema-ignore-dropped-tables branch March 20, 2023 09:41

shlomi-noach mentioned this pull request Mar 20, 2023

Cherry pick v16: Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12664

Merged

4 tasks

shlomi-noach mentioned this pull request Mar 20, 2023

Cherry pick v15: Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12665

Merged

4 tasks

shlomi-noach mentioned this pull request Mar 20, 2023

Cherry pick v14: Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12666

Merged

4 tasks

austenLacy mentioned this pull request Apr 26, 2023

Fixes the SwitchTraffic bug that wasn't respecting --dry_run for readonly and replica tablets during a resharding event Shopify/vitess#91

Merged

austenLacy mentioned this pull request Aug 1, 2023

Custom jwt http ACL policy Shopify/vitess#114

Closed

hmaurer mentioned this pull request Mar 21, 2024

oops #15542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12641

Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12641

shlomi-noach commented Mar 16, 2023 •

edited by mattlord

Loading

shlomi-noach commented Mar 16, 2023

shlomi-noach commented Mar 16, 2023

shlomi-noach commented Mar 16, 2023

shlomi-noach commented Mar 16, 2023

mattlord left a comment

mattlord Mar 16, 2023

shlomi-noach Mar 16, 2023

mattlord left a comment

mattlord Mar 16, 2023

shlomi-noach commented Mar 16, 2023

mattlord commented Mar 16, 2023

shlomi-noach commented Mar 19, 2023

shlomi-noach commented Mar 19, 2023

shlomi-noach commented Mar 19, 2023

shlomi-noach commented Mar 20, 2023

rohit-nayak-ps commented Mar 20, 2023

shlomi-noach commented Mar 20, 2023

Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12641

Mysqld.GetSchema: tolerate tables being dropped while inspecting schema #12641

Conversation

shlomi-noach commented Mar 16, 2023 • edited by mattlord Loading

Description

Related Issue(s)

Checklist

shlomi-noach commented Mar 16, 2023

shlomi-noach commented Mar 16, 2023

shlomi-noach commented Mar 16, 2023

shlomi-noach commented Mar 16, 2023

mattlord left a comment

Choose a reason for hiding this comment

mattlord Mar 16, 2023

Choose a reason for hiding this comment

shlomi-noach Mar 16, 2023

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

mattlord Mar 16, 2023

Choose a reason for hiding this comment

shlomi-noach commented Mar 16, 2023

mattlord commented Mar 16, 2023

shlomi-noach commented Mar 19, 2023

shlomi-noach commented Mar 19, 2023

shlomi-noach commented Mar 19, 2023

shlomi-noach commented Mar 20, 2023

rohit-nayak-ps commented Mar 20, 2023

shlomi-noach commented Mar 20, 2023

shlomi-noach commented Mar 16, 2023 •

edited by mattlord

Loading