sql: expose an ability to request redacted stmt bundles #123859
Conversation
|
This change is |
yuzefovich
force-pushed
the
bundle-redacted
branch
4 times, most recently
from
d5a618b to
ddec74d
Compare
yuzefovich
requested review from
nkodali,
a team and
DrewKimball
and removed request for
a team and
nkodali
There was a problem hiding this comment.
Reviewed 31 of 31 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/instrumentation.go line 635 at r1 (raw file):
// explicitly. We also need to set RedactValues when this bundle // is being collected for the request that asked for redaction. ih.explainFlags.RedactValues = true
[nit] Should we do this in Setup and setupWithPlanGist when ih.diagRequest is first set?
This commit extends the stmt bundle collection infrastructure to support requesting redacted stmt bundles. Previously, this was only possible via an explicit `EXPLAIN ANALYZE (DEBUG, REDACT)` stmt, but this commit adds the support via overloads to `crdb_internal.request_statement_bundle` builtin as well as the server API. This paves the way for requesting the redacted bundles via the DB Console, but it'll be done separately. To support this we need to add another boolean column to `system.statement_diagnostics_requests` table to indicate whether a request is for the redacted bundle or not. This requires adding a migration in which we populate the existing rows with all `false` values. In order to reduce the amount of code churn and simplify this change, this commit repurposes some of the existing code that we introduced the last time we added a migration for plan-gist matching in stmt bundles. In other words, this commit simultenously removes some stale code and handles the new column. Release note: None
yuzefovich
force-pushed
the
bundle-redacted
branch
from
ddec74d to
1c3cea8
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/instrumentation.go line 635 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit] Should we do this in
SetupandsetupWithPlanGistwhenih.diagRequestis first set?
Done.
|
bors r+ |
There was a problem hiding this comment.
Nicely done!
Reviewed 26 of 31 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status:
pkg/upgrade/upgrades/redacted_stmt_diagnostics_requests.go line 31 at r2 (raw file):
createCompletedIdx = ` CREATE INDEX completed_idx ON system.statement_diagnostics_requests (completed, ID)
nit: Should this be named with a _v3 suffix for future migrations? Or did you consider that a bad pattern?
pkg/sql/logictest/testdata/logic_test/gen_test_objects line 210 at r2 (raw file):
protected_ts_meta "vers ion" bigint "ran gelog" "%rangeID" bigint
Any idea what happened here?
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/redacted_stmt_diagnostics_requests.go line 31 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: Should this be named with a
_v3suffix for future migrations? Or did you consider that a bad pattern?
I thought it would be cleaner to just have completed_idx. We've modified this index a few times in the past, and I think every time it was v -> v2 or v2 -> v transition. At this point it would have been probably v6 or something if we simply incremented it every time. I don't really have an opinion here, just v seems cleanest to me. It don't think I've seen any guidelines for migrations in general either, perhaps it might be worth writing up a quick style guide?
pkg/sql/logictest/testdata/logic_test/gen_test_objects line 210 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Any idea what happened here?
IIUC planner.GenerateTestObjects (which powers crdb_internal.generate_test_objects builtin) uses system descriptors, and since we changed the descriptor for a system table, the output of the builtin also changed.
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/redacted_stmt_diagnostics_requests.go line 31 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I thought it would be cleaner to just have
completed_idx. We've modified this index a few times in the past, and I think every time it wasv -> v2orv2 -> vtransition. At this point it would have been probablyv6or something if we simply incremented it every time. I don't really have an opinion here, justvseems cleanest to me. It don't think I've seen any guidelines for migrations in general either, perhaps it might be worth writing up a quick style guide?
Eh, probably not worth a style guide. Just a drive-by comment. I don't have a strong opinion either.
pkg/sql/logictest/testdata/logic_test/gen_test_objects line 210 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
IIUC
planner.GenerateTestObjects(which powerscrdb_internal.generate_test_objectsbuiltin) uses system descriptors, and since we changed the descriptor for a system table, the output of the builtin also changed.
Ahh thanks for the info!
This commit extends the stmt bundle collection infrastructure to support requesting redacted stmt bundles. Previously, this was only possible via an explicit
EXPLAIN ANALYZE (DEBUG, REDACT)stmt, but this commit adds the support via overloads tocrdb_internal.request_statement_bundlebuiltin as well as the server API. This paves the way for requesting the redacted bundles via the DB Console, but it'll be done separately.To support this we need to add another boolean column to
system.statement_diagnostics_requeststable to indicate whether a request is for the redacted bundle or not. This requires adding a migration in which we populate the existing rows with allfalsevalues.In order to reduce the amount of code churn and simplify this change, this commit repurposes some of the existing code that we introduced the last time we added a migration for plan-gist matching in stmt bundles. In other words, this commit simultenously removes some stale code and handles the new column.
Epic: CRDB-37839
Release note: None