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

release-24.2: schemachanger: add PartitionName to IndexZoneConfig attr #134472

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

annrpom
Copy link
Contributor

@annrpom annrpom commented Nov 6, 2024

This patch adds a PartitionName attribute to the IndexZoneConfig
element to uniquely identify partitions from one another. Prior to this,
hitting a certain DROP path on a table with more than 1 partition on an
index would panic with an undropped backref error due to our inability
to distinguish these partitions from eachother.

Our code to drop these partition elements would always refer to the
top-most partition element in the descsCache's elementIndexMap
-- as the key to identify partition1 from partition2 was the same. So,
partition1 would be marked toAbsent "twice"; while partition2 got
ignored </3.

Another thing to note is that this bug is not just met for partitioned
tables of the nature mentioned above -- as the builderState's descriptor
cache would need to be unchanged between each partition's drop.

Epic: none
Fixes: #131862

Release note (bug fix): Addressed a bug with DROP CASCADE that would
occasionally panic with an undropped backref message on partitioned
tables.


Release justification: low-risk bug fix

Copy link

blathers-crl bot commented Nov 6, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Nov 6, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This patch adds a `PartitionName` attribute to the `IndexZoneConfig`
element to uniquely identify partitions from one another. Prior to this,
hitting a certain DROP path on a table with more than 1 partition on an
index would panic with an undropped backref error due to our inability
to distinguish these partitions from eachother.

Our code to drop these partition elements would always refer to the
top-most partition element in the descsCache's `elementIndexMap`
-- as the key to identify partition1 from partition2 was the same. So,
partition1 would be marked `toAbsent` "twice"; while partition2 got
ignored </3.

Another thing to note is that this bug is not just met for partitioned
tables of the nature mentioned above -- as the `builderState`'s descriptor
cache would need to be unchanged between each partition's drop.

Epic: none
Fixes: cockroachdb#131862

Release note (bug fix): Addressed a bug with DROP CASCADE that would
occasionally panic with an undropped backref message on partitioned
tables.
@annrpom annrpom marked this pull request as ready for review November 7, 2024 15:12
@annrpom annrpom requested a review from a team as a code owner November 7, 2024 15:12
@annrpom annrpom requested a review from rafiss November 7, 2024 15:12
@annrpom annrpom added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. labels Nov 7, 2024
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@annrpom annrpom added backport-23.1.x Flags PRs that need to be backported to 23.1 and removed backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. labels Nov 7, 2024
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

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

@annrpom annrpom merged commit 52a2db4 into cockroachdb:release-24.2 Nov 7, 2024
19 of 20 checks passed
craig bot pushed a commit that referenced this pull request Nov 21, 2024
135933: scpb: ensure IndexZoneConfig fields do not overlap r=annrpom a=annrpom

In [#134522](#134472), we added a fix to ensure that we cleaned up backrefs properly for our `IndexZoneConfig` element, but in doing so, we created an overlap of field ID `3` between v24.2+. This patch ensures that we do not re-use the same field ID for a different field in our IndexZoneConfig element by marking it as `reserved`.

Fixes: #133003
Fixes: #135807

Release note: None

Co-authored-by: Annie Pompa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants