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-23.2: spanconfigkvsubscriber: fix missing system span configs #114833

Merged
merged 1 commit into from Nov 22, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Nov 21, 2023

Backport 1/1 commits from #114050 on behalf of @adityamaru.

/cc @cockroachdb/release


Previously, if a range did not have an overlapping span config then ForEachOverlappingSpanConfig would not apply the relevant system span configs that may still apply to that range. One situation in which we could end up with such a range when a table is dropped and all of its data (including the range deletion tombstone installed by the drop) is GC'ed, the associated schema change GC job will delete the table's span config. In this case, we will not find any overlapping span configs for the table's span, but a system span config, such as a cluster wide protection policy, may still be applicable to the replica with the empty table span. A scan of that span AOST the timestamp at which we wrote the protection policy could result in a BatchTimestampBeforeGCError.

This change fixes the above bug by applying a fallback config to the span with no overlapping span configs, and combining the system span configs that apply to the span. The change adds a red-green test to exercise this logic.

Informs: #113867
Release note (bug fix): an empty range corresponding to a drop table did not respect system level span configurations such as protected timestamps, potentially causing reads above the protected timestamp to fail


Release justification: bug fix for a case where an empty span would not respect a protected timestamp record

@blathers-crl blathers-crl bot requested review from a team as code owners November 21, 2023 19:11
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.2-114050 branch from b8ce6da to a5970d0 Compare November 21, 2023 19:11
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Nov 21, 2023
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.2-114050 branch from b518a3b to 38541f6 Compare November 21, 2023 19:11
Copy link
Author

blathers-crl bot commented Nov 21, 2023

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 and one additional
    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 21, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@arulajmani arulajmani 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 8 of 8 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @blathers-crl[bot])


pkg/spanconfig/spanconfig.go line 235 at r1 (raw file):

	// If there are no overlapping configs for the supplied span, the supplied
	// callback is invoked on the fallback config combined with any applicable
	// system span configs

nit: missing period.

Previously, if a range did not have an overlapping span
config then `ForEachOverlappingSpanConfig` would not apply
the relevant system span configs that may still apply to
that range. One situation in which we could end up with
such a range when a table is dropped and all of its data (including the
range deletion tombstone installed by the drop) is GC'ed, the associated
schema change GC job will delete the table's span config. In this case, we
will not find any overlapping span configs for the table's span, but a
system span config, such as a cluster wide protection policy, may still be
applicable to the replica with the empty table span. A scan
of that span AOST the timestamp at which we wrote the protection
policy could result in a BatchTimestampBeforeGCError.

This change fixes the above bug by applying a fallback config
to the span with no overlapping span configs, and combining the
system span configs that apply to the span. The change adds a
red-green test to exercise this logic.

Informs: #113867
Release note (bug fix): an empty range corresponding to a drop table
did not respect system level span configurations such as protected
timestamps, potentially causing reads above the protected timestamp
to fail
@adityamaru adityamaru force-pushed the blathers/backport-release-23.2-114050 branch from 38541f6 to 6aa2c86 Compare November 22, 2023 14:24
@adityamaru adityamaru merged commit 9cd98d9 into release-23.2 Nov 22, 2023
5 of 6 checks passed
@adityamaru adityamaru deleted the blathers/backport-release-23.2-114050 branch November 22, 2023 15:02
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 blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants