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

*: remove the storage.mvcc.range_tombstones.enabled cluster setting #96966

Conversation

nicktrav
Copy link
Collaborator

Range tombstones were gated behind a cluster setting in 22.2. They are currently enabled by default on the master branch with the intention of being enabled by default in the 23.1 release.

Remove the cluster setting entirely.

Closes #91147.

Release note: None.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nicktrav nicktrav force-pushed the nickt.range-tombstones-cluster-setting branch 5 times, most recently from 696ebd7 to 5003d14 Compare February 11, 2023 00:43
@nicktrav nicktrav marked this pull request as ready for review February 13, 2023 15:28
@nicktrav nicktrav requested a review from a team February 13, 2023 15:28
@nicktrav nicktrav requested review from a team as code owners February 13, 2023 15:28
@nicktrav nicktrav requested review from a team, smg260, renatolabs and miretskiy and removed request for a team February 13, 2023 15:28
@nicktrav
Copy link
Collaborator Author

Opening this up for a first round of review. This removes the cluster setting and the helper functions. I lacked context on some of the changes in higher layers (DR / SQL), so I did my best there.

There's at least one test that is failing pkg/ccl/backupccl.TestDataDriven/in-progress-import-rollback, so would appreciate some eyes there.

@erikgrinaker - I also didn't touch the proto API here. It wasn't clear to me if we needed to keep the range tombstone fields around, or if they could be retired / moved to reserved. If we do need to remove, I can tackle that separately (the change looked much more nuanced).

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @jbowens, @miretskiy, @msbutler, @nicktrav, @renatolabs, and @smg260)


pkg/storage/mvcc.go line 88 at r1 (raw file):

var MVCCRangeTombstonesEnabled = settings.RegisterBoolSetting(
	settings.TenantReadOnly,
	"storage.mvcc.range_tombstones.enabled",

This should be added to settings.retiredSettings

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.

can you put the setting into the retiredSettings map of pkg/settings/registry.go?

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.

ah i see, Radu commented on that already

@rafiss rafiss requested review from rafiss and removed request for rafiss February 13, 2023 15:58
Range tombstones were gated behind a cluster setting in 22.2. They are
currently enabled by default on the master branch with the intention of
being enabled by default in the 23.1 release.

Remove the cluster setting entirely.

Closes cockroachdb#91147.

Release note: None.
@nicktrav nicktrav force-pushed the nickt.range-tombstones-cluster-setting branch from 5003d14 to e86668f Compare February 13, 2023 17:18
Copy link
Collaborator Author

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @jbowens, @miretskiy, @msbutler, @rafiss, @renatolabs, and @smg260)


pkg/storage/mvcc.go line 88 at r1 (raw file):

Previously, RaduBerinde wrote…

This should be added to settings.retiredSettings

Done. Thanks!

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

As I mentioned over on the issue, this change will mean that 23.1 nodes in a mixed 22.2/23.1 cluster will unconditionally send requests using MVCC range tombstones. I think that's fine, since we do expect them to work properly in 22.2 as well, but I suppose there's always a risk that we discover some bad bug in early 22.2 releases and users could hit this when upgrading to 23.1. The alternative would be to continue respecting the setting until the 23.1 upgrade is finalized, but that seems like overkill to me.

I also didn't touch the proto API here. It wasn't clear to me if we needed to keep the range tombstone fields around, or if they could be retired / moved to reserved. If we do need to remove, I can tackle that separately (the change looked much more nuanced).

We'll need to keep DeleteRangeRequest.UseRangeTombstone around -- the non-range-tombstone variant is often used by SQL for range deletions, and we can't use range tombstones here yet because they're not transactional. And in any case, we'd have to set it for compatibility with 22.2 nodes.

Reviewed 25 of 25 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @miretskiy, @msbutler, @nicktrav, @rafiss, @renatolabs, and @smg260)


pkg/sql/repair.go line 743 at r2 (raw file):

	}
	b := &kv.Batch{}
	if p.execCfg.Settings.Version.IsActive(ctx, clusterversion.TODODelete_V22_2UseDelRangeInGCJob) {

Should we get rid of clusterversion.TODODelete_V22_2UseDelRangeInGCJob while we're at it?


pkg/sql/gcjob/gc_job.go line 502 at r2 (raw file):

	ctx context.Context, details *jobspb.SchemaChangeGCDetails, s *cluster.Settings,
) bool {
	// TODO(ajwerner): Adopt the DeleteRange protocol for tenant GC.

@ajwerner @stevendanna Do you know the implications of this for C2C replication and unified architecture? Is it possible to replicate entire clusters with C2C, including all tenants, such that this tenant GC (using ClearRange) won't get replicated and the tenant will live on in the target cluster? Or is replication always done on a per-tenant basis, such that destroying the tenant will halt the C2C replication, making this moot?

@nicktrav
Copy link
Collaborator Author

We decided to keep the cluster setting around for 23.1. Going to close this out for now and revisit next cycle.

@nicktrav nicktrav closed this Feb 27, 2023
@nicktrav nicktrav deleted the nickt.range-tombstones-cluster-setting branch February 27, 2023 22:33
@erikgrinaker
Copy link
Contributor

We decided to keep the cluster setting around for 23.1. Going to close this out for now and revisit next cycle.

Did you check this with DR and schema? I think they wanted to rely on the MVCC history being unconditionally complete, for stuff like tenant backups and C2C replication.

@erikgrinaker
Copy link
Contributor

Nevermind, just saw #91147 (comment).

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.

storage: add unconditional range tombstone cluster version
5 participants