sql: support statement hint in routine body with SQL language

[ZhouXing19]

informs: #162627

This commit is to ensure statements within routines defined by SQL
language respect hint injections.

Note that the routines written in PLpgSQL language are not covered in
this commit.

Release note (bug fix): ensure statements within routines body defined by SQL
language respect hint injections.

[trunk-io]

🚫 This pull request was requested to be canceled by @ZhouXing19, so it was removed from the merge queue. See more details here.

• To merge this pull request, check the box to the left or comment /trunk merge below.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[DrewKimball]

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

---

pkg/sql/opt/optbuilder/routine.go line 466 at r1 (raw file):

			body[i] = stmtScope.expr
			bodyProps[i] = stmtScope.makePhysicalProps()
			bodyASTs[i] = maybeHintedStmt

I think this will cause statement stats to be recorded for the hinted AST fingerprint, which I'm not sure we want. At least, we don't do that for connExecutor-level SQL statements.

---

pkg/sql/opt_catalog.go line 490 at r1 (raw file):

		return ast
	}
	stmtHints, hintIDs := hintCache.MaybeGetStatementHints(ctx, fingerprint, fmtFlags)

I think there's a problem here - the hint IDs need to be tracked on the opt.Metadata, or else (once I get the fix in #162057 merged) cached plans with routines will not be invalidated after a statement in the routine is hinted (or un-hinted). I think at least in the longer term we should:

1. Change Metadata.SetHintIDs to Metadata.AddHintIDs.
2. Instead of calling SetHintIDs up at the connExecutor level, call them in the optbuilder for buildStmtAtRootWithScope, or some similar central location.

If we do that, every top-level statement will apply hints and also register the IDs of those hints with the metadata. Although, there might be some places where we don't want to apply hints, like formatting the body statements of a newly-created routine to be stored in the descriptor.

This might be a heavier lift than we need to just support routine statement hints, so if you see an easier way to solve this for now, feel free to go with that.

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[ZhouXing19]

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

---

pkg/sql/opt_catalog.go line 490 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I think there's a problem here - the hint IDs need to be tracked on the opt.Metadata, or else (once I get the fix in #162057 merged) cached plans with routines will not be invalidated after a statement in the routine is hinted (or un-hinted). I think at least in the longer term we should:

1. Change Metadata.SetHintIDs to Metadata.AddHintIDs.
2. Instead of calling SetHintIDs up at the connExecutor level, call them in the optbuilder for buildStmtAtRootWithScope, or some similar central location.

If we do that, every top-level statement will apply hints and also register the IDs of those hints with the metadata. Although, there might be some places where we don't want to apply hints, like formatting the body statements of a newly-created routine to be stored in the descriptor.

This might be a heavier lift than we need to just support routine statement hints, so if you see an easier way to solve this for now, feel free to go with that.

Added the second commit for the hint cache generation idea that was discussed offline.

---

pkg/sql/opt/optbuilder/routine.go line 466 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I think this will cause statement stats to be recorded for the hinted AST fingerprint, which I'm not sure we want. At least, we don't do that for connExecutor-level SQL statements.

Reverted. Though I'm curious, should we record statement stats for the hinted AST somewhere in the future? It feels like useful...

[DrewKimball]

I don't think loading the generation from the cache here is correct, since it could mean we get a later generation than the one for which hints were actually loaded. I think we should probably just use the same generation recorded in the DependencyDigest in the opt.Metadata.

[ZhouXing19]

Hmm, I'm not sure if we want to reuse DependencyDigest.HintsGeneration. 3 reasons:

1. When we're building the routine for the first time, md.digest.depDigest is zeroed out -- the digest is only populated later, at the end of a successful CheckDependencies.
2. Regarding the concern about getting "a later generation than the one for which hints were actually loaded": the ordering is, I think, actually the safe direction. b.evalCtx.Planner.GetHintCacheGeneration() is called before the loop that calls TryRewriteWithStmtHints, so if the cache advances between the read and the hint loading, the recorded generation (N) is earlier than the hints actually used (from N+1). On the next stale check, current_gen (N+1) != recorded_gen (N) -> memo stale -> plan rebuilt. This is a conservative false positive (unnecessary invalidation), never a false negative (missed invalidation).
3. My understanding is that depDigest.HintsGeneration and hintCacheGenerationInRoutines actually serve independent purposes and are written at different stages. The former is a global cache generation snapshot, is written when checking the staleness of memo and used as a fast-path digest comparison that gates all further checks. The latter is written while building the routine.

---

I also considered if we should have a depDigest.HintsGenerationInRoutines rather than metadata.hintCacheGenerationInRoutines, but my concern is that seems like DependencyDigest is set in an atomic manner (?). So if we still want to set the generation for routines when building the memo, I doubt if it should live with the existing fields.

Sorry for the lengthy response and happy to change this if I'm missing something — just wanted to walk through the reasoning to make sure we're on the same page.

[DrewKimball]

Thanks for the explanation. I didn't consider (1) and (3) - those are good reasons to track the generation separately from the digest. WRT (2), I think it's still not correct because we could build multiple routines, in which case we might advance the stored generation after grabbing hints for a routine we built earlier. On way to solve this might be to store the hint cache generation unconditionally at the top level in connExecutor, and then only check it if there are >0 routine deps.

[ZhouXing19]

Good point, I didn't think of the case where multiple routines being built in one statement. and storing the generation unconditionally at connExecutor makes sense to me.

[cockroach-teamcity]

⚪ Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	10.25m ±6%	10.22m ±5%	~	p=0.935 n=15
⚪ allocs/op	8.096k ±1%	8.094k ±0%	~	p=0.751 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/c8911e8/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/c8911e8a814e27e3de5bdae1301e9cca65d8c3d8/bin/pkg_sql_tests benchdiff/c8911e8/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/c8911e8/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/e904df3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/e904df395706e7e5941eb928130504fa0d5feda1/bin/pkg_sql_tests benchdiff/e904df3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/e904df3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=e904df3 --new=c8911e8 --memprofile ./pkg/sql/tests

