Repository navigation
Conversation
When two scans of the same BSUP file projected different columns of a named record type, their goroutines raced to bind the type's name. The loser panicked with "type redefined", or, if both got past the check in LookupTypeNamed, was handed the winner's type, so values could come out with the wrong fields. The loader now just calls LookupTypeNamed and returns the inner vector if the name is bound to another type, and LookupTypeNamed compares the types when DeclareTypeNamed finds the name already declared. Fixes #7641 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7641
When a query scans the same BSUP file more than once and the scans project different columns of a named record type, the scans' goroutines race to bind the type's name to their own projected type. The loser panicked with
type "foo" redefined. If both got past the check inLookupTypeNamedtogether, the loser was handed the winner's type for its own vector, so materialized values could come out with the wrong fields (e.g.,error("no such field b")).With this PR, the loader just calls
LookupTypeNamedand returns the inner vector if the name is bound to another type, instead of checkingLookupByNamefirst.LookupTypeNamedno longer checks for an existing name before callingDeclareTypeNamed, which checks again; it compares the types whenDeclareTypeNamedfinds the name, asDecodeTypeValuedoes. The new test fails onmainevery time, and with only the loader change it fails in 8 of 20 plain runs (all 20 with-race).On my laptop, a two-branch
forkover a one-record file panicked in 13 of 500 runs onmainand in 0 of 1000 with the fix. The mgbench bench3 q3 query that first showed it gave identical results in 200 of 200 runs with the fix, and bench3 query times didn't change.The patch was drafted with Claude Code. I reviewed and tested it, and ran it through Claude Code's
/code-review high.