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.

Sign up for GitHub

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

Jump to bottom

storage: add unconditional range tombstone cluster version #91147

Closed
erikgrinaker opened this issue Nov 2, 2022 · 7 comments · Fixed by #97770
Closed

storage: add unconditional range tombstone cluster version #91147

erikgrinaker opened this issue Nov 2, 2022 · 7 comments · Fixed by #97770
Assignees
@nicktrav
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-storage Storage Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Nov 2, 2022 •
edited by exalate-issue-sync bot
Loading

Before 23.1, we should remove the storage.mvcc.range_tombstones.enabled setting, to ensure all clusters are always MVCC-compliant.

Jira issue: CRDB-21122

Epic CRDB-20465
@erikgrinaker erikgrinaker added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-storage Relating to our storage engine (Pebble) on-disk storage. T-storage Storage Team labels Nov 2, 2022
@erikgrinaker
Copy link
Contributor Author

This means that 23.1 nodes in mixed 22.2 clusters will always write MVCC range tombstones, even when the corresponding 22.2 nodes have the setting off. I think this is probably OK? That also means that we don't need to keep the SQL schema logic around to write MVCC range tombstones in 23.1, even for mixed-version clusters.

cc @cockroachdb/sql-schema

@nicktrav nicktrav self-assigned this Feb 10, 2023
@nicktrav nicktrav mentioned this issue Feb 10, 2023
Merged
@nicktrav nicktrav mentioned this issue Feb 10, 2023
Closed
nicktrav added a commit to nicktrav/cockroach that referenced this issue Feb 11, 2023
@nicktrav
*: remove the storage.mvcc.range_tombstones.enabled cluster setting
5003d14 
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 added a commit to nicktrav/cockroach that referenced this issue Feb 13, 2023
@nicktrav
*: remove the storage.mvcc.range_tombstones.enabled cluster setting
e86668f 
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.
@jbowens
Copy link
Collaborator

jbowens commented Feb 23, 2023

This means that 23.1 nodes in mixed 22.2 clusters will always write MVCC range tombstones, even when the corresponding 22.2 nodes have the setting off. I think this is probably OK? That also means that we don't need to keep the SQL schema logic around to write MVCC range tombstones in 23.1, even for mixed-version clusters.

Given the prefix-synthesizing bug fixed in #97572, I think we should introduce a new cluster version gate that ensures a 23.1 node won't write MVCC range tombstones unless the full cluster has upgraded. Thoughts?

@erikgrinaker
Copy link
Contributor Author

Seems prudent. The cost would be that we'll need to keep the old schema/import code around for mixed-version clusters, but we can still guarantee that there won't be any non-MVCC operations after finalization (assuming we have a migration to flush out any old jobs or they transition to using range tombstones).

@erikgrinaker
Copy link
Contributor Author

cc @cockroachdb/disaster-recovery @cockroachdb/sql-schema

@ajwerner
Copy link
Contributor

Schema was not planning to remove the code. We are planning to just ratchet up the version gates and migrations written for 22.2 to 23.1

@erikgrinaker
Copy link
Contributor Author

Given the prefix-synthesizing bug fixed in #97572, I think we should introduce a new cluster version gate that ensures a 23.1 node won't write MVCC range tombstones unless the full cluster has upgraded. Thoughts?

I guess this means that we'll need to keep the setting around as well, but stop respecting it once 23.1 is finalized, so that an enabled setting still takes effect with an unfinalized 23.1 binary.

@jbowens jbowens changed the title storage: remove storage.mvcc.range_tombstones.enabled setting storage: add unconditional range tombstone cluster version Feb 27, 2023
@nicktrav
Copy link
Collaborator

@jbowens and I regrouped on this one today. Plan for 23.1 cycle:

  • Close out *: remove the storage.mvcc.range_tombstones.enabled cluster setting #96966 as obsolete.
  • Add an internal cluster version that we use as a feature gate.
  • Plumb feature gate into existing helper functions.
  • < cluster version - respect existing cluster setting.
  • >= cluster setting - unconditionally enable range tombstones. This would be the point of no return.
  • During 23.2 dev cycle, rip out the feature flag, helper functions and the cluster setting.

Let me know if I missed anything there.

nicktrav added a commit to nicktrav/cockroach that referenced this issue Feb 28, 2023
@nicktrav
clusterversion,storage: use range tombstones unconditionally in 23.1
7a6d0c1 
Currently, MVCC range tombstones are only enabled when the
`storage.mvcc.range_tombstones.enabled` cluster setting is `true`.
Originally, the plan for 23.1 was to remove this cluster setting
entirely, which would enable the feature unconditionally. However, it
was decided that for 23.1, the setting would remain, along with the
utility methods that determine whether or not the feature is enabled.

