Merge #143892 #144336

143892: sql: add session variable to set schema_locked by default on new tables r=fqazi a=fqazi

This patch will add a new session variable / cluster setting for automatically setting schema_locked:

1. Adds a new session variable `create_table_with_schema_locked` and cluster setting `sql.defaults.create_table_with_schema_locked`, which will create all new tables with schema_locked by default
2. Updates the schema changer to make sure rollback stages are always non-revertible. Previously, this was implicitly done, and is required for having schema_locked in the entire declarative schema changer test suite.
3. Update the sql/schemachanger package to always create tables wit schema_locked. This requires not only updating expected files, but also executing setup statements with the declarative schema changer and adjusting some rollback points for DML injection

Release note (sql change): The create_table_with_schema_locked session variable can be used to control if newly created tables should have schema_locked set.
Informs: #129694
Fixes: #143891


144336: kvserver: improve stats handling in multiSSTWriter r=tbg a=tbg

See individual commits.

Epic: CRDB-46488

Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>

Author: 4 people

pkg/ccl/schemachangerccl/BUILD.bazel

@@ -34,10 +34,12 @@ go_test(
         "//pkg/base",
         "//pkg/ccl",
         "//pkg/ccl/multiregionccl/multiregionccltestutils",
+        "//pkg/clusterversion",
         "//pkg/jobs",
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/server",
+        "//pkg/settings/cluster",
         "//pkg/sql",
         "//pkg/sql/schemachanger/scexec",
         "//pkg/sql/schemachanger/sctest",

pkg/ccl/schemachangerccl/backup_base_generated_test.go

@@ -13,8 +13,10 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/ccl/multiregionccl/multiregionccltestutils"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/sctest"
@@ -29,8 +31,9 @@ import (
 // MultiRegionTestClusterFactory is a multi-region implementation of the
 // sctest.TestServerFactory interface.
 type MultiRegionTestClusterFactory struct {
-	scexec *scexec.TestingKnobs
-	server *server.TestingKnobs
+	scexec               *scexec.TestingKnobs
+	server               *server.TestingKnobs
+	schemaLockedDisabled bool
 }
 
 var _ sctest.TestServerFactory = MultiRegionTestClusterFactory{}
@@ -52,6 +55,12 @@ func (f MultiRegionTestClusterFactory) WithMixedVersion() sctest.TestServerFacto
 	return f
 }
 
+// WithSchemaLockDisabled implements the sctest.TestServerFactory interface.
+func (f MultiRegionTestClusterFactory) WithSchemaLockDisabled() sctest.TestServerFactory {
+	f.schemaLockedDisabled = true
+	return f
+}
+
 // Run implements the sctest.TestServerFactory interface.
 func (f MultiRegionTestClusterFactory) Run(
 	ctx context.Context, t *testing.T, fn func(_ serverutils.TestServerInterface, _ *gosql.DB),
@@ -69,7 +78,13 @@ func (f MultiRegionTestClusterFactory) Run(
 	if f.scexec != nil {
 		knobs.SQLDeclarativeSchemaChanger = f.scexec
 	}
-	c, db, _ := multiregionccltestutils.TestingCreateMultiRegionCluster(t, numServers, knobs)
+	// Always run this test with schema_locked by default.
+	st := cluster.MakeTestingClusterSettings()
+	if f.server != nil && f.server.ClusterVersionOverride.Major != 0 {
+		st = cluster.MakeClusterSettingsWithVersions(clusterversion.Latest.Version(), f.server.ClusterVersionOverride)
+	}
+	sql.CreateTableWithSchemaLocked.Override(ctx, &st.SV, !f.schemaLockedDisabled)
+	c, db, _ := multiregionccltestutils.TestingCreateMultiRegionCluster(t, numServers, knobs, multiregionccltestutils.WithSettings(st))
 	defer c.Stopper().Stop(ctx)
 	fn(c.Server(0), db)
 }

pkg/ccl/schemachangerccl/schemachanger_ccl_test.go

@@ -475,6 +475,9 @@ ElementState:
 - TableLocalityGlobal:
     tableId: 110
   Status: PUBLIC
+- TableSchemaLocked:
+    tableId: 110
+  Status: PUBLIC
 - TableZoneConfig:
     seqNum: 0
     tableId: 110
@@ -874,6 +877,9 @@ ElementState:
     regionName: us-east2
     tableId: 109
   Status: PUBLIC
+- TableSchemaLocked:
+    tableId: 109
+  Status: PUBLIC
 - TableZoneConfig:
     seqNum: 0
     tableId: 109
@@ -1571,6 +1577,9 @@ ElementState:
 - TablePartitioning:
     tableId: 108
   Status: PUBLIC
+- TableSchemaLocked:
+    tableId: 108
+  Status: PUBLIC
 - TableZoneConfig:
     seqNum: 0
     tableId: 108

pkg/ccl/schemachangerccl/testdata/decomp/multiregion

@@ -545,6 +545,9 @@ ElementState:
     databaseId: 100
     tableId: 104
   Status: PUBLIC
+- TableSchemaLocked:
+    tableId: 104
+  Status: PUBLIC
 - UserPrivileges:
     descriptorId: 104
     privileges: "2"
@@ -977,6 +980,9 @@ ElementState:
     databaseId: 100
     tableId: 105
   Status: PUBLIC
+- TableSchemaLocked:
+    tableId: 105
+  Status: PUBLIC
 - UserPrivileges:
     descriptorId: 105
     privileges: "2"

pkg/ccl/schemachangerccl/testdata/decomp/partitioning

@@ -30,7 +30,10 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
  │         │    ├── ABSENT → PUBLIC           IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 1 (k), IndexID: 3}
  │         │    ├── ABSENT → PUBLIC           IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 2 (v), IndexID: 3}
  │         │    └── ABSENT → PUBLIC           IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 4 (w+), IndexID: 3}
