Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Mar 20, 2023
Merged

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

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be883bc
Mysqld.GetSchema: tolerate tables being dropped while inspecting schema
shlomi-noach Mar 16, 2023
eed2706
lack of primary key columns in STATISTICS does not mean table is drop…
shlomi-noach Mar 16, 2023
85f0273
populate validTds rather than rely on nil hints
shlomi-noach Mar 16, 2023
79cfdc2
re-introdce earlier check
shlomi-noach Mar 16, 2023
6ebebd8
use validTds, sync
shlomi-noach Mar 16, 2023
39cdc58
due to many tests consistently failing, trying a different approach: …
shlomi-noach Mar 19, 2023
6d866c8
grammar
shlomi-noach Mar 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion go/vt/mysqlctl/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"strings"
"sync"

"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/concurrency"
"vitess.io/vitess/go/vt/vterrors"
Expand Down Expand Up @@ -109,6 +110,16 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// the list of tables (collectBasicTableData(), earlier) and the point above where we investigate
// the table.
// This is fine. We identify the situation and continue to remove the table from our records
mattlord marked this conversation as resolved.
Show resolved Hide resolved
sqlErr, isSQLErr := mysql.NewSQLErrorFromError(err).(*mysql.SQLError)
if isSQLErr && sqlErr != nil && sqlErr.Number() == mysql.ERNoSuchTable {
td.Fields = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return
}

allErrors.RecordError(err)
cancel()
return
Expand All @@ -121,6 +132,8 @@ func (mysqld *Mysqld) GetSchema(ctx context.Context, dbName string, request *tab
}

// Get primary columns concurrently.
// The below runs a single query on `INFORMATION_SCHEMA` and does not interact with the actual tables.
// It is therefore safe to run even if some tables are dropped in the interim.
colMap := map[string][]string{}
if len(tableNames) > 0 {
wg.Add(1)
Expand All @@ -146,7 +159,18 @@ func (mysqld *Mysqld) GetSchema(ctx context.Context, dbName string, request *tab
td.PrimaryKeyColumns = colMap[td.Name]
}

sd.TableDefinitions = tds
sd.TableDefinitions = []*tabletmanagerdatapb.TableDefinition{}
for _, td := range tds {
if td.Fields == nil {
// means table was dropped before we called GetColumns
continue
}
mattlord marked this conversation as resolved.
Show resolved Hide resolved
if td.PrimaryKeyColumns == nil {
// means the table was dropped before we called getPrimaryKeyColumns()
continue
}
sd.TableDefinitions = append(sd.TableDefinitions, td)
}

tmutils.GenerateSchemaVersion(sd)
return sd, nil
Expand Down