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/schemachanger: ALTER PRIMARY KEY .. USING HASH can make the hash column public too late #128457

Closed
fqazi opened this issue Aug 7, 2024 · 2 comments · Fixed by #129828
Closed
Labels
branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-1 Issues/test failures with a fix SLA of 1 month T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@fqazi
Copy link
Collaborator

fqazi commented Aug 7, 2024

Previously, in the legacy schema changer, when executing ALTER TABLE PRIMARY KEY .. USING HASH... we have stages where a new primary index is public without all the columns inside it being public. This is unexpected, since the key columns / storing referenced in the primary key should be public at the same time as the index. This can cause issues for CDC: https://github.com/wenyihu6/cockroach/blob/2d4f2a14a05b8e23a134cad42b435886657f8172/pkg/ccl/changefeedccl/cdcevent/event.go#L312

We should ideally make sure the columns referenced by the primary key index are public at the same time. Or at the very least ensure this is guaranteed for key columns.

Jira issue: CRDB-41028

Epic CRDB-40419

@fqazi fqazi added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs branch-master Failures and bugs on the master branch. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 labels Aug 7, 2024
@fqazi fqazi self-assigned this Aug 7, 2024
@rafiss rafiss added the P-1 Issues/test failures with a fix SLA of 1 month label Aug 12, 2024
@exalate-issue-sync exalate-issue-sync bot added P-1 Issues/test failures with a fix SLA of 1 month and removed P-1 Issues/test failures with a fix SLA of 1 month labels Aug 12, 2024
@fqazi
Copy link
Collaborator Author

fqazi commented Aug 27, 2024

Digging into this more, I don't think there is any nice options here. I think it makes sense to have the column not be public but still be in the primary index, otherwise we can't support comma syntax properly (i.e. rollbacks would break). I'm going to make a patch for the CDC logic to tolerate this and add coverage.

@rafiss
Copy link
Collaborator

rafiss commented Aug 28, 2024

While working on this, can you also see if you can identify why #128420 is happening?

craig bot pushed a commit that referenced this issue Oct 8, 2024
129828: changefeed: support primary key columns which are not visible r=fqazi a=fqazi

Previously, when schema changes were happening with an active change feed, it was possible for ALTER PRIMARY KEY USING HASH to have an intermediate state where the hash column could exist within the primary key without being public. This is expected since complex schema changes could not roll back if these columns were made completely public at the same time as the primary key. This would break assumptions within CDC, which expected all primary key columns to be public. To address this, this patch modifies this logic to ignore columns that are writable but not yet fully readable in the primary key.

Fixes: #128457

Release note: None

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
@craig craig bot closed this as completed in 75f07dc Oct 8, 2024
kvoli pushed a commit to kvoli/cockroach that referenced this issue Oct 8, 2024
Previously, when schema changes were happening with an active change
feed, it was possible for ALTER PRIMARY KEY USING HASH to have an
intermediate state where the hash column could exist within the primary
key without being public. This is expected since complex schema changes
could not roll back if these columns were made completely public at the
same time as the primary key. This would break assumptions within CDC,
which expected all primary key columns to be public. To address this,
this patch modifies this logic to ignore columns that are writable but
not yet fully readable in the primary key.

Fixes: cockroachdb#128457

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-24.1 Used to mark GA and release blockers, technical advisories, and bugs for 24.1 branch-release-24.2 Used to mark GA and release blockers, technical advisories, and bugs for 24.2 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-support Would prevent or help troubleshoot a customer escalation - bugs, missing observability/tooling, docs P-1 Issues/test failures with a fix SLA of 1 month T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
Status: Triage
Development

Successfully merging a pull request may close this issue.

2 participants