- │         └── 16 Mutation operations
+ │         ├── 1 element transitioning toward TRANSIENT_PUBLIC
+ │         │    └── PUBLIC → ABSENT           TableSchemaLocked:{DescID: 108 (table_regional_by_row)}
+ │         └── 17 Mutation operations
+ │              ├── SetTableSchemaLocked {"TableID":108}
  │              ├── MakeAbsentColumnDeleteOnly {"Column":{"ColumnID":4,"TableID":108}}
  │              ├── SetColumnName {"ColumnID":4,"Name":"w","TableID":108}
  │              ├── UpsertColumnType {"ColumnType":{"ColumnID":4,"TableID":108}}
@@ -68,6 +71,8 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
  │    │    │    ├── PUBLIC           → ABSENT IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 1 (k), IndexID: 3}
  │    │    │    ├── PUBLIC           → ABSENT IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 2 (v), IndexID: 3}
  │    │    │    └── PUBLIC           → ABSENT IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 4 (w+), IndexID: 3}
+ │    │    ├── 1 element transitioning toward TRANSIENT_PUBLIC
+ │    │    │    └── ABSENT           → PUBLIC TableSchemaLocked:{DescID: 108 (table_regional_by_row)}
  │    │    └── 1 Mutation operation
  │    │         └── UndoAllInTxnImmediateMutationOpSideEffects
  │    └── Stage 2 of 2 in PreCommitPhase
@@ -90,7 +95,10 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
  │         │    ├── ABSENT → PUBLIC           IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 1 (k), IndexID: 3}
  │         │    ├── ABSENT → PUBLIC           IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 2 (v), IndexID: 3}
  │         │    └── ABSENT → PUBLIC           IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 4 (w+), IndexID: 3}
- │         └── 20 Mutation operations
+ │         ├── 1 element transitioning toward TRANSIENT_PUBLIC
+ │         │    └── PUBLIC → ABSENT           TableSchemaLocked:{DescID: 108 (table_regional_by_row)}
+ │         └── 21 Mutation operations
+ │              ├── SetTableSchemaLocked {"TableID":108}
  │              ├── MakeAbsentColumnDeleteOnly {"Column":{"ColumnID":4,"TableID":108}}
  │              ├── SetColumnName {"ColumnID":4,"Name":"w","TableID":108}
  │              ├── UpsertColumnType {"ColumnType":{"ColumnID":4,"TableID":108}}
@@ -167,7 +175,7 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
  │              ├── ValidateIndex {"IndexID":2,"TableID":108}
  │              └── ValidateColumnNotNull {"ColumnID":4,"IndexIDForValidation":2,"TableID":108}
  └── PostCommitNonRevertiblePhase
-      ├── Stage 1 of 3 in PostCommitNonRevertiblePhase
+      ├── Stage 1 of 4 in PostCommitNonRevertiblePhase
       │    ├── 4 elements transitioning toward PUBLIC
       │    │    ├── WRITE_ONLY            → PUBLIC           Column:{DescID: 108 (table_regional_by_row), ColumnID: 4 (w+)}
       │    │    ├── VALIDATED             → PUBLIC           PrimaryIndex:{DescID: 108 (table_regional_by_row), IndexID: 2 (table_regional_by_row_pkey+), ConstraintID: 2, TemporaryIndexID: 3, SourceIndexID: 1 (table_regional_by_row_pkey-)}
