sql: add transaction diagnostic bundle tables #152861

dhartunian · 2025-09-02T18:02:14Z

This commit contains a schema change and an upgrade migration to begin adding support for transaction diagnostic bundles.

The new functionality will maintain diagnostic bundle requests and record completed diagnostics very similar to statement diagnostics. We have a transaction_diagnostic_requests table which holds pending requests and marks them as completed. Future PRs will poll this table to update the in-memory registry of pending requests to match against incoming SQL. We also have a transaction_diagnostics table which holds rows for completed diagnostics requests.

Notable details:

Transaction diagnostic requests store a string array of all the fingerprints in the transaction to match incoming statements.
Completed transaction diagnostics store pointers to the statement diagnostic bundles within, and chunks that contain the trace for the entire transaction.
Statement diagnostic bundles will now potentially point to a "parent" transaction diagnostic bundle

Resolves: CRDB-53545
Epic: CRDB-53541

Release note: None

cockroach-teamcity · 2025-09-02T18:02:26Z

This change is

kyle-a-wong

@kyle-a-wong reviewed 10 of 10 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian)

pkg/upgrade/upgrades/v25_4_transaction_diagnostics_tables.go line 21 at r1 (raw file):

	addTransactionDiagnosticsIDColumn = `
ALTER TABLE system.statement_diagnostics
	ADD COLUMN IF NOT EXISTS transaction_diagnostics_id INT8

I think you need to add this to the primary family too

Suggestion:

ADD COLUMN IF NOT EXISTS transaction_diagnostics_id INT8 FAMILY primary

pkg/sql/catalog/systemschema/system.go line 563 at r1 (raw file):

	completed BOOL NOT NULL DEFAULT FALSE,
	transaction_fingerprint_id STRING NOT NULL,
	statement_fingerprints STRING[] NOT NULL,

Do you think these should have the BYTES type, to match the fingerprint_id column types in system.statement_statistics?

Code quote:

	transaction_fingerprint_id STRING NOT NULL,
	statement_fingerprints STRING[] NOT NULL,

pkg/sql/catalog/systemschema/system.go line 563 at r1 (raw file):

	completed BOOL NOT NULL DEFAULT FALSE,
	transaction_fingerprint_id STRING NOT NULL,
	statement_fingerprints STRING[] NOT NULL,

I think statement_fingerprints should be statement_fingerprint_ids, and it should store the IDs, not the fingerprints themselves. What do you think?

pkg/sql/catalog/systemschema/system.go line 579 at r1 (raw file):

	id INT8 DEFAULT unique_rowid() NOT NULL,
	transaction_fingerprint_id STRING NOT NULL,
	statement_fingerprints STRING[] NOT NULL,

same comments here

Code quote:

	transaction_fingerprint_id STRING NOT NULL,
	statement_fingerprints STRING[] NOT NULL,

dhartunian

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson, @kyle-a-wong, and @mw5h)

pkg/sql/catalog/systemschema/system.go line 563 at r1 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

Do you think these should have the BYTES type, to match the fingerprint_id column types in system.statement_statistics?

done.

pkg/sql/catalog/systemschema/system.go line 563 at r1 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

I think statement_fingerprints should be statement_fingerprint_ids, and it should store the IDs, not the fingerprints themselves. What do you think?

done.

pkg/sql/catalog/systemschema/system.go line 579 at r1 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

same comments here

done.

pkg/upgrade/upgrades/v25_4_transaction_diagnostics_tables.go line 21 at r1 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

I think you need to add this to the primary family too

done.

kyle-a-wong

@kyle-a-wong reviewed 1 of 17 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson and @mw5h)

kyle-a-wong

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jeffswenson and @mw5h)

pkg/sql/catalog/systemschema/system.go line 568 at r3 (raw file):

	expires_at TIMESTAMPTZ NULL,
	sampling_probability FLOAT NULL,
	redact BOOL NOT NULL DEFAULT FALSE,

Oops, I just noticed that this table doesnt include a "username" column for the user who initializes the request. Can you add this?

kyle-a-wong

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @jeffswenson, and @mw5h)

pkg/sql/catalog/systemschema/system.go line 568 at r3 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

Oops, I just noticed that this table doesnt include a "username" column for the user who initializes the request. Can you add this?

I think we will also need a transaction_diagnostics_id column in this table so that we can make a download link in db-console

dhartunian

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson, @kyle-a-wong, and @mw5h)

pkg/sql/catalog/systemschema/system.go line 568 at r3 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

I think we will also need a transaction_diagnostics_id column in this table so that we can make a download link in db-console

done on both. also changed to bundle_chunks.

mw5h

@mw5h reviewed 4 of 10 files at r1, 9 of 30 files at r2, 1 of 4 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson and @kyle-a-wong)

mw5h

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson and @kyle-a-wong)

kyle-a-wong

@kyle-a-wong reviewed 1 of 46 files at r6.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @jeffswenson)

blathers-crl · 2025-09-05T19:34:43Z

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.}

dhartunian · 2025-09-07T18:14:33Z

@kyle-a-wong can you take one more look. Here's what I changed:

Removed statement_diagnostics_ids from the transaction_diagnostics table. We already had this reference as a key from statement_diagnostics and this was redundant. I added an index to statement_diagnostics.transaction_diagnostics_id to facilitate lookup.
Added error column to transaction_diagnostics to match statement_diagnostics
Upgrade migration test was missing. Added it.

yuzefovich

Nice!

@yuzefovich reviewed 3 of 10 files at r1, 24 of 30 files at r2, 16 of 17 files at r3, 41 of 46 files at r6, 5 of 5 files at r7, 8 of 8 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @jeffswenson and @kyle-a-wong)

pkg/sql/catalog/systemschema/system.go line 545 at r8 (raw file):

	StatementDiagnosticsTableSchema = `