As there is still an expectation that the feature is enabled in 23.1,
add an internal cluster version that is used as a feature gate. If the
cluster is at least of version
`V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones
are _unconditionally_ enabled, irrespective of whether the cluster
setting is enabled or not.

Fix cockroachdb#91147.

Release note: None.
@nicktrav nicktrav mentioned this issue Feb 28, 2023
Merged
nicktrav added a commit to nicktrav/cockroach that referenced this issue Feb 28, 2023
@nicktrav
clusterversion,storage: use range tombstones unconditionally in 23.1
f733158 
Currently, MVCC range tombstones are only enabled when the
`storage.mvcc.range_tombstones.enabled` cluster setting is `true`.
Originally, the plan for 23.1 was to remove this cluster setting
entirely, which would enable the feature unconditionally. However, it
was decided that for 23.1, the setting would remain, along with the
utility methods that determine whether or not the feature is enabled.

As there is still an expectation that the feature is enabled in 23.1,
add an internal cluster version that is used as a feature gate. If the
cluster is at least of version
`V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones
are _unconditionally_ enabled, irrespective of whether the cluster
setting is enabled or not.

Fix cockroachdb#91147.

Release note: None.
nicktrav added a commit to nicktrav/cockroach that referenced this issue Feb 28, 2023
@nicktrav
clusterversion,storage: use range tombstones unconditionally in 23.1
4dbf890 
Currently, MVCC range tombstones are only enabled when the
`storage.mvcc.range_tombstones.enabled` cluster setting is `true`.
Originally, the plan for 23.1 was to remove this cluster setting
entirely, which would enable the feature unconditionally. However, it
was decided that for 23.1, the setting would remain, along with the
utility methods that determine whether or not the feature is enabled.

As there is still an expectation that the feature is enabled in 23.1,
add an internal cluster version that is used as a feature gate. If the
cluster is at least of version
`V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones
are _unconditionally_ enabled, irrespective of whether the cluster
setting is enabled or not.

Fix cockroachdb#91147.

Release note: None.
nicktrav added a commit to nicktrav/cockroach that referenced this issue Feb 28, 2023
@nicktrav
clusterversion,storage: use range tombstones unconditionally in 23.1
c73f143 
Currently, MVCC range tombstones are only enabled when the
`storage.mvcc.range_tombstones.enabled` cluster setting is `true`.
Originally, the plan for 23.1 was to remove this cluster setting
entirely, which would enable the feature unconditionally. However, it
was decided that for 23.1, the setting would remain, along with the
utility methods that determine whether or not the feature is enabled.

As there is still an expectation that the feature is enabled in 23.1,
add an internal cluster version that is used as a feature gate. If the
cluster is at least of version
`V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones
are _unconditionally_ enabled, irrespective of whether the cluster
setting is enabled or not.

Fix cockroachdb#91147.

Release note: None.
craig bot pushed a commit that referenced this issue Mar 1, 2023
@nicktrav
Merge #97770
21786aa 
97770: clusterversion,storage: use range tombstones unconditionally in 23.1 r=nicktrav a=nicktrav

Currently, MVCC range tombstones are only enabled when the `storage.mvcc.range_tombstones.enabled` cluster setting is `true`. Originally, the plan for 23.1 was to remove this cluster setting entirely, which would enable the feature unconditionally. However, it was decided that for 23.1, the setting would remain, along with the utility methods that determine whether or not the feature is enabled.

As there is still an expectation that the feature is enabled in 23.1, add an internal cluster version that is used as a feature gate. If the cluster is at least of version
`V23_1_MVCCRangeTombstonesUnconditionallyEnabled`, MVCC range tombstones are _unconditionally_ enabled, irrespective of whether the cluster setting is enabled or not.

Fix #91147.

Release note: None.

Epic: CRDB-20465.

Co-authored-by: Nick Travers <[email protected]>
@craig craig bot closed this as completed in 1c3e46d Mar 1, 2023
@nicktrav nicktrav mentioned this issue Mar 1, 2023
Closed
@jbowens jbowens moved this to Done in [Deprecated] Storage Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nicktrav nicktrav

Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-storage Storage Team
Projects
[Deprecated] Storage
Archived in project
Milestone
No milestone
4 participants
@erikgrinaker @nicktrav @jbowens @ajwerner