schemachanger: fix discards for explicit txns

Previously, discarding a zone config on an object in an explicit transaction would only discard the latest (categorized by highest seq num) element associated with it. This would mean for the following scenario: ``` BEGIN; ALTER TABLE t CONFIGURE ZONE USING num_replicas = 11; ALTER TABLE t CONFIGURE ZONE USING num_replicas = 12; ALTER TABLE t CONFIGURE ZONE DISCARD; COMMIT; ``` The discard would only discard the element with a replication factor of 12, leaving us with a table with a rep. factor of 11. This patch ensures we clean up all references of an element when discarding. Epic: none Release note: None
cockroachdb · Nov 20, 2024 · 739f490 · 739f490
1 parent d78b1db
commit 739f490
Show file tree

Hide file tree

Showing 26 changed files with 391 additions and 49 deletions.
diff --git a/pkg/ccl/schemachangerccl/backup_base_generated_test.go b/pkg/ccl/schemachangerccl/backup_base_generated_test.go
diff --git a/pkg/sql/logictest/testdata/logic_test/event_log b/pkg/sql/logictest/testdata/logic_test/event_log
@@ -548,7 +548,6 @@ ORDER BY "timestamp", info
 1  {"EventType": "set_zone_config", "Options": ["range_max_bytes = 67108865", "range_min_bytes = 16777216"], "ResolvedOldConfig": "range_min_bytes:134217728 range_max_bytes:536870912 gc:<ttl_seconds:14400 > num_replicas:3 inherited_constraints:false null_voter_constraints_is_empty:true inherited_lease_preferences:false ", "Tag": "CONFIGURE ZONE", "Target": "TABLE test.public.a", "User": "root"}
 1  {"EventType": "set_zone_config", "Options": ["\"gc.ttlseconds\" = 13000"], "ResolvedOldConfig": "range_min_bytes:134217728 range_max_bytes:536870912 gc:<ttl_seconds:14400 > num_replicas:3 inherited_constraints:false null_voter_constraints_is_empty:true inherited_lease_preferences:false ", "Tag": "CONFIGURE ZONE", "Target": "INDEX test.public.a@a_pkey", "User": "root"}
 1  {"EventType": "set_zone_config", "Options": ["\"gc.ttlseconds\" = 15000"], "ResolvedOldConfig": "range_min_bytes:134217728 range_max_bytes:536870912 gc:<ttl_seconds:14400 > num_replicas:3 inherited_constraints:false null_voter_constraints_is_empty:true inherited_lease_preferences:false ", "Tag": "CONFIGURE ZONE", "Target": "INDEX test.public.a@bar", "User": "root"}
-1  {"EventType": "set_zone_config", "Options": ["\"gc.ttlseconds\" = 15000"], "ResolvedOldConfig": "range_min_bytes:134217728 range_max_bytes:536870912 gc:<ttl_seconds:14400 > num_replicas:3 inherited_constraints:false null_voter_constraints_is_empty:true inherited_lease_preferences:false ", "Tag": "CONFIGURE ZONE", "Target": "INDEX test.public.a@bar", "User": "root"}
 
 skipif config 3node-tenant-default-configs
 query IT

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
@@ -94,14 +94,23 @@ func SetZoneConfig(b BuildCtx, n *tree.SetZoneConfig) {
 		if zco.isNoOp() {
 			return
 		}
-		toDrop, affectedSubzoneConfigsToUpdate := zco.getZoneConfigElemForDrop(b)
-		dropZoneConfigElem(b, toDrop, eventDetails)
-		addZoneConfigElem(b, affectedSubzoneConfigsToUpdate, oldZone, eventDetails)
+
+		toDropList, affectedSubzoneConfigsToUpdate := zco.getZoneConfigElemForDrop(b)
+		for i, e := range toDropList {
+			// Log the latest dropping element.
+			dropZoneConfigElem(b, e, eventDetails, i == len(toDropList)-1 /* isLoggingNeeded */)
+		}
+		for _, e := range affectedSubzoneConfigsToUpdate {
+			// No need to log the side effects.
+			addZoneConfigElem(b, e, oldZone, eventDetails, false /* isLoggingNeeded */)
+		}
 	} else {
 		toAdd, affectedSubzoneConfigsToUpdate := zco.getZoneConfigElemForAdd(b)
-		affectedSubzoneConfigsToUpdate = append([]scpb.Element{toAdd},
-			affectedSubzoneConfigsToUpdate...)
-		addZoneConfigElem(b, affectedSubzoneConfigsToUpdate, oldZone, eventDetails)
+		addZoneConfigElem(b, toAdd, oldZone, eventDetails, true)
+		for _, e := range affectedSubzoneConfigsToUpdate {
+			// No need to log the side effects.
+			addZoneConfigElem(b, e, oldZone, eventDetails, false /* isLoggingNeeded */)
+		}
 	}
 }
 