create table system.statement_diagnostics(
  id INT8 DEFAULT unique_rowid() NOT NULL,

nit: since we're here anyway, it'd be nice to fix the indentation.

dhartunian

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @jeffswenson, @kyle-a-wong, and @yuzefovich)

pkg/sql/catalog/systemschema/system.go line 545 at r8 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: since we're here anyway, it'd be nice to fix the indentation.

done

kyle-a-wong

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @jeffswenson and @yuzefovich)

pkg/upgrade/upgrades/v25_4_transaction_diagnostics_tables_test.go line 86 at r10 (raw file):

		)
	}
	// Validate that the eventlog table has the old

nit: statement diagnostics

Suggestion:

statement diagnostics

dhartunian

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @jeffswenson, @kyle-a-wong, and @yuzefovich)

pkg/upgrade/upgrades/v25_4_transaction_diagnostics_tables_test.go line 86 at r10 (raw file):

Previously, kyle-a-wong (Kyle Wong) wrote…

nit: statement diagnostics

done

dhartunian · 2025-09-08T17:17:22Z

TFTRs everyone!

bors r=kyle-a-wong,yuzefovich,mw5h

dhartunian · 2025-09-08T18:24:41Z

bors r-

This commit contains a schema change and an upgrade migration to begin adding support for transaction diagnostic bundles. The new functionality will maintain diagnostic bundle requests and record completed diagnostics very similar to statement diagnostics. We have a `transaction_diagnostic_requests` table which holds pending requests and marks them as completed. Future PRs will poll this table to update the in-memory registry of pending requests to match against incoming SQL. We also have a `transaction_diagnostics` table which holds rows for completed diagnostics requests. Notable details: - Transaction diagnostic bundle requests store fingerprint IDs for transactions and statements which can be resolved to fingerprints via the SQL Activity tables. - Completed transaction diagnostics reference chunks that contain the trace (and any other metadata) for the entire transaction. - Statement diagnostic bundles will now potentially point to a "parent" transaction diagnostic bundle Also fixed up some whitespace in the diagnostic bundle system schema. Resolves: CRDB-53545 Epic: CRDB-53541 Release note: None

dhartunian · 2025-09-08T20:49:39Z

bors r+

craig · 2025-09-08T21:42:57Z

Build succeeded:

dhartunian requested a review from kyle-a-wong September 2, 2025 18:02

dhartunian requested review from a team as code owners September 2, 2025 18:02

dhartunian mentioned this pull request Sep 2, 2025

CLAUDE: add upgrade-migration command #151572

Closed

kyle-a-wong requested changes Sep 2, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from 9df4106 to 403cd12 Compare September 3, 2025 15:30

dhartunian requested review from a team as code owners September 3, 2025 15:30

dhartunian requested review from jeffswenson, kyle-a-wong and mw5h and removed request for a team September 3, 2025 15:30

dhartunian commented Sep 3, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from 403cd12 to 7c7b6d5 Compare September 3, 2025 18:09

kyle-a-wong approved these changes Sep 3, 2025

View reviewed changes

kyle-a-wong requested changes Sep 3, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from 7c7b6d5 to 232ece7 Compare September 4, 2025 18:32

dhartunian commented Sep 4, 2025

View reviewed changes

dhartunian requested a review from kyle-a-wong September 4, 2025 18:33

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from 232ece7 to daa07a4 Compare September 4, 2025 18:37

mw5h reviewed Sep 4, 2025

View reviewed changes

mw5h approved these changes Sep 4, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch 2 times, most recently from 242ce9e to c3c202f Compare September 5, 2025 14:28

kyle-a-wong approved these changes Sep 5, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from c3c202f to a7873f7 Compare September 5, 2025 19:34

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch 5 times, most recently from 7f45a29 to 3d3f614 Compare September 7, 2025 18:12

dhartunian requested a review from kyle-a-wong September 7, 2025 18:14

yuzefovich approved these changes Sep 8, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch 2 times, most recently from 67e537f to 9964fac Compare September 8, 2025 14:31

dhartunian commented Sep 8, 2025

View reviewed changes

kyle-a-wong reviewed Sep 8, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from 9964fac to 188e2d6 Compare September 8, 2025 15:11

dhartunian commented Sep 8, 2025

View reviewed changes

dhartunian force-pushed the davidh/push-vsqqrowlurtr branch from 188e2d6 to ce8e441 Compare September 8, 2025 18:36

craig bot merged commit 5aeb08d into cockroachdb:master Sep 8, 2025
32 of 33 checks passed

celeste-cockroachdb bot added the target-release-25.4.0 label Sep 8, 2025

sql: add transaction diagnostic bundle tables #152861

sql: add transaction diagnostic bundle tables #152861

Uh oh!

Conversation

dhartunian commented Sep 2, 2025

Uh oh!

cockroach-teamcity commented Sep 2, 2025

Uh oh!

kyle-a-wong left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

kyle-a-wong left a comment

Choose a reason for hiding this comment

Uh oh!

kyle-a-wong left a comment

Choose a reason for hiding this comment

Uh oh!

kyle-a-wong left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

mw5h left a comment

Choose a reason for hiding this comment

Uh oh!

mw5h left a comment

Choose a reason for hiding this comment

Uh oh!

kyle-a-wong left a comment

Choose a reason for hiding this comment

Uh oh!

blathers-crl bot commented Sep 5, 2025

Uh oh!

dhartunian commented Sep 7, 2025

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

kyle-a-wong left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian commented Sep 8, 2025

Uh oh!

dhartunian commented Sep 8, 2025

Uh oh!

dhartunian commented Sep 8, 2025

Uh oh!

craig bot commented Sep 8, 2025

Uh oh!

Uh oh!

Uh oh!