sql/hints: use enabled and database columns

[michae2]

sql/hints: read enabled and database columns during hint matching

Read the enabled and database columns from system.statement_hints
when loading hints from the database into the hint cache. Also match on
current database when looking up hints with MaybeGetStatementHints.

Informs: #163522

Release note (sql change): Rewrite-inline-hints rules can now be scoped
to a specific database, and will only apply to matching statements when
the current database also matches. This database can be specified with
an optional third argument to
information_schema.crdb_rewrite_inline_hints.

Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com

---

sql/hints: add enable/disable statement hint builtin

Add information_schema.crdb_enable_statement_hints builtin with two
overloads: (enabled: bool, rowid: int) and (enabled: bool, statement_fingerprint: string). Both require REPAIRCLUSTER privilege.

The implementation uses a new SetHintEnabledInDB function that issues
a DELETE and an INSERT against system.statement_hints to set the
enabled column. The Planner interface gains a
SetStatementHintEnabled method to expose this to builtins.

The DELETE and INSERT are needed to invalidate the hint cache entry on
every node via the rangefeed on the secondary index of
system.statement_hints.

Fixes: #163522

Release note (sql change): This change introduces a new builtin
information_schema.crdb_enable_statement_hints which can be used to
enable or disable statement hints by hint ID or by statement
fingerprint.

Co-Authored-By: roachdev-claude roachdev-claude-bot@cockroachlabs.com

[trunk-io]

😎 Merged successfully - details.

[cockroach-teamcity]

This change is 

[michae2]

Note that the second commit has a problem, which is that after calling information_schema.crdb_enable_statement_hints the hint cache is not invalidated. I'm going to track this in a separate issue and fix in another PR because it will involve another migration.

Otherwise this should be RFAL.

[ZhouXing19]

Nice! Only left some nit comments.

@ZhouXing19 reviewed 17 files and all commit messages, and made 5 comments.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on DrewKimball and michae2).

---

-- commits line 12 at r1:
This seems inaccurate -- we already introduce the 3-arg overload in ba61a64 for information_schema.crdb_rewrite_inline_hints. I think we can just reuse the commit message as the release note.

---

-- commits line 22 at r2:
hmm, should we also have it take an optional database argument, i.e. (enabled: bool, statement_fingerprint: string, database: string), matching rewriteInlineHintsWithDatabaseOverload?

---

pkg/sql/hints/hint_cache.go line 545 at r1 (raw file):

			continue
		}
		if entry.hints[i].Database != "" && entry.hints[i].Database != currentDB {

nit: should we also include currentDB != nil? As in, only filter by db if currentDB is set?

---

pkg/sql/opt/exec/execbuilder/testdata/statement_hint_builtins line 1702 at r2 (raw file):

statement ok
SELECT crdb_internal.await_statement_hints_cache()

nit: is this await is a no-op here (since no rangefeed event fires for enabled-only updates)? Might just remove it? Same for the awaits following the clear_statement_hints_cache() below.

[michae2]

TFTR!

I'm going to try Drew's idea to force hint cache invalidation. Hold off on any more reviews for a sec...

[michae2]

Tried Drew's idea to use a DELETE and an INSERT to force hint cache invalidation. Seems to be working!

This should be RFAL.

@michae2 made 5 comments and resolved 1 discussion.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on DrewKimball and ZhouXing19).

---

-- commits line 12 at r1:

Previously, ZhouXing19 (Jane Xing) wrote…

This seems inaccurate -- we already introduce the 3-arg overload in ba61a64 for information_schema.crdb_rewrite_inline_hints. I think we can just reuse the commit message as the release note.

It's true, I'm cheating a bit by using the release note that I should have used in #164562... I think it's more clear for users to explain things in terms of the builtins but this is confusing. I'll try to reword it.

---

-- commits line 22 at r2:

Previously, ZhouXing19 (Jane Xing) wrote…

hmm, should we also have it take an optional database argument, i.e. (enabled: bool, statement_fingerprint: string, database: string), matching rewriteInlineHintsWithDatabaseOverload?

Good point! We might want to do this for delete_statement_hints, too?

If it's ok with you I'll open an issue and do this in another PR.

---

pkg/sql/hints/hint_cache.go line 545 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: should we also include currentDB != nil? As in, only filter by db if currentDB is set?

Good catch! Done.

---

pkg/sql/opt/exec/execbuilder/testdata/statement_hint_builtins line 1702 at r2 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: is this await is a no-op here (since no rangefeed event fires for enabled-only updates)? Might just remove it? Same for the awaits following the clear_statement_hints_cache() below.

Good point. I've now removed the clear_statement_hints_cache since the fix seems to be working!

[michae2]

@michae2 made 1 comment.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on DrewKimball and ZhouXing19).

---

-- commits line 12 at r1:

Previously, michae2 (Michael Erickson) wrote…

It's true, I'm cheating a bit by using the release note that I should have used in #164562... I think it's more clear for users to explain things in terms of the builtins but this is confusing. I'll try to reword it.

Updated the release note.

[michae2]

/trunk merge

[DrewKimball]

Nice work! Belated LGTM