⚪ Sysbench [KV, 3node, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	3.107m ±1%	3.113m ±1%	~	p=0.744 n=15
⚪ allocs/op	2.104k ±0%	2.104k ±0%	~	p=0.554 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/c8911e8/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/c8911e8a814e27e3de5bdae1301e9cca65d8c3d8/bin/pkg_sql_tests benchdiff/c8911e8/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/c8911e8/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/e904df3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/e904df395706e7e5941eb928130504fa0d5feda1/bin/pkg_sql_tests benchdiff/e904df3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/e904df3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=e904df3 --new=c8911e8 --memprofile ./pkg/sql/tests

🔴 Sysbench [KV, 3node, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note
🔴 sec/op	2.895m ±1%	2.938m ±2%	+1.51%	p=0.021 n=15
⚪ allocs/op	4.216k ±0%	4.218k ±0%	~	p=0.429 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/c8911e8/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/c8911e8a814e27e3de5bdae1301e9cca65d8c3d8/bin/pkg_sql_tests benchdiff/c8911e8/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/c8911e8/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/e904df3/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/e904df395706e7e5941eb928130504fa0d5feda1/bin/pkg_sql_tests benchdiff/e904df3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/e904df3/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=e904df3 --new=c8911e8 --memprofile ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/c8911e8a814e27e3de5bdae1301e9cca65d8c3d8/22775235526-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/e904df395706e7e5941eb928130504fa0d5feda1/22775235526-1/\* old/

built with commit: c8911e8a814e27e3de5bdae1301e9cca65d8c3d8

[DrewKimball]

BTW, you don't need to rebase this on #162057. I'm currently tracking down a bug either in the fix I made, or one that was revealed by the new caching behavior. So, it might take a while for #162057 to get merged.

[ZhouXing19]

Thanks for the heads up! I'll drop the second commit (hint cache generation invalidation) from this PR and just keep the first one (hint support in routine bodies). The cache invalidation logic depends on #162057 for meaningful testing — without routine plan caching, plans are always rebuilt, so there's nothing to invalidate. I'll put it up as a separate PR once #162057 lands.

[DrewKimball]

Nice work!

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

[cockroach-teamcity]

⚪ Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	10.23m ±1%	10.29m ±2%	~	p=0.744 n=15
⚪ allocs/op	8.100k ±1%	8.103k ±1%	~	p=0.588 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/f18f3fc/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/f18f3fc6ce446eefee2e0bfac50960d7bc2b291c/bin/pkg_sql_tests benchdiff/f18f3fc/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/f18f3fc/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/748defa/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/748defa603da84ae946cc7b8e16222cc404ff44e/bin/pkg_sql_tests benchdiff/748defa/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/748defa/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=748defa --new=f18f3fc --memprofile ./pkg/sql/tests

⚪ Sysbench [KV, 3node, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	3.168m ±1%	3.183m ±1%	~	p=0.436 n=15
⚪ allocs/op	2.105k ±0%	2.105k ±0%	~	p=1.000 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/f18f3fc/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/f18f3fc6ce446eefee2e0bfac50960d7bc2b291c/bin/pkg_sql_tests benchdiff/f18f3fc/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/f18f3fc/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/748defa/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/748defa603da84ae946cc7b8e16222cc404ff44e/bin/pkg_sql_tests benchdiff/748defa/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/748defa/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=748defa --new=f18f3fc --memprofile ./pkg/sql/tests

⚪ Sysbench [KV, 3node, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	2.909m ±1%	2.926m ±2%	~	p=0.233 n=15
⚪ allocs/op	4.219k ±0%	4.220k ±0%	~	p=0.862 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/f18f3fc/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/f18f3fc6ce446eefee2e0bfac50960d7bc2b291c/bin/pkg_sql_tests benchdiff/f18f3fc/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/f18f3fc/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/748defa/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/748defa603da84ae946cc7b8e16222cc404ff44e/bin/pkg_sql_tests benchdiff/748defa/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/748defa/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=748defa --new=f18f3fc --memprofile ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/f18f3fc6ce446eefee2e0bfac50960d7bc2b291c/22783039261-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/748defa603da84ae946cc7b8e16222cc404ff44e/22783039261-1/\* old/

built with commit: f18f3fc6ce446eefee2e0bfac50960d7bc2b291c

[cockroach-teamcity]

⚪ Sysbench [SQL, 3node, oltp_read_write]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	10.76m ±2%	10.72m ±2%	~	p=0.436 n=15
⚪ allocs/op	8.137k ±1%	8.127k ±1%	~	p=0.767 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/01f2702/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/01f2702e3c9a64013ea3e210c9741418c67f6aec/bin/pkg_sql_tests benchdiff/01f2702/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/01f2702/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7ca1f2b/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7ca1f2bf5201bf4d60844ff404a9eb788296179d/bin/pkg_sql_tests benchdiff/7ca1f2b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7ca1f2b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=7ca1f2b --new=01f2702 --memprofile ./pkg/sql/tests

⚪ Sysbench [KV, 3node, oltp_read_only]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	3.063m ±1%	3.060m ±1%	~	p=0.512 n=15
⚪ allocs/op	2.103k ±0%	2.104k ±0%	~	p=0.650 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/01f2702/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/01f2702e3c9a64013ea3e210c9741418c67f6aec/bin/pkg_sql_tests benchdiff/01f2702/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/01f2702/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7ca1f2b/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7ca1f2bf5201bf4d60844ff404a9eb788296179d/bin/pkg_sql_tests benchdiff/7ca1f2b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7ca1f2b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_read_only$ --old=7ca1f2b --new=01f2702 --memprofile ./pkg/sql/tests

⚪ Sysbench [KV, 3node, oltp_write_only]

Metric	Old Commit	New Commit	Delta	Note
⚪ sec/op	2.839m ±1%	2.833m ±1%	~	p=0.412 n=15
⚪ allocs/op	4.208k ±0%	4.208k ±0%	~	p=0.992 n=15

Reproduce

benchdiff binaries:

mkdir -p benchdiff/01f2702/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/01f2702e3c9a64013ea3e210c9741418c67f6aec/bin/pkg_sql_tests benchdiff/01f2702/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/01f2702/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/7ca1f2b/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/7ca1f2bf5201bf4d60844ff404a9eb788296179d/bin/pkg_sql_tests benchdiff/7ca1f2b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/7ca1f2b/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

# NB: for best (most stable) results, also add a suitable `--benchtime` that
# results in ~1s to ~5s of benchmark runs. For example, if ops average ~3ms, a
# benchtime of `1000x` is appropriate.
#
# Some benchmarks (in particular BenchmarkSysbench) output additional memory
# profiles covering only the execution (excluding the setup/teardown) - those
# should be preferred for analysis since they more closely correspond to what's
# reported as B/op and alloc/op.
benchdiff --run=^BenchmarkSysbench/KV/3node/oltp_write_only$ --old=7ca1f2b --new=01f2702 --memprofile ./pkg/sql/tests

Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/01f2702e3c9a64013ea3e210c9741418c67f6aec/22784728158-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/7ca1f2bf5201bf4d60844ff404a9eb788296179d/22784728158-1/\* old/

built with commit: 01f2702e3c9a64013ea3e210c9741418c67f6aec

[ZhouXing19]

TFTR!

/trunk merge

[michae2]

Nice job! But there's a problem.

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

---

pkg/sql/opt/optbuilder/routine.go line 457 at r13 (raw file):

			hintStmtFingerprint := tree.FormatStatementHideConstants(stmts[i].AST, fmtFlags)
			maybeHintedStmt := b.catalog.TryRewriteWithStmtHints(b.ctx, hintStmtFingerprint, stmts[i].AST, fmtFlags)
			stmtScope := b.buildStmtAtRootWithScope(maybeHintedStmt, nil /* desiredTypes */, bodyScope)

There's a problem here: we need to be able to fall back to the original AST if we get an error during optimization of the rewritten AST. This is because we can't guarantee the hint will always be satisfied by the optimizer.

The normal code path does this in two places in plan_opt, where it tries optimizing twice, once with the rewritten AST and once with the original AST if the rewritten AST didn't work. Here's one of those spots (notice the two calls to makeOptimizerPlanInternal):

cockroach/pkg/sql/plan_opt.go

Lines 293 to 318 in 7ca1f2b

	func (p *planner) makeOptimizerPlan(ctx context.Context) error {
	ctx, sp := tracing.ChildSpan(ctx, "optimizer")
	defer sp.Finish()
	p.curPlan.init(&p.stmt, &p.instrumentation)
	opc := &p.optPlanningCtx
	// If there are externally-injected hints, first try planning with the
	// injected hints.
	if p.stmt.ASTWithInjectedHints != nil {
	opc.log(ctx, "trying planning with injected hints")
	p.usingHintInjection = true
	opc.reset(ctx)
	err := p.makeOptimizerPlanInternal(ctx)
	p.usingHintInjection = false
	if !errorDueToInjectedHint(err) {
	return err
	}
	// Do not return the error. If semantic analysis or optimization failed, try
	// planning again without injected hints.
	log.Eventf(ctx, "planning with injected hints failed with: %v", err)
	opc.log(ctx, "falling back to planning without injected hints")
	}
	opc.reset(ctx)
	return p.makeOptimizerPlanInternal(ctx)
	}

For routines, I'm not quite sure where we call Optimize(), but we might need to build two stmtScopes here... or change this field to a closure that we could invoke again if the call to Optimize() failed.

---

pkg/sql/opt/optbuilder/routine.go line 472 at r13 (raw file):

				bodyTags[i] = ""
			} else {
				bodyTags[i] = maybeHintedStmt.StatementTag()

nit: I would continue to use the tag from the original AST (this is also what conn executor does).

[ZhouXing19]

/trunk cancel

[DrewKimball]

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

---

pkg/sql/opt/optbuilder/routine.go line 457 at r13 (raw file):

Previously, michae2 (Michael Erickson) wrote…

There's a problem here: we need to be able to fall back to the original AST if we get an error during optimization of the rewritten AST. This is because we can't guarantee the hint will always be satisfied by the optimizer.

The normal code path does this in two places in plan_opt, where it tries optimizing twice, once with the rewritten AST and once with the original AST if the rewritten AST didn't work. Here's one of those spots (notice the two calls to makeOptimizerPlanInternal):

cockroach/pkg/sql/plan_opt.go

Lines 293 to 318 in 7ca1f2b

	func (p *planner) makeOptimizerPlan(ctx context.Context) error {
	ctx, sp := tracing.ChildSpan(ctx, "optimizer")
	defer sp.Finish()
	p.curPlan.init(&p.stmt, &p.instrumentation)
	opc := &p.optPlanningCtx
	// If there are externally-injected hints, first try planning with the
	// injected hints.
	if p.stmt.ASTWithInjectedHints != nil {
	opc.log(ctx, "trying planning with injected hints")
	p.usingHintInjection = true
	opc.reset(ctx)
	err := p.makeOptimizerPlanInternal(ctx)
	p.usingHintInjection = false
	if !errorDueToInjectedHint(err) {
	return err
	}
	// Do not return the error. If semantic analysis or optimization failed, try
	// planning again without injected hints.
	log.Eventf(ctx, "planning with injected hints failed with: %v", err)
	opc.log(ctx, "falling back to planning without injected hints")
	}
	opc.reset(ctx)
	return p.makeOptimizerPlanInternal(ctx)
	}

For routines, I'm not quite sure where we call Optimize(), but we might need to build two stmtScopes here... or change this field to a closure that we could invoke again if the call to Optimize() failed.

Hm, that's tricky. Optimize() could happen when the main query is optimized if the routine is inlined, or else it'll happen during execution when the routine is called with concrete arguments supplied. So maybe we need to:

1. Disable inlining if there are hints (oof), and:
2. store both the hinted and unhinted built+normalized body statement so that when planning the body statement we can fall back to the original if there's an error.

[michae2]

[notes from project meeting]

• [drew] we might need to make the optbuild step lazy, so that it happens at the same place as optimization
• [jane] if user injects hints / sets variables for the statement calling the routine, and also for statements within the routine, is there a conflict?
• [drew] I think we would apply the routine body hints "on top" and then roll those back. rewrite_inline_hints shouldn't be a problem
• [jane] let's continue discussion after branch cut
• [michael] "redo" during inlining is confusing
• [drew] maybe don't inline hint?
• [becca] if body statement is inlined is that after hints are already applied?
• [drew] yes, but hints could be invalid, could cause entire query to fail to optimize. then we need to retry, but hard to know which hint caused it to fail
• [becca] would it have failed anyway? hint might apply before udf inlined, and then not apply after?
• [michael] delay between applying hint and finding out if hint works is the problem
• [becca] I see, fallback logic would be trickier after inlining
• [drew] one option we could still take is instead of matching from statement_hints table, could use alter routine command so hint is associated with routine descriptor, and that would come with semantics that routine doesn't get inlined. routines are different from normal SQL: we can run an alter, don't necessarily need to use hints table. and if you wanted to rewrite the inline hints, could simply alter the routine anyway!
• [michael] can we also set variables in routines?
• [drew] no, that's true. also plan pinning
• [jane] are we saying that rather than rewrite statement when building, run alter routine?
• [drew] routines, since their sql is defined in database, you can just alter the database. hints are necessary for ordinary sql statements
• [becca] until we support dynamic sql
• [drew] I think the same argument would hold, that you could change the dynamic sql to add a hint
• [becca] but couldn't the statement be passed in from the application?
• [drew] I think it's usually used to run the same statement on multiple tables.... but that's a good point... we could do something before the dynamic sql statement, some special statement
• [michael] I think this undo / redo thing is specific to rewrite_inline_hints
• [drew] session variables might apply to building or optimization or execution, which is a little tricky. but not insurmountable
• [becca] we can mark this as a known limitation for 26.2. workaround to alter the body of the routine

[ZhouXing19]

[weekly meeting]

1. We also need to consider similar inline issue with views.
2. Within routine body, applying hint to one sql stmt might interfere with applying hint on another sql stmt. Michael mentioned some cases with lookup join.
3. We have 2 issues here: (a) inlining, as we won't be able to figure out what belongs to the original routine body and what does not; (b) the seperation of the optimization and building -- if the issue with applied hints doesn't not happen until the build stage, it might be too late to rollback to the unhinted version.
4. Might worth looking at what other systems do.