-
Notifications
You must be signed in to change notification settings - Fork 4k
sql/*: add hint injection #158096
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.
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
base: master
Are you sure you want to change the base?
sql/*: add hint injection #158096
Conversation
When loading an external statement hint into the statement hints cache, we might need to call some function to get the hint ready for use. (For hint injections, this function is `tree.NewHintInjectionDonor` which parses and walks the donor statement fingerprint.) This function could fail, in which case we want to skip over the hint but not return an error from `GetStatementHintsFromDB`. This function could succeed but create some extra state which we need to save. This commit adds a new `parseHint` step which calls any functions needed to get the hint ready, and creates a new `hints.Hint` struct which holds the object(s) created when parsing hints. (These are analogous to `parseStats` and `TableStatistic` from the stats cache.) Informs: cockroachdb#153633 Release note: None
1. During `ReloadHintsIfStale` we now call `Validate` and `InjectHints` using the donor to perform the AST rewrite. We save the rewritten AST in the statement separately from the original AST. 2. We wrap `prepareUsingOptimizer` and `makeOptimizerPlan` with functions that first try preparing / planning with injected hints, and then try again without injected hints in case the injected hints are invalid. With these two pieces we can now actually perform hint injection. Fixes: cockroachdb#153633 Release note (sql change): A new "hint injection" ability has been added, which allows operators to dynamically inject inline hints into statements, without modifying the text of those statements. Hints can be injected using the builtin function `crdb_internal.inject_hint` with the target statement fingerprint to rewrite. For example, to add an index hint to the statement `SELECT * FROM my_table WHERE col = 3`, use: ``` SELECT crdb_internal.inject_hint( 'SELECT * FROM my_table WHERE col = _', 'SELECT * FROM my_table@my_table_col_idx WHERE col = _' ); ``` Whenever a statement is executed matching statement fingerprint `SELECT * FROM my_table WHERE col = _`, it will first be rewritten to include the injected index hint.
If we build a memo with hint injection, and then later we realize that memo won't work (maybe because we discover the hint is unsatisfiable during execution of a prepared statement) we need to invalidate the cached memo. To do this, add a usingHintInjection field which tells the memo staleness check whether we're trying with or without hint injection. Also, in a related but separate change, this commit adds all matching HintIDs to the optimizer metadata so that we don't invalidate cached memos if the hintsGeneration changed due to some unrelated statement hints changing. Informs: cockroachdb#153633 Release note: None
michae2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
pkg/sql/plan_opt.go line 563 at r3 (raw file):
ast = opc.p.stmt.ASTWithInjectedHints } f.Metadata().SetHintIDs(opc.p.GetHintIDs())
I wasn't completely happy with this call to SetHintIDs. It could also be done inside optbuild somewhere. This seemed like an ok spot because we're mostly not accessing the planner inside optbuild.
pkg/sql/hints/hint_table.go line 168 at r1 (raw file):
func (hint *Hint) Size() int { // TODO(michae2): add size of HintInjectionDonor
Whoops, I forgot to do this. One sec.
michae2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
pkg/sql/logictest/testdata/logic_test/statement_hint_builtins line 216 at r2 (raw file):
query T SELECT regexp_replace(message, E'\\d+', 'x') FROM [SHOW TRACE FOR SESSION] WHERE message LIKE '%injected hints%'
I think some of these tracing checks will need the same adjustment as in #158026.
yuzefovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see this come together! I skimmed the PR and had a few questions, will defer to others for closer review.
@yuzefovich reviewed 6 of 6 files at r1, 7 of 7 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @michae2)
pkg/sql/logictest/testdata/logic_test/statement_hint_builtins line 216 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
I think some of these tracing checks will need the same adjustment as in #158026.
Yes, opc.log appends the statement to all messages under high vmodule config.
-- commits line 37 at r2:
nit: have we settled on the naming? It's probably the first time we're documenting the statement hints feature, so we should agree on terminology.
pkg/sql/logictest/testdata/logic_test/statement_hint_builtins line 251 at r2 (raw file):
statement ok SELECT a, x FROM abc JOIN xy ON y = b WHERE a = 5
nit: might also be nice to EXPLAIN the statement before hint injection to show that lookup join is used.
pkg/sql/hints/hint_table.go line 93 at r1 (raw file):
hintID, fingerprint, hint, err := parseHint(it.Cur(), fingerprintFlags) if err != nil { log.Dev.Warningf(
nit: I wonder whether we should add a limiter to this warning, like once a second, but maybe it'd only matter if we have the hint cache thrashing.
pkg/sql/hints/BUILD.bazel line 27 at r1 (raw file):
"//pkg/sql/hintpb", "//pkg/sql/isql", "//pkg/sql/parser",
nit: I now have uneasy feeling when adding new dependencies on sql/parser since that package is often the build bottleneck. It might be nice to use parserutils.ParseOne to avoid that.
pkg/sql/statement.go line 185 at r2 (raw file):
) for i, hint := range s.Hints {
Do we plan to document the scenario when multiple hints can be applied to a single stmt? In the current version, the first (in ASC creation order) successful injection wins, but perhaps a more intuitive behavior would be for the latest hint to win?
pkg/sql/logictest/testdata/logic_test/statement_hint_builtins line 450 at r3 (raw file):
SET tracing = off # (This should be empty.)
nit: there is query empty directive for this (or statement count 0).
pkg/sql/logictest/testdata/logic_test/statement_hint_builtins line 532 at r3 (raw file):
injected hints from external statement hint x trying preparing with injected hints preparing with injected hints failed with: index "abc_foo" not found
Can we avoid trying to use the same invalid hint twice?
sql: add parseHint step when loading hint into hints cache
When loading an external statement hint into the statement hints cache,
we might need to call some function to get the hint ready for use. (For
hint injections, this function is
tree.NewHintInjectionDonorwhichparses and walks the donor statement fingerprint.) This function could
fail, in which case we want to skip over the hint but not return an
error from
GetStatementHintsFromDB. This function could succeed butcreate some extra state which we need to save.
This commit adds a new
parseHintstep which calls any functions neededto get the hint ready, and creates a new
hints.Hintstruct which holdsthe object(s) created when parsing hints. (These are analogous to
parseStatsandTableStatisticfrom the stats cache.)Informs: #153633
Release note: None
sql/*: add hint injection
During
ReloadHintsIfStalewe now callValidateandInjectHintsusing the donor to perform the AST rewrite. We save the rewritten AST
in the statement separately from the original AST.
We wrap
prepareUsingOptimizerandmakeOptimizerPlanwithfunctions that first try preparing / planning with injected hints,
and then try again without injected hints in case the injected hints
are invalid.
With these two pieces we can now actually perform hint injection.
Fixes: #153633
Release note (sql change): A new "hint injection" ability has been
added, which allows operators to dynamically inject inline hints into
statements, without modifying the text of those statements. Hints can be
injected using the builtin function
crdb_internal.inject_hintwith thetarget statement fingerprint to rewrite. For example, to add an index
hint to the statement
SELECT * FROM my_table WHERE col = 3, use:Whenever a statement is executed matching statement fingerprint
SELECT * FROM my_table WHERE col = _, it will first be rewrittento include the injected index hint.
sql/*: invalidate cached memos after hint injection changes
If we build a memo with hint injection, and then later we realize that
memo won't work (maybe because we discover the hint is unsatisfiable
during execution of a prepared statement) we need to invalidate the
cached memo.
To do this, add a usingHintInjection field which tells the memo
staleness check whether we're trying with or without hint injection.
Also, in a related but separate change, this commit adds all matching
HintIDs to the optimizer metadata so that we don't invalidate cached
memos if the hintsGeneration changed due to some unrelated statement
hints changing.
Informs: #153633
Release note: None