sql: add transaction_fingerprint_id to system.statement_statistics pk#69320
Conversation
|
This change is |
dbca9f9 to
3440eab
Compare
maryliag
left a comment
There was a problem hiding this comment.
From an observability perspective
But get the approval from SQL Schema too
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @Azhng)
3440eab to
f708a8c
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 480 at r2 (raw file):
plan JSONB NOT NULL, PRIMARY KEY (aggregated_ts, fingerprint_id, transaction_fingerprint_id, plan_hash, app_name, node_id)
How did you decide where to put this in the primary key? Is the expectation that people will be looking up statements by transaction_fingerprint_id most often?
pkg/sql/catalog/systemschema/system.go, line 482 at r2 (raw file):
PRIMARY KEY (aggregated_ts, fingerprint_id, transaction_fingerprint_id, plan_hash, app_name, node_id) USING HASH WITH BUCKET_COUNT = 8, INDEX "fingerprint_stats_idx" (fingerprint_id, aggregated_ts, transaction_fingerprint_id, plan_hash, app_name, node_id),
I should have caught this earlier, it's a bit unconventional to explicitly state these primary key columns in the index definition. All secondary indexes have all of the primary index columns in their key. I think that this should just be INDEX (fingerprint_id) to better capture its intention.
f708a8c to
0684691
Compare
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 480 at r2 (raw file):
Previously, ajwerner wrote…
How did you decide where to put this in the primary key? Is the expectation that people will be looking up statements by
transaction_fingerprint_idmost often?
My reasoning here is that this is primary for Transaction Detail Page in the DB Console. Since there we will be looking at the statements belonging to a transaction_fingerprint_id.
pkg/sql/catalog/systemschema/system.go, line 482 at r2 (raw file):
Previously, ajwerner wrote…
I should have caught this earlier, it's a bit unconventional to explicitly state these primary key columns in the index definition. All secondary indexes have all of the primary index columns in their key. I think that this should just be
INDEX (fingerprint_id)to better capture its intention.
Done.
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 480 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
My reasoning here is that this is primary for Transaction Detail Page in the DB Console. Since there we will be looking at the statements belonging to a
transaction_fingerprint_id.
In that case, do we want to change the secondary index to be (fingerprint_id, transaction_fingerprint_id)? Or do we want it to be (fingerprint_id, aggregated_ts)? I assume that the way this query will work is by looking up the transaction, which, I assume, has the set of statement fingerprints. Then, we'd go fetch each statement fingerprint. That means we want. an index based on fingerprint_id, transaction_fingerprint_id regardless of the timestamp, no?
0684691 to
503091c
Compare
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/systemschema/system.go, line 480 at r2 (raw file):
Previously, ajwerner wrote…
In that case, do we want to change the secondary index to be
(fingerprint_id, transaction_fingerprint_id)? Or do we want it to be(fingerprint_id, aggregated_ts)? I assume that the way this query will work is by looking up the transaction, which, I assume, has the set of statement fingerprints. Then, we'd go fetch each statement fingerprint. That means we want. an index based on fingerprint_id, transaction_fingerprint_id regardless of the timestamp, no?
Good point. Updated the secondary index to include transaction_fingerprint_id.
Previously, statements belong to different transaction fingerprints but has same statement fingerprint IDs get rolled up into a single entry in system.statement_statistics. This commit introduce additional transaction_fingerprint_id column in the system.statement_statistics primary key to enable that differentiation. This commit does not implement filling the new columns. Therefore, the new column is currently left as blank. This will be addressed in the later PRs. Release justification: Category 4: Low risk, high benefit changes to existing functionality Release note (sql change): add transaction_fingerprint_id to system.statement_statistics primary key.
503091c to
5b5c301
Compare
|
@ajwerner RFAL |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
|
TFTR! bors r=ajwerner,maryliag |
|
Build succeeded: |
Previously, statements belong to different transaction fingerprints but
has same statement fingerprint IDs get rolled up into a single entry
in system.statement_statistics.
This commit introduce additional transaction_fingerprint_id column in the
system.statement_statistics primary key to enable that differentiation.
This commit does not implement filling the new columns. Therefore, the
new column is currently left as blank. This will be addressed in the later
PRs.
Release justification: Category 4: Low risk, high benefit changes to existing
functionality
Release note (sql change): add transaction_fingerprint_id to
system.statement_statistics primary key.