sql: add descriptors.json and schema_changes.txt to stmtbundle#170622
Conversation
|
😎 Merged successfully - details. |
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I have added a few people who may be able to assist in reviewing: 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Quick ping for review whenever you get a chance, happy to address any feedback. |
When investigating optimizer surprises, the bundle reviewer often needs to rule out schema-state interactions. Two new files help: descriptors.json with the pretty-printed JSON for each descriptor used by the statement, and schema_changes.txt with the recent schema-change job history for those descriptors. The declarative schema changer drains payload.DescriptorIDs as targets complete, so completed declarative jobs have an empty descriptorIds field. PrintSchemaChanges therefore also matches the descriptor's fully-qualified name against the job's description text. Fixes: cockroachdb#170491 Epic: None Release note (general change): Statement bundles produced by EXPLAIN ANALYZE (DEBUG) now include descriptors.json (pretty-printed descriptor JSON for each object used by the statement) and schema_changes.txt (recent schema-change history for those descriptors).
3221ac5 to
1c5a284
Compare
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
Pushed a follow-up. The unit_tests failure was TestDistributedQueryErrorIsRetriedLocally (pkg/sql/distsql_running_test.go), which asserts on the bundle's file list. I missed updating that hardcoded list to include the two new files this PR adds (descriptors.json and schema_changes.txt). Verified the fix locally with ./dev test pkg/sql -f TestDistributedQueryErrorIsRetriedLocally now. Should be good. |
michae2
left a comment
There was a problem hiding this comment.
Nice work, @virajchogle!
/trunk merge
@michae2 reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on virajchogle).
|
Detected infrastructure failure on trunk-merge branch (matched: self-hosted runner lost communication with the server). Automatically resubmitting to merge queue (attempt 1 of 2). (run link) |
|
/trunk merge |
|
Thanks for the review, @michae2! Glad to have my first CRDB PR land. More incoming! |
When investigating optimizer surprises, the bundle reviewer often needs to rule out schema-state interactions. Two new files help: descriptors.json with the pretty-printed JSON for each descriptor used by the statement, and schema_changes.txt with the recent schema-change job history for those descriptors.
The declarative schema changer drains payload.DescriptorIDs as targets complete, so completed declarative jobs have an empty descriptorIds field. PrintSchemaChanges therefore also matches the descriptor's fully-qualified name against the job's description text.
Verified manually on a single-node demo cluster:
Fixes: #170491
Epic: None
Release note (general change): Statement bundles produced by EXPLAIN ANALYZE (DEBUG) now include descriptors.json (pretty-printed descriptor JSON for each object used by the statement) and schema_changes.txt (recent schema-change history for those descriptors).
@michae2