Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: fix gist decoding in foreign DB #110966

Merged
merged 1 commit into from
Sep 22, 2023
Merged

Conversation

cucaroach
Copy link
Contributor

@cucaroach cucaroach commented Sep 20, 2023

When a gist is exported and decoded in a foreign DB it should still
decode w/o panic. If a table scan result columns are fed into a
group by column the group by column code expects the result columns to
be in tact and panics. Fix this by making fake "unknown" result columns
so we can print the plan.

Making this change led to an unintended change in gists_tpce, this is
because the catalog was shared between the test files and I added a
table. Fix this for the future by creating a new catalog for each file.
But with this change the table ids encoded in the gists_tpce got
smaller.

Epic: None
Fixes: #110964
Release note (bug fix): Fixed panic when decoding gist in DB without the
table referred to by the gist.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

When a gist is exported and decoded in a foreign DB it should still
decode w/o panic. If a table scan result columns are fed into a
group by column the group by column code expects the result columns to
be in tact and panics. Fix this by making fake "unknown" result columns
so we can print the plan.

Making this change led to an unintended change in gists_tpce, this is
because the catalog was shared between the test files and I added a
table.  Fix this for the future by creating a new catalog for each file.
But with this change the table ids encoded in the gists_tpce got
smaller.

Epic: None
Fixes: cockroachdb#110964
Release note (bug fix): Fixed panic when decoding gist in DB without the
table referred to by the gist.
@cucaroach cucaroach marked this pull request as ready for review September 21, 2023 14:44
@cucaroach cucaroach requested a review from a team as a code owner September 21, 2023 14:44
@cucaroach cucaroach requested review from michae2 and a team and removed request for a team September 21, 2023 14:44
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Nice fix! Thanks for explaining the gists_tpce change.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

@cucaroach
Copy link
Contributor Author

bors r+
TFTR!

@yuzefovich
Copy link
Member

bors timed out, courtesy merge

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 22, 2023

This PR was included in a batch that timed out, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Sep 22, 2023

Build succeeded:

@craig craig bot merged commit 989af00 into cockroachdb:master Sep 22, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql/gists: unknownTable leads to panics and fails to decode
4 participants