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

DDL: do not Reload() for 'CREATE TEMPORARY' and 'DROP TEMPORARY' statements #12144

Merged
merged 5 commits into from
Jan 25, 2023
Merged

DDL: do not Reload() for 'CREATE TEMPORARY' and 'DROP TEMPORARY' statements #12144

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84dfc5b
DDL: do not Reload() for 'CREATE TEMPORARY' and 'DROP TEMPORARY' stat…
shlomi-noach Jan 25, 2023
db12da4
simplified check for temporary table
shlomi-noach Jan 25, 2023
718156f
simplified check again for temporary table
shlomi-noach Jan 25, 2023
08acc00
added plan test to see NeedsReservedConn is true for temporary table …
harshit-gangal Jan 25, 2023
24e7c22
added schema reload timing metric and add test to verify if reload ex…
harshit-gangal Jan 25, 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
39 changes: 25 additions & 14 deletions go/vt/vttablet/tabletserver/query_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,20 +657,31 @@ func (qre *QueryExecutor) execDDL(conn *StatefulConnection) (*sqltypes.Result, e
}
}

defer func() {
// Call se.Reload() with includeStats=false as obtaining table
// size stats involves joining `information_schema.tables`,
// which can be very costly on systems with a large number of
// tables.
//
// Instead of synchronously recalculating table size stats
// after every DDL, let them be outdated until the periodic
// schema reload fixes it.
if err := qre.tsv.se.ReloadAtEx(qre.ctx, mysql.Position{}, false); err != nil {
log.Errorf("failed to reload schema %v", err)
}
}()

isTemporaryTable := false
switch stmt := qre.plan.FullStmt.(type) {
case *sqlparser.CreateTable:
isTemporaryTable = stmt.IsTemporary()
case *sqlparser.DropTable:
isTemporaryTable = stmt.IsTemporary()
}

if !isTemporaryTable {
Copy link
Member

Choose a reason for hiding this comment

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

In the analyzeDDL plan
we mark in the plan NeedsReservedConn to true, so we can use that field here and ignore the reload

return &Plan{PlanID: PlanDDL, FullQuery: fullQuery, FullStmt: stmt, NeedsReservedConn: stmt.IsTemporary()}, nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can there be a future scenario where NeedsReservedConn is true even if the table is not TEMPORARY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've meanwhile simplified the check for IsTemporary()

Copy link
Member

Choose a reason for hiding this comment

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

I do not expect that to change and I have added a test specifically for that.
We can keep the current simplified version as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's keep the existing version, then, as it is more explicit 🙏

// Temporary tables are limited to the session creating them. There is no need to Reload()
// the table because other connections will not be able to see the table anyway.
defer func() {
// Call se.Reload() with includeStats=false as obtaining table
// size stats involves joining `information_schema.tables`,
// which can be very costly on systems with a large number of
// tables.
//
// Instead of synchronously recalculating table size stats
// after every DDL, let them be outdated until the periodic
// schema reload fixes it.
if err := qre.tsv.se.ReloadAtEx(qre.ctx, mysql.Position{}, false); err != nil {
log.Errorf("failed to reload schema %v", err)
}
}()
}
sql := qre.query
// If FullQuery is not nil, then the DDL query was fully parsed
// and we should use the ast to generate the query instead.
Expand Down