@@ -203,23 +212,26 @@ func astToZoneConfigObject(b BuildCtx, n *tree.SetZoneConfig) (zoneConfigObject,
 }
 
 func dropZoneConfigElem(
-	b BuildCtx, elem scpb.Element, eventDetails eventpb.CommonZoneConfigDetails,
+	b BuildCtx, elem scpb.Element, eventDetails eventpb.CommonZoneConfigDetails, isLoggingNeeded bool,
 ) {
-	info := &eventpb.RemoveZoneConfig{CommonZoneConfigDetails: eventDetails}
 	b.Drop(elem)
-	b.LogEventForExistingPayload(elem, info)
+	if isLoggingNeeded {
+		info := &eventpb.RemoveZoneConfig{CommonZoneConfigDetails: eventDetails}
+		b.LogEventForExistingPayload(elem, info)
+	}
 }
 
 func addZoneConfigElem(
 	b BuildCtx,
-	elems []scpb.Element,
+	elem scpb.Element,
 	oldZone *zonepb.ZoneConfig,
 	eventDetails eventpb.CommonZoneConfigDetails,
+	isLoggingNeeded bool,
 ) {
-	info := &eventpb.SetZoneConfig{CommonZoneConfigDetails: eventDetails,
-		ResolvedOldConfig: oldZone.String()}
-	for _, e := range elems {
-		b.Add(e)
-		b.LogEventForExistingPayload(e, info)
+	b.Add(elem)
+	if isLoggingNeeded {
+		info := &eventpb.SetZoneConfig{CommonZoneConfigDetails: eventDetails,
+			ResolvedOldConfig: oldZone.String()}
+		b.LogEventForExistingPayload(elem, info)
 	}
 }
diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/database_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/database_zone_config.go
@@ -43,14 +43,23 @@ func (dzo *databaseZoneConfigObj) getZoneConfigElemForAdd(
 }
 
 func (dzo *databaseZoneConfigObj) getZoneConfigElemForDrop(
-	_ BuildCtx,
-) (scpb.Element, []scpb.Element) {
-	elem := &scpb.DatabaseZoneConfig{
-		DatabaseID: dzo.databaseID,
-		ZoneConfig: dzo.zoneConfig,
-		SeqNum:     dzo.seqNum,
+	b BuildCtx,
+) ([]scpb.Element, []scpb.Element) {
+	var elems []scpb.Element
+	if dzo.seqNum > 0 {
+		for i := range dzo.seqNum {
+			elems = append(elems, &scpb.DatabaseZoneConfig{
+				DatabaseID: dzo.databaseID,
+				SeqNum:     i + 1,
+			})
+		}
+	} else {
+		elems = append(elems, &scpb.DatabaseZoneConfig{
+			DatabaseID: dzo.databaseID,
+			SeqNum:     0,
+		})
 	}
-	return elem, nil
+	return elems, nil
 }
 
 func (dzo *databaseZoneConfigObj) checkPrivilegeForSetZoneConfig(

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/index_zone_config.go
@@ -80,10 +80,12 @@ func (izo *indexZoneConfigObj) getZoneConfigElemForAdd(b BuildCtx) (scpb.Element
 	return szCfg, szCfgsToUpdate
 }
 
-func (izo *indexZoneConfigObj) getZoneConfigElemForDrop(b BuildCtx) (scpb.Element, []scpb.Element) {
+func (izo *indexZoneConfigObj) getZoneConfigElemForDrop(
+	b BuildCtx,
+) ([]scpb.Element, []scpb.Element) {
 	// TODO(annie): this will need to be revised in order to implement subzone
 	// discards.
-	return izo.getZoneConfigElemForAdd(b)
+	return nil, nil
 }
 
 func (izo *indexZoneConfigObj) retrievePartialZoneConfig(b BuildCtx) *zonepb.ZoneConfig {

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/named_range_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/named_range_zone_config.go
@@ -43,13 +43,22 @@ func (rzo *namedRangeZoneConfigObj) getZoneConfigElemForAdd(
 
 func (rzo *namedRangeZoneConfigObj) getZoneConfigElemForDrop(
 	_ BuildCtx,
-) (scpb.Element, []scpb.Element) {
-	elem := &scpb.NamedRangeZoneConfig{
-		RangeID:    rzo.rangeID,
-		ZoneConfig: rzo.zoneConfig,
-		SeqNum:     rzo.seqNum,
+) ([]scpb.Element, []scpb.Element) {
+	var elems []scpb.Element
+	if rzo.seqNum > 0 {
+		for i := range rzo.seqNum {
+			elems = append(elems, &scpb.NamedRangeZoneConfig{
+				RangeID: rzo.rangeID,
+				SeqNum:  i + 1,
+			})
+		}
+	} else {
+		elems = append(elems, &scpb.NamedRangeZoneConfig{
+			RangeID: rzo.rangeID,
+			SeqNum:  0,
+		})
 	}
-	return elem, nil
+	return elems, nil
 }
 
 func (rzo *namedRangeZoneConfigObj) checkPrivilegeForSetZoneConfig(

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go
@@ -91,10 +91,10 @@ func (pzo *partitionZoneConfigObj) getZoneConfigElemForAdd(
 
 func (pzo *partitionZoneConfigObj) getZoneConfigElemForDrop(
 	b BuildCtx,
-) (scpb.Element, []scpb.Element) {
+) ([]scpb.Element, []scpb.Element) {
 	// TODO(annie): this will need to be revised in order to implement subzone
 	// discards. This is fine for now as we fallback before we can get here.
-	return pzo.getZoneConfigElemForAdd(b)
+	return nil, nil
 }
 
 func (pzo *partitionZoneConfigObj) retrievePartialZoneConfig(b BuildCtx) *zonepb.ZoneConfig {

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/table_zone_config.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/table_zone_config.go
@@ -41,13 +41,24 @@ func (tzo *tableZoneConfigObj) getZoneConfigElemForAdd(_ BuildCtx) (scpb.Element
 	return elem, nil
 }
 
-func (tzo *tableZoneConfigObj) getZoneConfigElemForDrop(_ BuildCtx) (scpb.Element, []scpb.Element) {
-	elem := &scpb.TableZoneConfig{
-		TableID:    tzo.tableID,
-		ZoneConfig: tzo.zoneConfig,
-		SeqNum:     tzo.seqNum,
+func (tzo *tableZoneConfigObj) getZoneConfigElemForDrop(
+	_ BuildCtx,
+) ([]scpb.Element, []scpb.Element) {
+	var elems []scpb.Element
+	if tzo.seqNum > 0 {
+		for i := range tzo.seqNum {
+			elems = append(elems, &scpb.TableZoneConfig{
+				TableID: tzo.tableID,
+				SeqNum:  i + 1,
+			})
+		}
+	} else {
+		elems = append(elems, &scpb.TableZoneConfig{
+			TableID: tzo.tableID,
+			SeqNum:  0,
+		})
 	}
-	return elem, nil
+	return elems, nil
 }
 
 func (tzo *tableZoneConfigObj) checkPrivilegeForSetZoneConfig(

diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/zone_config_helpers.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/zone_config_helpers.go
@@ -59,12 +59,14 @@ type zoneConfigObjBuilder interface {
 	// elements around.
 	getZoneConfigElemForAdd(b BuildCtx) (scpb.Element, []scpb.Element)
 
-	// getZoneConfigElemForDrop retrieves (scpb.Element, []scpb.Element) needed
-	// for dropping the zone config object. The slice of multiple elements
+	// getZoneConfigElemForDrop retrieves ([]scpb.Element, []scpb.Element) needed
+	// for dropping the zone config object. The second slice of multiple elements
 	// to be modified becomes more relevant for subzone configs -- as configuring
 	// the zone on indexes and partitions can shift the subzone spans for other
-	// elements around.
-	getZoneConfigElemForDrop(b BuildCtx) (scpb.Element, []scpb.Element)
+	// elements around. The first slice is used to ensure all references with
+	// varying `seqNum`s for the element are dropped (relevant only in explicit
+	// transactions).
+	getZoneConfigElemForDrop(b BuildCtx) ([]scpb.Element, []scpb.Element)
 
 	// getTargetID returns the target ID of the zone config object. This is either
 	// a database or a table ID.

diff --git a/pkg/sql/schemachanger/sctest_generated_test.go b/pkg/sql/schemachanger/sctest_generated_test.go
diff --git a/...nd/alter_database_configure_zone_discard/alter_database_configure_zone_discard.definition b/...nd/alter_database_configure_zone_discard/alter_database_configure_zone_discard.definition
@@ -4,5 +4,6 @@ CREATE DATABASE db;
 
 test
 ALTER DATABASE db CONFIGURE ZONE USING num_replicas = 7, gc.ttlseconds = 10000;
+ALTER DATABASE db CONFIGURE ZONE USING num_replicas = 8;
 ALTER DATABASE db CONFIGURE ZONE DISCARD;
 ----
diff --git a/.../alter_database_configure_zone_discard/alter_database_configure_zone_discard.side_effects b/.../alter_database_configure_zone_discard/alter_database_configure_zone_discard.side_effects
@@ -7,6 +7,7 @@ CREATE DATABASE db;
 
 /* test */
 ALTER DATABASE db CONFIGURE ZONE USING num_replicas = 7, gc.ttlseconds = 10000;
+ALTER DATABASE db CONFIGURE ZONE USING num_replicas = 8;
 ALTER DATABASE db CONFIGURE ZONE DISCARD;
 ----
 begin transaction #1
@@ -27,6 +28,20 @@ write *eventpb.SetZoneConfig to event log:
 ## StatementPhase stage 1 of 1 with 1 MutationType op
 upsert zone config for #104
 checking for feature: CONFIGURE ZONE
+write *eventpb.SetZoneConfig to event log:
+  config:
+    options:
+    - num_replicas = 8
+    target: DATABASE db
+  resolvedOldConfig: 'gc:<ttl_seconds:10000 > num_replicas:7 inherited_constraints:true null_voter_constraints_is_empty:false inherited_lease_preferences:true '
+  sql:
+    descriptorId: 104
+    statement: ALTER DATABASE ‹db› CONFIGURE ZONE USING ‹num_replicas› = ‹8›
+    tag: CONFIGURE ZONE
+    user: root
+## StatementPhase stage 1 of 1 with 1 MutationType op
+upsert zone config for #104
+checking for feature: CONFIGURE ZONE
 write *eventpb.RemoveZoneConfig to event log:
   config:
     target: DATABASE db
@@ -35,8 +50,9 @@ write *eventpb.RemoveZoneConfig to event log:
     statement: ALTER DATABASE ‹db› CONFIGURE ZONE DISCARD
     tag: CONFIGURE ZONE
     user: root
-## StatementPhase stage 1 of 1 with 1 MutationType op
+## StatementPhase stage 1 of 1 with 2 MutationType ops
 deleting zone config for #104
+upsert zone config for #104
 # end StatementPhase
 # begin PreCommitPhase
 ## PreCommitPhase stage 1 of 1 with 1 MutationType op

diff --git a/...re_zone_discard__statement_1_of_2.explain → ...re_zone_discard__statement_1_of_3.explain b/...re_zone_discard__statement_1_of_2.explain → ...re_zone_discard__statement_1_of_3.explain
diff --git a/...e_discard__statement_1_of_2.explain_shape → ...e_discard__statement_1_of_3.explain_shape b/...e_discard__statement_1_of_2.explain_shape → ...e_discard__statement_1_of_3.explain_shape