Skip to content

Commit 6d09b87

Browse files
committed
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.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: #153633 Release note: None
1 parent 0f2bc7b commit 6d09b87

File tree

9 files changed

+224
-87
lines changed

9 files changed

+224
-87
lines changed

pkg/sql/hints/BUILD.bazel

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,14 @@ go_library(
2424
"//pkg/sql/catalog/systemschema",
2525
"//pkg/sql/hintpb",
2626
"//pkg/sql/isql",
27+
"//pkg/sql/parserutils",
2728
"//pkg/sql/rowenc",
2829
"//pkg/sql/sem/tree",
2930
"//pkg/sql/sessiondata",
3031
"//pkg/sql/types",
3132
"//pkg/util/buildutil",
3233
"//pkg/util/cache",
34+
"//pkg/util/errorutil",
3335
"//pkg/util/hlc",
3436
"//pkg/util/log",
3537
"//pkg/util/metamorphic",
@@ -59,11 +61,14 @@ go_test(
5961
"//pkg/security/securityassets",
6062
"//pkg/security/securitytest",
6163
"//pkg/server",
64+
"//pkg/settings/cluster",
6265
"//pkg/sql",
6366
"//pkg/sql/catalog/descs",
6467
"//pkg/sql/hintpb",
6568
"//pkg/sql/isql",
69+
"//pkg/sql/parserutils",
6670
"//pkg/sql/randgen",
71+
"//pkg/sql/sem/tree",
6772
"//pkg/sql/stats",
6873
"//pkg/storage/fs",
6974
"//pkg/testutils",

pkg/sql/hints/hint_cache.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
2626
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
2727
"github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema"
28-
"github.com/cockroachdb/cockroach/pkg/sql/hintpb"
2928
"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
3029
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
3130
"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -406,8 +405,8 @@ func (c *StatementHintsCache) GetGeneration() int64 {
406405
// plans). It returns nil if the statement has no hints, or there was an error
407406
// retrieving them.
408407
func (c *StatementHintsCache) MaybeGetStatementHints(
409-
ctx context.Context, statementFingerprint string,
410-
) (hints []hintpb.StatementHintUnion, ids []int64) {
408+
ctx context.Context, statementFingerprint string, fingerprintFlags tree.FmtFlags,
409+
) (hints []Hint, ids []int64) {
411410
hash := fnv.New64()
412411
_, err := hash.Write([]byte(statementFingerprint))
413412
if err != nil {
@@ -430,7 +429,7 @@ func (c *StatementHintsCache) MaybeGetStatementHints(
430429
if !ok {
431430
// The plan hints were evicted from the cache. Retrieve them from the
432431
// database and add them to the cache.
433-
return c.addCacheEntryLocked(ctx, statementHash, statementFingerprint)
432+
return c.addCacheEntryLocked(ctx, statementHash, statementFingerprint, fingerprintFlags)
434433
}
435434
entry := e.(*cacheEntry)
436435
c.maybeWaitForRefreshLocked(ctx, entry, statementHash)
@@ -464,8 +463,11 @@ func (c *StatementHintsCache) maybeWaitForRefreshLocked(
464463
// other queries to wait for the result via sync.Cond. Note that the lock is
465464
// released while reading from the db, and then reacquired.
466465
func (c *StatementHintsCache) addCacheEntryLocked(
467-
ctx context.Context, statementHash int64, statementFingerprint string,
468-
) (hints []hintpb.StatementHintUnion, ids []int64) {
466+
ctx context.Context,
467+
statementHash int64,
468+
statementFingerprint string,
469+
fingerprintFlags tree.FmtFlags,
470+
) (hints []Hint, ids []int64) {
469471
c.mu.AssertHeld()
470472

471473
// Add a cache entry that other queries can find and wait on until we have the
@@ -483,7 +485,7 @@ func (c *StatementHintsCache) addCacheEntryLocked(
483485
defer c.mu.Lock()
484486
log.VEventf(ctx, 1, "reading hints for query %s", statementFingerprint)
485487
entry.ids, entry.fingerprints, entry.hints, err =
486-
GetStatementHintsFromDB(ctx, c.db.Executor(), statementHash)
488+
GetStatementHintsFromDB(ctx, c.db.Executor(), statementHash, fingerprintFlags)
487489
log.VEventf(ctx, 1, "finished reading hints for query %s", statementFingerprint)
488490
}()
489491

@@ -517,16 +519,14 @@ type cacheEntry struct {
517519
// be duplicate entries in the fingerprints slice.
518520
// TODO(drewk): consider de-duplicating the fingerprint strings to reduce
519521
// memory usage.
520-
hints []hintpb.StatementHintUnion
522+
hints []Hint
521523
fingerprints []string
522524
ids []int64
523525
}
524526

525527
// getMatchingHints returns the plan hints and row IDs for the given
526528
// fingerprint, or nil if they don't exist. The results are in order of row ID.
527-
func (entry *cacheEntry) getMatchingHints(
528-
statementFingerprint string,
529-
) (hints []hintpb.StatementHintUnion, ids []int64) {
529+
func (entry *cacheEntry) getMatchingHints(statementFingerprint string) (hints []Hint, ids []int64) {
530530
for i := range entry.hints {
531531
if entry.fingerprints[i] == statementFingerprint {
532532
hints = append(hints, entry.hints[i])

0 commit comments

Comments
 (0)