@@ -198,7 +206,7 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
       │         ├── RefreshStats {"TableID":108}
       │         ├── SetJobStateOnDescriptor {"DescriptorID":108}
       │         └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"PostCommitNonRev..."}
-      ├── Stage 2 of 3 in PostCommitNonRevertiblePhase
+      ├── Stage 2 of 4 in PostCommitNonRevertiblePhase
       │    ├── 4 elements transitioning toward ABSENT
       │    │    ├── PUBLIC    → ABSENT      IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 3 (crdb_region), IndexID: 1 (table_regional_by_row_pkey-)}
       │    │    ├── PUBLIC    → ABSENT      IndexColumn:{DescID: 108 (table_regional_by_row), ColumnID: 1 (k), IndexID: 1 (table_regional_by_row_pkey-)}
@@ -211,15 +219,22 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
       │         ├── RemoveColumnFromIndex {"ColumnID":2,"IndexID":1,"Kind":2,"TableID":108}
       │         ├── SetJobStateOnDescriptor {"DescriptorID":108}
       │         └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"PostCommitNonRev..."}
-      └── Stage 3 of 3 in PostCommitNonRevertiblePhase
-           ├── 1 element transitioning toward TRANSIENT_ABSENT
-           │    └── PUBLIC      → TRANSIENT_ABSENT IndexData:{DescID: 108 (table_regional_by_row), IndexID: 3}
-           ├── 2 elements transitioning toward ABSENT
-           │    ├── DELETE_ONLY → ABSENT           PrimaryIndex:{DescID: 108 (table_regional_by_row), IndexID: 1 (table_regional_by_row_pkey-), ConstraintID: 1}
-           │    └── PUBLIC      → ABSENT           IndexData:{DescID: 108 (table_regional_by_row), IndexID: 1 (table_regional_by_row_pkey-)}
-           └── 5 Mutation operations
-                ├── MakeIndexAbsent {"IndexID":1,"TableID":108}
-                ├── CreateGCJobForIndex {"IndexID":1,"TableID":108}
-                ├── CreateGCJobForIndex {"IndexID":3,"TableID":108}
+      ├── Stage 3 of 4 in PostCommitNonRevertiblePhase
+      │    ├── 1 element transitioning toward TRANSIENT_ABSENT
+      │    │    └── PUBLIC      → TRANSIENT_ABSENT IndexData:{DescID: 108 (table_regional_by_row), IndexID: 3}
+      │    ├── 2 elements transitioning toward ABSENT
+      │    │    ├── DELETE_ONLY → ABSENT           PrimaryIndex:{DescID: 108 (table_regional_by_row), IndexID: 1 (table_regional_by_row_pkey-), ConstraintID: 1}
+      │    │    └── PUBLIC      → ABSENT           IndexData:{DescID: 108 (table_regional_by_row), IndexID: 1 (table_regional_by_row_pkey-)}
+      │    └── 5 Mutation operations
+      │         ├── MakeIndexAbsent {"IndexID":1,"TableID":108}
+      │         ├── CreateGCJobForIndex {"IndexID":1,"TableID":108}
+      │         ├── CreateGCJobForIndex {"IndexID":3,"TableID":108}
+      │         ├── SetJobStateOnDescriptor {"DescriptorID":108}
+      │         └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"PostCommitNonRev..."}
+      └── Stage 4 of 4 in PostCommitNonRevertiblePhase
+           ├── 1 element transitioning toward TRANSIENT_PUBLIC
+           │    └── ABSENT → TRANSIENT_PUBLIC TableSchemaLocked:{DescID: 108 (table_regional_by_row)}
+           └── 3 Mutation operations
+                ├── SetTableSchemaLocked {"Locked":true,"TableID":108}
                 ├── RemoveJobStateFromDescriptor {"DescriptorID":108}
                 └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"all stages compl..."}

pkg/ccl/schemachangerccl/testdata/end_to_end/add_column_regional_by_row/add_column_regional_by_row.explain

@@ -18,4 +18,4 @@ Schema change plan for ALTER TABLE ‹multiregion_db›.‹public›.‹table_re
  ├── execute 1 system table mutations transaction
  ├── validate UNIQUE constraint backed by index table_regional_by_row_pkey+ in relation table_regional_by_row
  ├── validate NOT NULL constraint on column w+ in index table_regional_by_row_pkey+ in relation table_regional_by_row
- └── execute 3 system table mutations transactions
+ └── execute 4 system table mutations transactions