diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index 8aebb9576135..256ca0cd3af3 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -1917,8 +1917,8 @@ func revalidateIndexes( // We don't actually need the 'historical' read the way the schema change does // since our table is offline. runner := descs.NewHistoricalInternalExecTxnRunner(hlc.Timestamp{}, func(ctx context.Context, fn descs.InternalExecFn) error { - return execCfg.InternalDB.Txn(ctx, func(ctx context.Context, txn isql.Txn) error { - return fn(ctx, txn, descs.FromTxn(txn)) + return execCfg.InternalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error { + return fn(ctx, txn) }) }) diff --git a/pkg/ccl/schemachangerccl/backup_base_generated_test.go b/pkg/ccl/schemachangerccl/backup_base_generated_test.go index 8db271e972e9..7ff06900a66e 100644 --- a/pkg/ccl/schemachangerccl/backup_base_generated_test.go +++ b/pkg/ccl/schemachangerccl/backup_base_generated_test.go @@ -38,6 +38,11 @@ func TestBackup_base_add_column_no_default(t *testing.T) { defer log.Scope(t).Close(t) sctest.Backup(t, "pkg/sql/schemachanger/testdata/end_to_end/add_column_no_default", newCluster) } +func TestBackup_base_alter_table_add_check_unvalidated(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.Backup(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated", newCluster) +} func TestBackup_base_alter_table_add_check_vanilla(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/cmd/roachtest/tests/inconsistency.go b/pkg/cmd/roachtest/tests/inconsistency.go index 164d6912e8de..9ca222f1c4e2 100644 --- a/pkg/cmd/roachtest/tests/inconsistency.go +++ b/pkg/cmd/roachtest/tests/inconsistency.go @@ -32,11 +32,9 @@ func registerInconsistency(r registry.Registry) { } func runInconsistency(ctx context.Context, t test.Test, c cluster.Cluster) { - startOps := option.DefaultStartOpts() - nodes := c.Range(1, 3) c.Put(ctx, t.Cockroach(), "./cockroach", nodes) - c.Start(ctx, t.L(), startOps, install.MakeClusterSettings(), nodes) + c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), nodes) { db := c.Conn(ctx, t.L(), 1) @@ -45,19 +43,18 @@ func runInconsistency(ctx context.Context, t test.Test, c cluster.Cluster) { // to expect it. _, err := db.ExecContext(ctx, `SET CLUSTER SETTING server.consistency_check.interval = '0'`) require.NoError(t, err) - err = WaitFor3XReplication(ctx, t, db) - require.NoError(t, err) - _, db = db.Close(), nil + require.NoError(t, WaitFor3XReplication(ctx, t, db)) + require.NoError(t, db.Close()) } // Stop the cluster "gracefully" by letting each node initiate a "hard - // shutdown" This will prevent any potential problems in which data isn't + // shutdown". This will prevent any potential problems in which data isn't // synced. It seems (remotely) possible (see #64602) that without this, we // sometimes let the inconsistency win by ending up replicating it to all // nodes. This has not been conclusively proven, though. // - // First SIGINT initiates graceful shutdown, second one initiates a - // "hard" (i.e. don't shed leases, etc) shutdown. + // First SIGINT initiates graceful shutdown, second one initiates a "hard" + // (i.e. don't shed leases, etc) shutdown. stopOpts := option.DefaultStopOpts() stopOpts.RoachprodOpts.Wait = false stopOpts.RoachprodOpts.Sig = 2 @@ -95,7 +92,8 @@ func runInconsistency(ctx context.Context, t test.Test, c cluster.Cluster) { // If the consistency check "fails to fail", the verbose logging will help // determine why. startOpts := option.DefaultStartOpts() - startOpts.RoachprodOpts.ExtraArgs = append(startOpts.RoachprodOpts.ExtraArgs, "--vmodule=consistency_queue=5,replica_consistency=5,queue=5") + startOpts.RoachprodOpts.ExtraArgs = append(startOpts.RoachprodOpts.ExtraArgs, + "--vmodule=consistency_queue=5,replica_consistency=5,queue=5") c.Start(ctx, t.L(), startOpts, install.MakeClusterSettings(), nodes) m.Go(func(ctx context.Context) error { select { @@ -107,54 +105,57 @@ func runInconsistency(ctx context.Context, t test.Test, c cluster.Cluster) { time.Sleep(10 * time.Second) // wait for n1-n3 to all be known as live to each other - // set an aggressive consistency check interval, but only now (that we're + // Set an aggressive consistency check interval, but only now (that we're // reasonably sure all nodes are live, etc). This makes sure that the consistency // check runs against all three nodes. If it targeted only two nodes, a random // one would fatal - not what we want. { db := c.Conn(ctx, t.L(), 2) _, err := db.ExecContext(ctx, `SET CLUSTER SETTING server.consistency_check.interval = '10ms'`) - if err != nil { - t.Fatal(err) - } - _ = db.Close() - } - - if err := m.WaitE(); err == nil { - t.Fatal("expected a node to crash") + require.NoError(t, err) + require.NoError(t, db.Close()) } + require.Error(t, m.WaitE(), "expected a node to crash") time.Sleep(20 * time.Second) // wait for liveness to time out for dead nodes db := c.Conn(ctx, t.L(), 2) rows, err := db.Query(`SELECT node_id FROM crdb_internal.gossip_nodes WHERE is_live = false;`) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) var ids []int for rows.Next() { var id int - if err := rows.Scan(&id); err != nil { - t.Fatal(err) - } + require.NoError(t, rows.Scan(&id)) ids = append(ids, id) } - if err := rows.Err(); err != nil { - t.Fatal(err) - } - if len(ids) != 1 { - t.Fatalf("expected one dead NodeID, got %v", ids) - } - const expr = "This.node.is.terminating.because.a.replica.inconsistency.was.detected" - c.Run(ctx, c.Node(1), "grep "+ - expr+" "+"{log-dir}/cockroach.log") + require.NoError(t, rows.Err()) + require.Len(t, ids, 1, "expected one dead NodeID") - if err := c.StartE(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.Node(1)); err == nil { - // NB: we can't easily verify the error because there's a lot of output - // which isn't fully included in the error returned from StartE. - t.Fatalf("node restart should have failed") + const expr = "This.node.is.terminating.because.a.replica.inconsistency.was.detected" + c.Run(ctx, c.Node(1), "grep "+expr+" {log-dir}/cockroach.log") + + // Make sure that every node creates a checkpoint. + for n := 1; n <= 3; n++ { + // Notes it in the log. + const expr = "creating.checkpoint.*with.spans" + c.Run(ctx, c.Node(n), "grep "+expr+" {log-dir}/cockroach.log") + // Creates at least one checkpoint directory (in rare cases it can be + // multiple if multiple consistency checks fail in close succession), and + // puts spans information into the checkpoint.txt file in it. + c.Run(ctx, c.Node(n), "find {store-dir}/auxiliary/checkpoints -name checkpoint.txt") + // The checkpoint can be inspected by the tooling. + c.Run(ctx, c.Node(n), "./cockroach debug range-descriptors "+ + "$(find {store-dir}/auxiliary/checkpoints/* -type d -depth 0 | head -n1)") + c.Run(ctx, c.Node(n), "./cockroach debug range-data --limit 10 "+ + "$(find {store-dir}/auxiliary/checkpoints/* -type d -depth 0 | head -n1) 1") } + // NB: we can't easily verify the error because there's a lot of output which + // isn't fully included in the error returned from StartE. + require.Error(t, c.StartE( + ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.Node(1), + ), "node restart should have failed") + // roachtest checks that no nodes are down when the test finishes, but in this // case we have a down node that we can't restart. Remove the data dir, which // tells roachtest to ignore this node. diff --git a/pkg/jobs/registry.go b/pkg/jobs/registry.go index a8f47562e251..2f54afd45587 100644 --- a/pkg/jobs/registry.go +++ b/pkg/jobs/registry.go @@ -111,8 +111,8 @@ type Registry struct { adoptionCh chan adoptionNotice sqlInstance sqlliveness.Instance - // sessionBoundInternalExecutorFactory provides a way for jobs to create - // internal executors. This is rarely needed, and usually job resumers should + // internalDB provides a way for jobs to create internal executors. + // This is rarely needed, and usually job resumers should // use the internal executor from the JobExecCtx. The intended user of this // interface is the schema change job resumer, which needs to set the // tableCollectionModifier on the internal executor to different values in diff --git a/pkg/kv/kvserver/consistency_queue_test.go b/pkg/kv/kvserver/consistency_queue_test.go index 993c64705e98..6f670bd51392 100644 --- a/pkg/kv/kvserver/consistency_queue_test.go +++ b/pkg/kv/kvserver/consistency_queue_test.go @@ -258,9 +258,9 @@ func TestCheckConsistencyInconsistent(t *testing.T) { stickyEngineRegistry := server.NewStickyInMemEnginesRegistry() defer stickyEngineRegistry.CloseAllStickyInMemEngines() - // The cluster has 3 node, with 1 store per node. The test writes a few KVs to - // a range, which gets replicated to all 3 stores. Then it manually replaces - // an entry in s2. The consistency check must detect this and terminate n2/s2. + // The cluster has 3 nodes, one store per node. The test writes a few KVs to a + // range, which gets replicated to all 3 stores. Then it manually replaces an + // entry in s2. The consistency check must detect this and terminate n2/s2. const numStores = 3 testKnobs := kvserver.StoreTestingKnobs{DisableConsistencyQueue: true} var tc *testcluster.TestCluster diff --git a/pkg/kv/kvserver/replica_consistency.go b/pkg/kv/kvserver/replica_consistency.go index 1e90400f3234..3781f4a2846e 100644 --- a/pkg/kv/kvserver/replica_consistency.go +++ b/pkg/kv/kvserver/replica_consistency.go @@ -665,8 +665,10 @@ func (r *Replica) computeChecksumPostApply( } // NB: the names here will match on all nodes, which is nice for debugging. tag := fmt.Sprintf("r%d_at_%d", r.RangeID, as.RaftAppliedIndex) - if dir, err := r.store.checkpoint(ctx, tag); err != nil { - log.Warningf(ctx, "unable to create checkpoint %s: %+v", dir, err) + spans := r.store.checkpointSpans(&desc) + log.Warningf(ctx, "creating checkpoint %s with spans %+v", tag, spans) + if dir, err := r.store.checkpoint(tag, spans); err != nil { + log.Warningf(ctx, "unable to create checkpoint %s: %+v", tag, err) } else { log.Warningf(ctx, "created checkpoint %s", dir) } @@ -721,7 +723,6 @@ func (r *Replica) computeChecksumPostApply( } r.computeChecksumDone(c, result) } - var shouldFatal bool for _, rDesc := range cc.Terminate { if rDesc.StoreID == r.store.StoreID() && rDesc.ReplicaID == r.replicaID { @@ -757,23 +758,19 @@ A file preventing this node from restarting was placed at: Checkpoints are created on each node/store hosting this range, to help investigate the cause. Only nodes that are more likely to have incorrect data -are terminated, and usually a majority of replicas continue running. - -The storage checkpoint directory MUST be deleted or moved away timely, on the -nodes that continue operating. Over time the storage engine gets updated and -compacted, which leads to checkpoints becoming a full copy of a past state. Even -with no writes to the database, on these stores disk consumption may double in a -matter of hours/days, depending on compaction schedule. +are terminated, and usually a majority of replicas continue running. Checkpoints +are partial, i.e. contain only the data from to the inconsistent range, and +possibly its neighbouring ranges. -Checkpoints are very helpful in debugging this issue, so before deleting them, -please consider alternative actions: +The storage checkpoint directories can/should be deleted when no longer needed. +They are very helpful in debugging this issue, so before deleting them, please +consider alternative actions: -- If the store has enough capacity, hold off deleting the checkpoint until CRDB - staff has diagnosed the issue. -- Consider backing up the checkpoints before removing them, e.g. by snapshotting - the disk. +- If the store has enough capacity, hold off the deletion until CRDB staff has + diagnosed the issue. +- Back up the checkpoints for later investigation. - If the stores are nearly full, but the cluster has enough capacity, consider - gradually decomissioning the affected nodes, to retain the checkpoints. + gradually decommissioning the affected nodes, to retain the checkpoints. To inspect the checkpoints, one can use the cockroach debug range-data tool, and command line tools like diff. For example: diff --git a/pkg/kv/kvserver/replica_consistency_test.go b/pkg/kv/kvserver/replica_consistency_test.go index 018b27834689..78626f5ee4b2 100644 --- a/pkg/kv/kvserver/replica_consistency_test.go +++ b/pkg/kv/kvserver/replica_consistency_test.go @@ -79,6 +79,92 @@ func TestReplicaChecksumVersion(t *testing.T) { }) } +func TestStoreCheckpointSpans(t *testing.T) { + defer leaktest.AfterTest(t)() + + s := Store{} + s.mu.replicasByKey = newStoreReplicaBTree() + s.mu.replicaPlaceholders = map[roachpb.RangeID]*ReplicaPlaceholder{} + + makeDesc := func(rangeID roachpb.RangeID, start, end string) roachpb.RangeDescriptor { + desc := roachpb.RangeDescriptor{RangeID: rangeID} + if start != "" { + desc.StartKey = roachpb.RKey(start) + desc.EndKey = roachpb.RKey(end) + } + return desc + } + var descs []roachpb.RangeDescriptor + addReplica := func(rangeID roachpb.RangeID, start, end string) { + desc := makeDesc(rangeID, start, end) + r := &Replica{RangeID: rangeID, startKey: desc.StartKey} + r.mu.state.Desc = &desc + r.isInitialized.Set(desc.IsInitialized()) + require.NoError(t, s.addToReplicasByRangeIDLocked(r)) + if r.IsInitialized() { + require.NoError(t, s.addToReplicasByKeyLocked(r)) + descs = append(descs, desc) + } + } + addPlaceholder := func(rangeID roachpb.RangeID, start, end string) { + require.NoError(t, s.addPlaceholderLocked( + &ReplicaPlaceholder{rangeDesc: makeDesc(rangeID, start, end)}, + )) + } + + addReplica(1, "a", "b") + addReplica(4, "b", "c") + addPlaceholder(5, "c", "d") + addReplica(2, "e", "f") + addReplica(3, "", "") // uninitialized + + want := [][]string{{ + // r1 with keys [a, b). The checkpoint includes range-ID replicated and + // unreplicated keyspace for ranges 1-2 and 4. Range 2 is included because + // it's a neighbour of r1 by range ID. The checkpoint also includes + // replicated user keyspace {a-c} owned by ranges 1 and 4. + "/Local/RangeID/{1\"\"-3\"\"}", + "/Local/RangeID/{4\"\"-5\"\"}", + "/Local/Range\"{a\"-c\"}", + "/Local/Lock/Intent/Local/Range\"{a\"-c\"}", + "/Local/Lock/Intent\"{a\"-c\"}", + "{a-c}", + }, { + // r4 with keys [b, c). The checkpoint includes range-ID replicated and + // unreplicated keyspace for ranges 3-4, 1 and 2. Range 3 is included + // because it's a neighbour of r4 by range ID. The checkpoint also includes + // replicated user keyspace {a-f} owned by ranges 1, 4, and 2. + "/Local/RangeID/{3\"\"-5\"\"}", + "/Local/RangeID/{1\"\"-2\"\"}", + "/Local/RangeID/{2\"\"-3\"\"}", + "/Local/Range\"{a\"-f\"}", + "/Local/Lock/Intent/Local/Range\"{a\"-f\"}", + "/Local/Lock/Intent\"{a\"-f\"}", + "{a-f}", + }, { + // r2 with keys [e, f). The checkpoint includes range-ID replicated and + // unreplicated keyspace for ranges 1-3 and 4. Ranges 1 and 3 are included + // because they are neighbours of r2 by range ID. The checkpoint also + // includes replicated user keyspace {b-f} owned by ranges 4 and 2. + "/Local/RangeID/{1\"\"-4\"\"}", + "/Local/RangeID/{4\"\"-5\"\"}", + "/Local/Range\"{b\"-f\"}", + "/Local/Lock/Intent/Local/Range\"{b\"-f\"}", + "/Local/Lock/Intent\"{b\"-f\"}", + "{b-f}", + }} + + require.Len(t, want, len(descs)) + for i, desc := range descs { + spans := s.checkpointSpans(&desc) + got := make([]string, 0, len(spans)) + for _, s := range spans { + got = append(got, s.String()) + } + require.Equal(t, want[i], got, i) + } +} + func TestGetChecksumNotSuccessfulExitConditions(t *testing.T) { defer leaktest.AfterTest(t)() diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index 4d9f7fcc829c..06dd59d52255 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -50,6 +50,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/multiqueue" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/raftentry" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/rangefeed" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/tenantrate" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/tscache" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/txnrecovery" @@ -2879,18 +2880,77 @@ func (s *Store) checkpointsDir() string { return filepath.Join(s.engine.GetAuxiliaryDir(), "checkpoints") } -// checkpoint creates a RocksDB checkpoint in the auxiliary directory with the -// provided tag used in the filepath. The filepath for the checkpoint directory -// is returned. -func (s *Store) checkpoint(ctx context.Context, tag string) (string, error) { +// checkpointSpans returns key spans containing the given range. The spans may +// be wider, and contain a few extra ranges that surround the given range. The +// extension of the spans gives more information for debugging consistency or +// storage issues, e.g. in situations when a recent reconfiguration like split +// or merge occurred. +func (s *Store) checkpointSpans(desc *roachpb.RangeDescriptor) []roachpb.Span { + // Find immediate left and right neighbours by range ID. + var prevID, nextID roachpb.RangeID + s.mu.replicasByRangeID.Range(func(r *Replica) { + if id, our := r.RangeID, desc.RangeID; id < our && id > prevID { + prevID = id + } else if id > our && (nextID == 0 || id < nextID) { + nextID = id + } + }) + if prevID == 0 { + prevID = desc.RangeID + } + if nextID == 0 { + nextID = desc.RangeID + } + + // Find immediate left and right neighbours by user key. + s.mu.RLock() + left := s.mu.replicasByKey.LookupPrecedingReplica(context.Background(), desc.StartKey) + right := s.mu.replicasByKey.LookupNextReplica(context.Background(), desc.EndKey) + s.mu.RUnlock() + + // Cover all range IDs (prevID, desc.RangeID, nextID) using a continuous span. + spanRangeIDs := func(first, last roachpb.RangeID) roachpb.Span { + return roachpb.Span{ + Key: keys.MakeRangeIDPrefix(first), + EndKey: keys.MakeRangeIDPrefix(last).PrefixEnd(), + } + } + spans := []roachpb.Span{spanRangeIDs(prevID, nextID)} + + userKeys := desc.RSpan() + // Include the rangeID-local data comprising ranges left, desc, and right. + if left != nil { + userKeys.Key = left.Desc().StartKey + // Skip this range ID if it was already covered by [prevID, nextID]. + if id := left.RangeID; id < prevID || id > nextID { + spans = append(spans, spanRangeIDs(id, id)) + } + } + if right != nil { + userKeys.EndKey = right.Desc().EndKey + // Skip this range ID if it was already covered by [prevID, nextID]. + if id := right.RangeID; id < prevID || id > nextID { + spans = append(spans, spanRangeIDs(id, id)) + } + } + // Include replicated user key span containing ranges left, desc, and right. + // TODO(tbg): rangeID is ignored here, make a rangeID-agnostic helper. + spans = append(spans, rditer.Select(0, rditer.SelectOpts{ReplicatedBySpan: userKeys})...) + + return spans +} + +// checkpoint creates a Pebble checkpoint in the auxiliary directory with the +// provided tag used in the filepath. Returns the path to the created checkpoint +// directory. The checkpoint includes only files that intersect with either of +// the provided key spans. If spans is empty, it includes the entire store. +func (s *Store) checkpoint(tag string, spans []roachpb.Span) (string, error) { checkpointBase := s.checkpointsDir() _ = s.engine.MkdirAll(checkpointBase) - checkpointDir := filepath.Join(checkpointBase, tag) - if err := s.engine.CreateCheckpoint(checkpointDir); err != nil { + if err := s.engine.CreateCheckpoint(checkpointDir, spans); err != nil { return "", err } - return checkpointDir, nil } diff --git a/pkg/kv/kvserver/store_replica_btree.go b/pkg/kv/kvserver/store_replica_btree.go index 45825392db36..1530511dc2a9 100644 --- a/pkg/kv/kvserver/store_replica_btree.go +++ b/pkg/kv/kvserver/store_replica_btree.go @@ -89,6 +89,22 @@ func (b *storeReplicaBTree) LookupPrecedingReplica(ctx context.Context, key roac return repl } +// LookupNextReplica returns the first / leftmost Replica (if any) whose start +// key is greater or equal to the provided key. Placeholders are ignored. +func (b *storeReplicaBTree) LookupNextReplica(ctx context.Context, key roachpb.RKey) *Replica { + var repl *Replica + if err := b.ascendRange(ctx, key, roachpb.RKeyMax, func(_ context.Context, it replicaOrPlaceholder) error { + if it.repl != nil { + repl = it.repl + return iterutil.StopIteration() + } + return nil + }); err != nil { + panic(err) + } + return repl +} + // VisitKeyRange invokes the visitor on all the replicas for ranges that // overlap [startKey, endKey), or until the visitor returns false, in the // specific order. (The items in the btree are non-overlapping). diff --git a/pkg/kv/kvserver/store_replica_btree_test.go b/pkg/kv/kvserver/store_replica_btree_test.go index 2a89c101910c..3b13f004f3c9 100644 --- a/pkg/kv/kvserver/store_replica_btree_test.go +++ b/pkg/kv/kvserver/store_replica_btree_test.go @@ -87,7 +87,7 @@ func TestStoreReplicaBTree_VisitKeyRange(t *testing.T) { }) } -func TestStoreReplicaBTree_LookupPrecedingReplica(t *testing.T) { +func TestStoreReplicaBTree_LookupPrecedingAndNextReplica(t *testing.T) { defer leaktest.AfterTest(t)() ctx := context.Background() @@ -116,25 +116,29 @@ func TestStoreReplicaBTree_LookupPrecedingReplica(t *testing.T) { require.Zero(t, b.ReplaceOrInsertReplica(ctx, repl5)) for i, tc := range []struct { - key string - expRepl *Replica + key string + preRepl *Replica + nextRepl *Replica }{ - {"", nil}, - {"a", nil}, - {"aa", nil}, - {"b", repl2}, - {"bb", repl2}, - {"c", repl3}, - {"cc", repl3}, - {"d", repl3}, - {"dd", repl3}, - {"e", repl3}, - {"ee", repl3}, - {"f", repl5}, - {"\xff\xff", repl5}, + {"", nil, repl2}, + {"a", nil, repl2}, + {"aa", nil, repl3}, + {"b", repl2, repl3}, + {"bb", repl2, repl5}, + {"c", repl3, repl5}, + {"cc", repl3, repl5}, + {"d", repl3, repl5}, + {"dd", repl3, repl5}, + {"e", repl3, repl5}, + {"ee", repl3, nil}, + {"f", repl5, nil}, + {"\xff\xff", repl5, nil}, } { - if repl := b.LookupPrecedingReplica(ctx, roachpb.RKey(tc.key)); repl != tc.expRepl { - t.Errorf("%d: expected replica %v; got %v", i, tc.expRepl, repl) + if got, want := b.LookupPrecedingReplica(ctx, roachpb.RKey(tc.key)), tc.preRepl; got != want { + t.Errorf("%d: expected preceding replica %v; got %v", i, want, got) + } + if got, want := b.LookupNextReplica(ctx, roachpb.RKey(tc.key)), tc.nextRepl; got != want { + t.Errorf("%d: expected next replica %v; got %v", i, want, got) } } } diff --git a/pkg/server/debug/replay/replay.go b/pkg/server/debug/replay/replay.go index f0ea6342dbff..7a9adec314c2 100644 --- a/pkg/server/debug/replay/replay.go +++ b/pkg/server/debug/replay/replay.go @@ -85,7 +85,7 @@ func (h *HTTPHandler) HandleRequest(w http.ResponseWriter, req *http.Request) { } if !wc.IsRunning() { wc.Start(captureFS, actionJSON.CaptureDirectory) - err = s.Engine().CreateCheckpoint(captureFS.PathJoin(actionJSON.CaptureDirectory, "checkpoint")) + err = s.Engine().CreateCheckpoint(captureFS.PathJoin(actionJSON.CaptureDirectory, "checkpoint"), nil) } if err != nil { return err diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 13287ece04da..465f6b4219dd 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -545,7 +545,7 @@ func (n *alterTableNode) startExec(params runParams) error { ck.CheckDesc().Validity = descpb.ConstraintValidity_Validated } else if fk := c.AsForeignKey(); fk != nil { if err := validateFkInTxn( - params.ctx, params.p.InternalSQLTxn(), params.p.descCollection, n.tableDesc, name, + params.ctx, params.p.InternalSQLTxn(), n.tableDesc, name, ); err != nil { return err } diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 3edce37697c3..8f1e4fa0de10 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -140,7 +140,7 @@ func (sc *SchemaChanger) getChunkSize(chunkSize int64) int64 { } // scTxnFn is the type of functions that operates using transactions in the backfiller. -type scTxnFn func(ctx context.Context, txn isql.Txn, evalCtx *extendedEvalContext) error +type scTxnFn func(ctx context.Context, txn descs.Txn, evalCtx *extendedEvalContext) error // historicalTxnRunner is the type of the callback used by the various // helper functions to run checks at a fixed timestamp (logically, at @@ -152,12 +152,10 @@ func (sc *SchemaChanger) makeFixedTimestampRunner(readAsOf hlc.Timestamp) histor runner := func(ctx context.Context, retryable scTxnFn) error { return sc.fixedTimestampTxnWithExecutor(ctx, readAsOf, func( ctx context.Context, - txn isql.Txn, - sd *sessiondata.SessionData, - descriptors *descs.Collection, + txn descs.Txn, ) error { // We need to re-create the evalCtx since the txn may retry. - evalCtx := createSchemaChangeEvalCtx(ctx, sc.execCfg, sd, readAsOf, descriptors) + evalCtx := createSchemaChangeEvalCtx(ctx, sc.execCfg, txn.SessionData(), readAsOf, txn.Descriptors()) return retryable(ctx, txn, &evalCtx) }) } @@ -173,11 +171,9 @@ func (sc *SchemaChanger) makeFixedTimestampInternalExecRunner( ) error { return sc.fixedTimestampTxnWithExecutor(ctx, readAsOf, func( ctx context.Context, - txn isql.Txn, - _ *sessiondata.SessionData, - descriptors *descs.Collection, + txn descs.Txn, ) error { - return retryable(ctx, txn, descriptors) + return retryable(ctx, txn) }) }) } @@ -185,13 +181,12 @@ func (sc *SchemaChanger) makeFixedTimestampInternalExecRunner( func (sc *SchemaChanger) fixedTimestampTxn( ctx context.Context, readAsOf hlc.Timestamp, - retryable func(ctx context.Context, txn isql.Txn, descriptors *descs.Collection) error, + retryable func(ctx context.Context, txn descs.Txn) error, ) error { return sc.fixedTimestampTxnWithExecutor(ctx, readAsOf, func( - ctx context.Context, txn isql.Txn, _ *sessiondata.SessionData, - descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - return retryable(ctx, txn, descriptors) + return retryable(ctx, txn) }) } @@ -200,18 +195,16 @@ func (sc *SchemaChanger) fixedTimestampTxnWithExecutor( readAsOf hlc.Timestamp, retryable func( ctx context.Context, - txn isql.Txn, - sd *sessiondata.SessionData, - descriptors *descs.Collection, + txn descs.Txn, ) error, ) error { return sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { if err := txn.KV().SetFixedTimestamp(ctx, readAsOf); err != nil { return err } - return retryable(ctx, txn, txn.SessionData(), descriptors) + return retryable(ctx, txn) }) } @@ -446,8 +439,8 @@ func (sc *SchemaChanger) dropConstraints( } // Create update closure for the table and all other tables with backreferences. - if err := sc.txn(ctx, func(ctx context.Context, txn isql.Txn, descsCol *descs.Collection) error { - scTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + if err := sc.txn(ctx, func(ctx context.Context, txn descs.Txn) error { + scTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } @@ -477,7 +470,7 @@ func (sc *SchemaChanger) dropConstraints( if def.Name != constraint.GetName() { continue } - backrefTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) + backrefTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) if err != nil { return err } @@ -486,7 +479,7 @@ func (sc *SchemaChanger) dropConstraints( ); err != nil { return err } - if err := descsCol.WriteDescToBatch( + if err := txn.Descriptors().WriteDescToBatch( ctx, true /* kvTrace */, backrefTable, b, ); err != nil { return err @@ -525,7 +518,7 @@ func (sc *SchemaChanger) dropConstraints( } } } - if err := descsCol.WriteDescToBatch( + if err := txn.Descriptors().WriteDescToBatch( ctx, true /* kvTrace */, scTable, b, ); err != nil { return err @@ -538,13 +531,13 @@ func (sc *SchemaChanger) dropConstraints( log.Info(ctx, "finished dropping constraints") tableDescs := make(map[descpb.ID]catalog.TableDescriptor, len(fksByBackrefTable)+1) if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) (err error) { - if tableDescs[sc.descID], err = descsCol.ByIDWithLeased(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID); err != nil { + if tableDescs[sc.descID], err = txn.Descriptors().ByIDWithLeased(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID); err != nil { return err } for id := range fksByBackrefTable { - desc, err := descsCol.ByIDWithLeased(txn.KV()).WithoutOffline().Get().Table(ctx, id) + desc, err := txn.Descriptors().ByIDWithLeased(txn.KV()).WithoutOffline().Get().Table(ctx, id) if err != nil { return err } @@ -581,9 +574,9 @@ func (sc *SchemaChanger) addConstraints( // Create update closure for the table and all other tables with backreferences if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - scTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + scTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } @@ -632,7 +625,7 @@ func (sc *SchemaChanger) addConstraints( } if !foundExisting { scTable.OutboundFKs = append(scTable.OutboundFKs, *fk.ForeignKeyDesc()) - backrefTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) + backrefTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) if err != nil { return err } @@ -656,7 +649,7 @@ func (sc *SchemaChanger) addConstraints( // the last put will win but it's perhaps not ideal. We could add // code to deduplicate but it doesn't seem worth the hassle. if backrefTable != scTable { - if err := descsCol.WriteDescToBatch( + if err := txn.Descriptors().WriteDescToBatch( ctx, true /* kvTrace */, backrefTable, b, ); err != nil { return err @@ -686,7 +679,7 @@ func (sc *SchemaChanger) addConstraints( } } } - if err := descsCol.WriteDescToBatch( + if err := txn.Descriptors().WriteDescToBatch( ctx, true /* kvTrace */, scTable, b, ); err != nil { return err @@ -727,9 +720,9 @@ func (sc *SchemaChanger) validateConstraints( var tableDesc catalog.TableDescriptor if err := sc.fixedTimestampTxn(ctx, readAsOf, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - tableDesc, err = descriptors.ByID(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID) + tableDesc, err = txn.Descriptors().ByID(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID) return err }); err != nil { return err @@ -756,7 +749,7 @@ func (sc *SchemaChanger) validateConstraints( } desc := descI.(*tabledesc.Mutable) // Each check operates at the historical timestamp. - return runHistoricalTxn(ctx, func(ctx context.Context, txn isql.Txn, evalCtx *extendedEvalContext) error { + return runHistoricalTxn(ctx, func(ctx context.Context, txn descs.Txn, evalCtx *extendedEvalContext) error { // If the constraint is a check constraint that fails validation, we // need a semaContext set up that can resolve types in order to pretty // print the check expression back to the user. @@ -782,7 +775,7 @@ func (sc *SchemaChanger) validateConstraints( return err } } else if c.AsForeignKey() != nil { - if err := validateFkInTxn(ctx, txn, collection, desc, c.GetName()); err != nil { + if err := validateFkInTxn(ctx, txn, desc, c.GetName()); err != nil { return err } } else if c.AsUniqueWithoutIndex() != nil { @@ -922,10 +915,10 @@ func (sc *SchemaChanger) distIndexBackfill( var mutationIdx int if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, col *descs.Collection, + ctx context.Context, txn descs.Txn, ) (err error) { todoSpans, _, mutationIdx, err = rowexec.GetResumeSpans( - ctx, sc.jobRegistry, txn, sc.execCfg.Codec, col, sc.descID, + ctx, sc.jobRegistry, txn, sc.execCfg.Codec, txn.Descriptors(), sc.descID, sc.mutationID, filter, ) return err @@ -960,7 +953,7 @@ func (sc *SchemaChanger) distIndexBackfill( const pageSize = 10000 noop := func(_ []kv.KeyValue) error { return nil } if err := sc.fixedTimestampTxn(ctx, writeAsOf, func( - ctx context.Context, txn isql.Txn, _ *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { for _, span := range targetSpans { // TODO(dt): a Count() request would be nice here if the target isn't @@ -976,7 +969,7 @@ func (sc *SchemaChanger) distIndexBackfill( } log.Infof(ctx, "persisting target safe write time %v...", writeAsOf) if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, _ *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { details := sc.job.Details().(jobspb.SchemaChangeDetails) details.WriteTimestamp = writeAsOf @@ -1003,7 +996,7 @@ func (sc *SchemaChanger) distIndexBackfill( // The txn is used to fetch a tableDesc, partition the spans and set the // evalCtx ts all of which is during planning of the DistSQL flow. if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { // It is okay to release the lease on the descriptor before running the @@ -1017,12 +1010,12 @@ func (sc *SchemaChanger) distIndexBackfill( // clear what this buys us in terms of checking the descriptors validity. // Thus, in favor of simpler code and no correctness concerns we release // the lease once the flow is planned. - tableDesc, err := sc.getTableVersion(ctx, txn.KV(), descriptors, version) + tableDesc, err := sc.getTableVersion(ctx, txn.KV(), txn.Descriptors(), version) if err != nil { return err } sd := NewFakeSessionData(sc.execCfg.SV()) - evalCtx = createSchemaChangeEvalCtx(ctx, sc.execCfg, sd, txn.KV().ReadTimestamp(), descriptors) + evalCtx = createSchemaChangeEvalCtx(ctx, sc.execCfg, sd, txn.KV().ReadTimestamp(), txn.Descriptors()) planCtx = sc.distSQLPlanner.NewPlanningCtx(ctx, &evalCtx, nil, /* planner */ txn.KV(), DistributionTypeSystemTenantOnly) indexBatchSize := indexBackfillBatchSize.Get(&sc.execCfg.Settings.SV) @@ -1302,10 +1295,10 @@ func (sc *SchemaChanger) distColumnBackfill( // variable and assign to todoSpans after committing. var updatedTodoSpans []roachpb.Span if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { updatedTodoSpans = todoSpans - tableDesc, err := sc.getTableVersion(ctx, txn.KV(), descriptors, version) + tableDesc, err := sc.getTableVersion(ctx, txn.KV(), txn.Descriptors(), version) if err != nil { return err } @@ -1318,7 +1311,7 @@ func (sc *SchemaChanger) distColumnBackfill( } cbw := MetadataCallbackWriter{rowResultWriter: &errOnlyResultWriter{}, fn: metaFn} sd := NewFakeSessionData(sc.execCfg.SV()) - evalCtx := createSchemaChangeEvalCtx(ctx, sc.execCfg, sd, txn.KV().ReadTimestamp(), descriptors) + evalCtx := createSchemaChangeEvalCtx(ctx, sc.execCfg, sd, txn.KV().ReadTimestamp(), txn.Descriptors()) recv := MakeDistSQLReceiver( ctx, &cbw, @@ -1432,9 +1425,9 @@ func (sc *SchemaChanger) validateIndexes(ctx context.Context) error { readAsOf := sc.clock.Now() var tableDesc catalog.TableDescriptor if err := sc.fixedTimestampTxn(ctx, readAsOf, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) (err error) { - tableDesc, err = descriptors.ByID(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID) + tableDesc, err = txn.Descriptors().ByID(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID) return err }); err != nil { return err @@ -1530,14 +1523,14 @@ func ValidateConstraint( // The check operates at the historical timestamp. return runHistoricalTxn.Exec(ctx, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { // Use a schema resolver because we need to resolve types by ID and table by name. - resolver := NewSkippingCacheSchemaResolver(descriptors, sessiondata.NewStack(sessionData), txn.KV(), nil /* authAccessor */) + resolver := NewSkippingCacheSchemaResolver(txn.Descriptors(), sessiondata.NewStack(sessionData), txn.KV(), nil /* authAccessor */) semaCtx := tree.MakeSemaContext() semaCtx.TypeResolver = resolver semaCtx.TableNameResolver = resolver - defer func() { descriptors.ReleaseAll(ctx) }() + defer func() { txn.Descriptors().ReleaseAll(ctx) }() switch catalog.GetConstraintType(constraint) { case catconstants.ConstraintTypeCheck: @@ -1565,7 +1558,7 @@ func ValidateConstraint( ) case catconstants.ConstraintTypeFK: fk := constraint.AsForeignKey() - targetTable, err := descriptors.ByID(txn.KV()).Get().Table(ctx, fk.GetReferencedTableID()) + targetTable, err := txn.Descriptors().ByID(txn.KV()).Get().Table(ctx, fk.GetReferencedTableID()) if err != nil { return err } @@ -1651,7 +1644,7 @@ func ValidateInvertedIndexes( key := span.Key endKey := span.EndKey if err := runHistoricalTxn.Exec(ctx, func( - ctx context.Context, txn isql.Txn, _ *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { for { kvs, err := txn.KV().Scan(ctx, key, endKey, 1000000) @@ -1760,7 +1753,7 @@ func countExpectedRowsForInvertedIndex( var expectedCount int64 if err := runHistoricalTxn.Exec(ctx, func( - ctx context.Context, txn isql.Txn, _ *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { var stmt string geoConfig := idx.GetGeoConfig() @@ -1946,7 +1939,7 @@ func populateExpectedCounts( } var tableRowCount int64 if err := runHistoricalTxn.Exec(ctx, func( - ctx context.Context, txn isql.Txn, _ *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { var s strings.Builder for _, idx := range indexes { @@ -2059,7 +2052,7 @@ func countIndexRowsAndMaybeCheckUniqueness( // Retrieve the row count in the index. var idxLen int64 if err := runHistoricalTxn.Exec(ctx, func( - ctx context.Context, txn isql.Txn, _ *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { query := fmt.Sprintf(`SELECT count(1) FROM [%d AS t]@[%d]`, desc.GetID(), idx.GetID()) // If the index is a partial index the predicate must be added @@ -2266,10 +2259,10 @@ func (sc *SchemaChanger) mergeFromTemporaryIndex( ) error { var tbl *tabledesc.Mutable if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { var err error - tbl, err = descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + tbl, err = txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) return err }); err != nil { return err @@ -2288,9 +2281,9 @@ func (sc *SchemaChanger) mergeFromTemporaryIndex( func (sc *SchemaChanger) runStateMachineAfterTempIndexMerge(ctx context.Context) error { var runStatus jobs.RunningStatus return sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - tbl, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + tbl, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } @@ -2320,7 +2313,7 @@ func (sc *SchemaChanger) runStateMachineAfterTempIndexMerge(ctx context.Context) if runStatus == "" || tbl.Dropped() { return nil } - if err := descsCol.WriteDesc( + if err := txn.Descriptors().WriteDesc( ctx, true /* kvTrace */, tbl, txn.KV(), ); err != nil { return err @@ -2663,11 +2656,7 @@ func validateCheckInTxn( } func getTargetTablesAndFk( - ctx context.Context, - srcTable *tabledesc.Mutable, - txn *kv.Txn, - descsCol *descs.Collection, - fkName string, + ctx context.Context, srcTable *tabledesc.Mutable, txn descs.Txn, fkName string, ) ( syntheticDescs []catalog.Descriptor, fk *descpb.ForeignKeyConstraint, @@ -2688,7 +2677,7 @@ func getTargetTablesAndFk( if fk == nil { return nil, nil, nil, errors.AssertionFailedf("foreign key %s does not exist", fkName) } - targetTable, err = descsCol.ByID(txn).Get().Table(ctx, fk.ReferencedTableID) + targetTable, err = txn.Descriptors().ByID(txn.KV()).Get().Table(ctx, fk.ReferencedTableID) if err != nil { return nil, nil, nil, err } @@ -2714,13 +2703,9 @@ func getTargetTablesAndFk( // It operates entirely on the current goroutine and is thus able to // reuse an existing kv.Txn safely. func validateFkInTxn( - ctx context.Context, - txn isql.Txn, - descsCol *descs.Collection, - srcTable *tabledesc.Mutable, - fkName string, + ctx context.Context, txn descs.Txn, srcTable *tabledesc.Mutable, fkName string, ) error { - syntheticDescs, fk, targetTable, err := getTargetTablesAndFk(ctx, srcTable, txn.KV(), descsCol, fkName) + syntheticDescs, fk, targetTable, err := getTargetTablesAndFk(ctx, srcTable, txn, fkName) if err != nil { return err } diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index a03175bc0950..b1fe4cd26487 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -1206,7 +1206,7 @@ func MakeTestCollection(ctx context.Context, leaseManager *lease.Manager) Collec } // InternalExecFn is the type of functions that operates using an internalExecutor. -type InternalExecFn func(ctx context.Context, txn isql.Txn, descriptors *Collection) error +type InternalExecFn func(ctx context.Context, txn Txn) error // HistoricalInternalExecTxnRunnerFn callback for executing with the internal executor // at a fixed timestamp. diff --git a/pkg/sql/logictest/testdata/logic_test/drop_table b/pkg/sql/logictest/testdata/logic_test/drop_table index 49c8388f13f0..d1a43b524334 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_table +++ b/pkg/sql/logictest/testdata/logic_test/drop_table @@ -114,3 +114,27 @@ COMMIT; statement error pgcode 42P01 relation "to_drop" does not exist DROP TABLE to_drop; + +subtest drop_table_with_not_valid_constraints + +statement ok +CREATE TABLE t_with_not_valid_constraints_1 (i INT PRIMARY KEY, j INT); + +statement ok +CREATE TABLE t_with_not_valid_constraints_2 (i INT PRIMARY KEY, j INT); + +statement ok +ALTER TABLE t_with_not_valid_constraints_1 ADD CHECK (i > 0) NOT VALID; + +statement ok +SET experimental_enable_unique_without_index_constraints = true; +ALTER TABLE t_with_not_valid_constraints_1 ADD UNIQUE WITHOUT INDEX (j) NOT VALID; + +statement ok +ALTER TABLE t_with_not_valid_constraints_1 ADD FOREIGN KEY (i) REFERENCES t_with_not_valid_constraints_2 (i) NOT VALID; + +statement ok +DROP TABLE t_with_not_valid_constraints_1; + +statement ok +DROP TABLE t_with_not_valid_constraints_2; diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 68f5ab736115..2d976eb66699 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -440,8 +440,8 @@ func (sc *SchemaChanger) maybeMakeAddTablePublic( } log.Info(ctx, "making table public") - return sc.txn(ctx, func(ctx context.Context, txn isql.Txn, descsCol *descs.Collection) error { - mut, err := descsCol.MutableByID(txn.KV()).Table(ctx, table.GetID()) + return sc.txn(ctx, func(ctx context.Context, txn descs.Txn) error { + mut, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, table.GetID()) if err != nil { return err } @@ -449,7 +449,7 @@ func (sc *SchemaChanger) maybeMakeAddTablePublic( return nil } mut.State = descpb.DescriptorState_PUBLIC - return descsCol.WriteDesc(ctx, true /* kvTrace */, mut, txn.KV()) + return txn.Descriptors().WriteDesc(ctx, true /* kvTrace */, mut, txn.KV()) }) } @@ -481,8 +481,8 @@ func (sc *SchemaChanger) ignoreRevertedDropIndex( if !table.IsPhysicalTable() { return nil } - return sc.txn(ctx, func(ctx context.Context, txn isql.Txn, descsCol *descs.Collection) error { - mut, err := descsCol.MutableByID(txn.KV()).Table(ctx, table.GetID()) + return sc.txn(ctx, func(ctx context.Context, txn descs.Txn) error { + mut, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, table.GetID()) if err != nil { return err } @@ -500,7 +500,7 @@ func (sc *SchemaChanger) ignoreRevertedDropIndex( mutationsModified = true } if mutationsModified { - return descsCol.WriteDesc(ctx, true /* kvTrace */, mut, txn.KV()) + return txn.Descriptors().WriteDesc(ctx, true /* kvTrace */, mut, txn.KV()) } return nil }) @@ -574,9 +574,9 @@ func (sc *SchemaChanger) getTargetDescriptor(ctx context.Context) (catalog.Descr // Retrieve the descriptor that is being changed. var desc catalog.Descriptor if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descriptors *descs.Collection, + ctx context.Context, txn descs.Txn, ) (err error) { - desc, err = descriptors.ByID(txn.KV()).Get().Desc(ctx, sc.descID) + desc, err = txn.Descriptors().ByID(txn.KV()).Get().Desc(ctx, sc.descID) return err }); err != nil { return nil, err @@ -894,8 +894,8 @@ func (sc *SchemaChanger) handlePermanentSchemaChangeError( // initialize the job running status. func (sc *SchemaChanger) initJobRunningStatus(ctx context.Context) error { - return sc.txn(ctx, func(ctx context.Context, txn isql.Txn, descriptors *descs.Collection) error { - desc, err := descriptors.ByID(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID) + return sc.txn(ctx, func(ctx context.Context, txn descs.Txn) error { + desc, err := txn.Descriptors().ByID(txn.KV()).WithoutNonPublic().Get().Table(ctx, sc.descID) if err != nil { return err } @@ -1034,8 +1034,8 @@ func (sc *SchemaChanger) rollbackSchemaChange(ctx context.Context, err error) er // table was in the ADD state and the schema change failed, then we need to // clean up the descriptor. gcJobID := sc.jobRegistry.MakeJobID() - if err := sc.txn(ctx, func(ctx context.Context, txn isql.Txn, descsCol *descs.Collection) error { - scTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + if err := sc.txn(ctx, func(ctx context.Context, txn descs.Txn) error { + scTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } @@ -1046,17 +1046,17 @@ func (sc *SchemaChanger) rollbackSchemaChange(ctx context.Context, err error) er b := txn.KV().NewBatch() // For views, we need to clean up and references that exist to tables. if scTable.IsView() { - if err := sc.dropViewDeps(ctx, descsCol, txn.KV(), b, scTable); err != nil { + if err := sc.dropViewDeps(ctx, txn.Descriptors(), txn.KV(), b, scTable); err != nil { return err } } scTable.SetDropped() scTable.DropTime = timeutil.Now().UnixNano() const kvTrace = false - if err := descsCol.WriteDescToBatch(ctx, kvTrace, scTable, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, scTable, b); err != nil { return err } - if err := descsCol.DeleteNamespaceEntryToBatch(ctx, kvTrace, scTable, b); err != nil { + if err := txn.Descriptors().DeleteNamespaceEntryToBatch(ctx, kvTrace, scTable, b); err != nil { return err } @@ -1094,13 +1094,13 @@ func (sc *SchemaChanger) RunStateMachineBeforeBackfill(ctx context.Context) erro var runStatus jobs.RunningStatus if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - tbl, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + tbl, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } - dbDesc, err := descsCol.ByID(txn.KV()).WithoutNonPublic().Get().Database(ctx, tbl.GetParentID()) + dbDesc, err := txn.Descriptors().ByID(txn.KV()).WithoutNonPublic().Get().Database(ctx, tbl.GetParentID()) if err != nil { return err } @@ -1143,7 +1143,7 @@ func (sc *SchemaChanger) RunStateMachineBeforeBackfill(ctx context.Context) erro tbl, m, false, // isDone - descsCol, + txn.Descriptors(), ); err != nil { return err } @@ -1151,7 +1151,7 @@ func (sc *SchemaChanger) RunStateMachineBeforeBackfill(ctx context.Context) erro if doNothing := runStatus == "" || tbl.Dropped(); doNothing { return nil } - if err := descsCol.WriteDesc( + if err := txn.Descriptors().WriteDesc( ctx, true /* kvTrace */, tbl, txn.KV(), ); err != nil { return err @@ -1200,9 +1200,9 @@ func (sc *SchemaChanger) stepStateMachineAfterIndexBackfill(ctx context.Context) var runStatus jobs.RunningStatus if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - tbl, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + tbl, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } @@ -1232,7 +1232,7 @@ func (sc *SchemaChanger) stepStateMachineAfterIndexBackfill(ctx context.Context) if runStatus == "" || tbl.Dropped() { return nil } - if err := descsCol.WriteDesc( + if err := txn.Descriptors().WriteDesc( ctx, true /* kvTrace */, tbl, txn.KV(), ); err != nil { return err @@ -1342,24 +1342,24 @@ func (sc *SchemaChanger) done(ctx context.Context) error { var depMutationJobs []jobspb.JobID var otherJobIDs []jobspb.JobID err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { depMutationJobs = depMutationJobs[:0] otherJobIDs = otherJobIDs[:0] var err error - scTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + scTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } - dbDesc, err := descsCol.ByID(txn.KV()).WithoutNonPublic().Get().Database(ctx, scTable.GetParentID()) + dbDesc, err := txn.Descriptors().ByID(txn.KV()).WithoutNonPublic().Get().Database(ctx, scTable.GetParentID()) if err != nil { return err } collectReferencedTypeIDs := func() (catalog.DescriptorIDSet, error) { typeLookupFn := func(id descpb.ID) (catalog.TypeDescriptor, error) { - desc, err := descsCol.ByIDWithLeased(txn.KV()).WithoutNonPublic().Get().Type(ctx, id) + desc, err := txn.Descriptors().ByIDWithLeased(txn.KV()).WithoutNonPublic().Get().Type(ctx, id) if err != nil { return nil, err } @@ -1415,13 +1415,13 @@ func (sc *SchemaChanger) done(ctx context.Context) error { if fk := m.AsForeignKey(); fk != nil && fk.Adding() && fk.GetConstraintValidity() == descpb.ConstraintValidity_Unvalidated { // Add backreference on the referenced table (which could be the same table) - backrefTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) + backrefTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) if err != nil { return err } backrefTable.InboundFKs = append(backrefTable.InboundFKs, *fk.ForeignKeyDesc()) if backrefTable != scTable { - if err := descsCol.WriteDescToBatch(ctx, kvTrace, backrefTable, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, backrefTable, b); err != nil { return err } } @@ -1437,7 +1437,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { scTable, m, true, // isDone - descsCol, + txn.Descriptors(), ); err != nil { return err } @@ -1594,7 +1594,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { } if err := setNewLocalityConfig( - ctx, txn.KV(), descsCol, b, scTable, localityConfigToSwapTo, kvTrace, + ctx, txn.KV(), txn.Descriptors(), b, scTable, localityConfigToSwapTo, kvTrace, ); err != nil { return err } @@ -1690,7 +1690,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { // Update the set of back references. for id, isAddition := range update { - typ, err := descsCol.MutableByID(txn.KV()).Type(ctx, id) + typ, err := txn.Descriptors().MutableByID(txn.KV()).Type(ctx, id) if err != nil { return err } @@ -1699,7 +1699,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { } else { typ.RemoveReferencingDescriptorID(scTable.ID) } - if err := descsCol.WriteDescToBatch(ctx, kvTrace, typ, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, typ, b); err != nil { return err } } @@ -1731,12 +1731,12 @@ func (sc *SchemaChanger) done(ctx context.Context) error { // Update the set of back references. for id, colIDSet := range update { - tbl, err := descsCol.MutableByID(txn.KV()).Table(ctx, id) + tbl, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, id) if err != nil { return err } tbl.UpdateColumnsDependedOnBy(scTable.ID, colIDSet) - if err := descsCol.WriteDescToBatch(ctx, kvTrace, tbl, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, tbl, b); err != nil { return err } } @@ -1745,7 +1745,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { // Clean up any comments related to the mutations, specifically if we need // to drop them. for _, comment := range commentsToDelete { - if err := descsCol.DeleteCommentInBatch( + if err := txn.Descriptors().DeleteCommentInBatch( ctx, false /* kvTrace */, b, catalogkeys.MakeCommentKey(uint32(comment.id), uint32(comment.subID), comment.commentType), ); err != nil { return err @@ -1753,23 +1753,23 @@ func (sc *SchemaChanger) done(ctx context.Context) error { } for _, comment := range commentsToSwap { - cmt, found := descsCol.GetComment(catalogkeys.MakeCommentKey(uint32(comment.id), uint32(comment.oldSubID), comment.commentType)) + cmt, found := txn.Descriptors().GetComment(catalogkeys.MakeCommentKey(uint32(comment.id), uint32(comment.oldSubID), comment.commentType)) if !found { continue } - if err := descsCol.DeleteCommentInBatch( + if err := txn.Descriptors().DeleteCommentInBatch( ctx, false /* kvTrace */, b, catalogkeys.MakeCommentKey(uint32(comment.id), uint32(comment.oldSubID), comment.commentType), ); err != nil { return err } - if err := descsCol.WriteCommentToBatch( + if err := txn.Descriptors().WriteCommentToBatch( ctx, false /* kvTrace */, b, catalogkeys.MakeCommentKey(uint32(comment.id), uint32(comment.newSubID), comment.commentType), cmt, ); err != nil { return err } } - if err := descsCol.WriteDescToBatch(ctx, kvTrace, scTable, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, scTable, b); err != nil { return err } if err := txn.KV().Run(ctx, b); err != nil { @@ -1924,8 +1924,8 @@ func (sc *SchemaChanger) maybeReverseMutations(ctx context.Context, causingError // Get the other tables whose foreign key backreferences need to be removed. alreadyReversed := false const kvTrace = true // TODO(ajwerner): figure this out - err := sc.txn(ctx, func(ctx context.Context, txn isql.Txn, descsCol *descs.Collection) error { - scTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + err := sc.txn(ctx, func(ctx context.Context, txn descs.Txn) error { + scTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } @@ -1998,7 +1998,7 @@ func (sc *SchemaChanger) maybeReverseMutations(ctx context.Context, causingError } // Get the foreign key backreferences to remove. if fk := constraint.AsForeignKey(); fk != nil { - backrefTable, err := descsCol.MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) + backrefTable, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, fk.GetReferencedTableID()) if err != nil { return err } @@ -2009,7 +2009,7 @@ func (sc *SchemaChanger) maybeReverseMutations(ctx context.Context, causingError log.Infof(ctx, "error attempting to remove backreference %s during rollback: %s", constraint.GetName(), err) } - if err := descsCol.WriteDescToBatch(ctx, kvTrace, backrefTable, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, backrefTable, b); err != nil { return err } } @@ -2030,7 +2030,7 @@ func (sc *SchemaChanger) maybeReverseMutations(ctx context.Context, causingError // Read the table descriptor from the store. The Version of the // descriptor has already been incremented in the transaction and // this descriptor can be modified without incrementing the version. - if err := descsCol.WriteDescToBatch(ctx, kvTrace, scTable, b); err != nil { + if err := txn.Descriptors().WriteDescToBatch(ctx, kvTrace, scTable, b); err != nil { return err } if err := txn.KV().Run(ctx, b); err != nil { @@ -2447,27 +2447,7 @@ type SchemaChangerTestingKnobs struct { func (*SchemaChangerTestingKnobs) ModuleTestingKnobs() {} // txn is a convenient wrapper around descs.Txn(). -// -// TODO(ajwerner): Replace this with direct calls to DescsTxn. -func (sc *SchemaChanger) txn( - ctx context.Context, f func(context.Context, isql.Txn, *descs.Collection) error, -) error { - return sc.txnWithExecutor(ctx, func( - ctx context.Context, txn isql.Txn, _ *sessiondata.SessionData, - collection *descs.Collection, - ) error { - return f(ctx, txn, collection) - }) -} - -// txnWithExecutor is to run internal executor within a txn. -func (sc *SchemaChanger) txnWithExecutor( - ctx context.Context, - f func( - context.Context, isql.Txn, *sessiondata.SessionData, - *descs.Collection, - ) error, -) error { +func (sc *SchemaChanger) txn(ctx context.Context, f func(context.Context, descs.Txn) error) error { if fn := sc.testingKnobs.RunBeforeDescTxn; fn != nil { if err := fn(sc.job.ID()); err != nil { return err @@ -2476,7 +2456,7 @@ func (sc *SchemaChanger) txnWithExecutor( return sc.execCfg.InternalDB.DescsTxn(ctx, func( ctx context.Context, txn descs.Txn, ) error { - return f(ctx, txn, txn.SessionData(), txn.Descriptors()) + return f(ctx, txn) }) } @@ -3085,9 +3065,9 @@ func (sc *SchemaChanger) getDependentMutationsJobs( func (sc *SchemaChanger) preSplitHashShardedIndexRanges(ctx context.Context) error { if err := sc.txn(ctx, func( - ctx context.Context, txn isql.Txn, descsCol *descs.Collection, + ctx context.Context, txn descs.Txn, ) error { - tableDesc, err := descsCol.MutableByID(txn.KV()).Table(ctx, sc.descID) + tableDesc, err := txn.Descriptors().MutableByID(txn.KV()).Table(ctx, sc.descID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scbuild/builder_test.go b/pkg/sql/schemachanger/scbuild/builder_test.go index 6812504ff741..a07e96057991 100644 --- a/pkg/sql/schemachanger/scbuild/builder_test.go +++ b/pkg/sql/schemachanger/scbuild/builder_test.go @@ -93,6 +93,7 @@ func TestBuildDataDriven(t *testing.T) { // changer will allow non-fully implemented operations. sd.NewSchemaChangerMode = sessiondatapb.UseNewSchemaChangerUnsafe sd.ApplicationName = "" + sd.EnableUniqueWithoutIndexConstraints = true }, ), ), diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go index bab311fe92f8..8dd1b4726c91 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go @@ -52,20 +52,21 @@ var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{ reflect.TypeOf((*tree.AlterTableAddConstraint)(nil)): {fn: alterTableAddConstraint, on: true, extraChecks: func( t *tree.AlterTableAddConstraint, ) bool { - // Support ALTER TABLE ... ADD PRIMARY KEY if d, ok := t.ConstraintDef.(*tree.UniqueConstraintTableDef); ok && d.PrimaryKey && t.ValidationBehavior == tree.ValidationDefault { + // Support ALTER TABLE ... ADD PRIMARY KEY return true - } else if ok && d.WithoutIndex && t.ValidationBehavior == tree.ValidationDefault { + } else if ok && d.WithoutIndex { + // Support ALTER TABLE ... ADD UNIQUE WITHOUT INDEX [NOT VALID] return true } - // Support ALTER TABLE ... ADD CONSTRAINT CHECK - if _, ok := t.ConstraintDef.(*tree.CheckConstraintTableDef); ok && t.ValidationBehavior == tree.ValidationDefault { + // Support ALTER TABLE ... ADD CONSTRAINT CHECK [NOT VALID] + if _, ok := t.ConstraintDef.(*tree.CheckConstraintTableDef); ok { return true } - // Support ALTER TABLE ... ADD CONSTRAINT FOREIGN KEY - if _, ok := t.ConstraintDef.(*tree.ForeignKeyConstraintTableDef); ok && t.ValidationBehavior == tree.ValidationDefault { + // Support ALTER TABLE ... ADD CONSTRAINT FOREIGN KEY [NOT VALID] + if _, ok := t.ConstraintDef.(*tree.ForeignKeyConstraintTableDef); ok { return true } @@ -83,6 +84,9 @@ var alterTableAddConstraintMinSupportedClusterVersion = map[string]clusterversio "ADD_CHECK_DEFAULT": clusterversion.V23_1Start, "ADD_FOREIGN_KEY_DEFAULT": clusterversion.V23_1Start, "ADD_UNIQUE_WITHOUT_INDEX_DEFAULT": clusterversion.V23_1Start, + "ADD_CHECK_SKIP": clusterversion.V23_1, + "ADD_UNIQUE_WITHOUT_INDEX_SKIP": clusterversion.V23_1, + "ADD_FOREIGN_KEY_SKIP": clusterversion.V23_1, } func init() { @@ -186,7 +190,10 @@ func alterTableAddConstraintSupportedInCurrentClusterVersion( cmdKey += "_SKIP" } - minSupportedClusterVersion := alterTableAddConstraintMinSupportedClusterVersion[cmdKey] + minSupportedClusterVersion, ok := alterTableAddConstraintMinSupportedClusterVersion[cmdKey] + if !ok { + return false + } return b.EvalCtx().Settings.Version.IsActive(b, minSupportedClusterVersion) } diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go index df72176612d5..e139d46b85ed 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go @@ -41,17 +41,13 @@ func alterTableAddConstraint( case *tree.UniqueConstraintTableDef: if d.PrimaryKey && t.ValidationBehavior == tree.ValidationDefault { alterTableAddPrimaryKey(b, tn, tbl, t) - } else if d.WithoutIndex && t.ValidationBehavior == tree.ValidationDefault { + } else if d.WithoutIndex { alterTableAddUniqueWithoutIndex(b, tn, tbl, t) } case *tree.CheckConstraintTableDef: - if t.ValidationBehavior == tree.ValidationDefault { - alterTableAddCheck(b, tn, tbl, t) - } + alterTableAddCheck(b, tn, tbl, t) case *tree.ForeignKeyConstraintTableDef: - if t.ValidationBehavior == tree.ValidationDefault { - alterTableAddForeignKey(b, tn, tbl, t) - } + alterTableAddForeignKey(b, tn, tbl, t) } } @@ -80,7 +76,7 @@ func alterTableAddPrimaryKey( } // alterTableAddCheck contains logic for building -// `ALTER TABLE ... ADD CONSTRAINT ... CHECK`. +// `ALTER TABLE ... ADD CHECK ... [NOT VALID]`. // It assumes `t` is such a command. func alterTableAddCheck( b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableAddConstraint, @@ -113,18 +109,31 @@ func alterTableAddCheck( panic(err) } - // 3. Add relevant check constraint element: CheckConstraint and ConstraintName. + // 3. Add relevant check constraint element: + // - CheckConstraint or CheckConstraintUnvalidated + // - ConstraintName constraintID := b.NextTableConstraintID(tbl.TableID) - ck := &scpb.CheckConstraint{ - TableID: tbl.TableID, - ConstraintID: constraintID, - ColumnIDs: colIDs.Ordered(), - Expression: *b.WrapExpression(tbl.TableID, typedCkExpr), - FromHashShardedColumn: ckDef.FromHashShardedColumn, - IndexIDForValidation: getIndexIDForValidationForConstraint(b, tbl.TableID), + if t.ValidationBehavior == tree.ValidationDefault { + ck := &scpb.CheckConstraint{ + TableID: tbl.TableID, + ConstraintID: constraintID, + ColumnIDs: colIDs.Ordered(), + Expression: *b.WrapExpression(tbl.TableID, typedCkExpr), + FromHashShardedColumn: ckDef.FromHashShardedColumn, + IndexIDForValidation: getIndexIDForValidationForConstraint(b, tbl.TableID), + } + b.Add(ck) + b.LogEventForExistingTarget(ck) + } else { + ck := &scpb.CheckConstraintUnvalidated{ + TableID: tbl.TableID, + ConstraintID: constraintID, + ColumnIDs: colIDs.Ordered(), + Expression: *b.WrapExpression(tbl.TableID, typedCkExpr), + } + b.Add(ck) + b.LogEventForExistingTarget(ck) } - b.Add(ck) - b.LogEventForExistingTarget(ck) constraintName := string(ckDef.Name) if constraintName == "" { @@ -162,7 +171,7 @@ func getIndexIDForValidationForConstraint(b BuildCtx, tableID catid.DescID) (ret } // alterTableAddForeignKey contains logic for building -// `ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY`. +// `ALTER TABLE ... ADD FOREIGN KEY ... [NOT VALID]`. // It assumes `t` is such a command. func alterTableAddForeignKey( b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableAddConstraint, @@ -358,19 +367,34 @@ func alterTableAddForeignKey( // 12. (Finally!) Add a ForeignKey_Constraint, ConstraintName element to // builder state. constraintID := b.NextTableConstraintID(tbl.TableID) - fk := &scpb.ForeignKeyConstraint{ - TableID: tbl.TableID, - ConstraintID: constraintID, - ColumnIDs: originColIDs, - ReferencedTableID: referencedTableID, - ReferencedColumnIDs: referencedColIDs, - OnUpdateAction: tree.ForeignKeyReferenceActionValue[fkDef.Actions.Update], - OnDeleteAction: tree.ForeignKeyReferenceActionValue[fkDef.Actions.Delete], - CompositeKeyMatchMethod: tree.CompositeKeyMatchMethodValue[fkDef.Match], - IndexIDForValidation: getIndexIDForValidationForConstraint(b, tbl.TableID), - } - b.Add(fk) - b.LogEventForExistingTarget(fk) + if t.ValidationBehavior == tree.ValidationDefault { + fk := &scpb.ForeignKeyConstraint{ + TableID: tbl.TableID, + ConstraintID: constraintID, + ColumnIDs: originColIDs, + ReferencedTableID: referencedTableID, + ReferencedColumnIDs: referencedColIDs, + OnUpdateAction: tree.ForeignKeyReferenceActionValue[fkDef.Actions.Update], + OnDeleteAction: tree.ForeignKeyReferenceActionValue[fkDef.Actions.Delete], + CompositeKeyMatchMethod: tree.CompositeKeyMatchMethodValue[fkDef.Match], + IndexIDForValidation: getIndexIDForValidationForConstraint(b, tbl.TableID), + } + b.Add(fk) + b.LogEventForExistingTarget(fk) + } else { + fk := &scpb.ForeignKeyConstraintUnvalidated{ + TableID: tbl.TableID, + ConstraintID: constraintID, + ColumnIDs: originColIDs, + ReferencedTableID: referencedTableID, + ReferencedColumnIDs: referencedColIDs, + OnUpdateAction: tree.ForeignKeyReferenceActionValue[fkDef.Actions.Update], + OnDeleteAction: tree.ForeignKeyReferenceActionValue[fkDef.Actions.Delete], + CompositeKeyMatchMethod: tree.CompositeKeyMatchMethodValue[fkDef.Match], + } + b.Add(fk) + b.LogEventForExistingTarget(fk) + } b.Add(&scpb.ConstraintWithoutIndexName{ TableID: tbl.TableID, ConstraintID: constraintID, @@ -378,7 +402,8 @@ func alterTableAddForeignKey( }) } -// alterTableAddUniqueWithoutIndex contains logic for building ALTER TABLE ... ADD CONSTRAINT ... UNIQUE WITHOUT INDEX. +// alterTableAddUniqueWithoutIndex contains logic for building +// `ALTER TABLE ... ADD UNIQUE WITHOUT INDEX ... [NOT VALID]`. // It assumes `t` is such a command. func alterTableAddUniqueWithoutIndex( b BuildCtx, tn *tree.TableName, tbl *scpb.Table, t *tree.AlterTableAddConstraint, @@ -462,17 +487,30 @@ func alterTableAddUniqueWithoutIndex( // 5. (Finally!) Add a UniqueWithoutIndex, ConstraintName element to builder state. constraintID := b.NextTableConstraintID(tbl.TableID) - uwi := &scpb.UniqueWithoutIndexConstraint{ - TableID: tbl.TableID, - ConstraintID: constraintID, - ColumnIDs: colIDs, - IndexIDForValidation: getIndexIDForValidationForConstraint(b, tbl.TableID), - } - if d.Predicate != nil { - uwi.Predicate = b.WrapExpression(tbl.TableID, d.Predicate) + if t.ValidationBehavior == tree.ValidationDefault { + uwi := &scpb.UniqueWithoutIndexConstraint{ + TableID: tbl.TableID, + ConstraintID: constraintID, + ColumnIDs: colIDs, + IndexIDForValidation: getIndexIDForValidationForConstraint(b, tbl.TableID), + } + if d.Predicate != nil { + uwi.Predicate = b.WrapExpression(tbl.TableID, d.Predicate) + } + b.Add(uwi) + b.LogEventForExistingTarget(uwi) + } else { + uwi := &scpb.UniqueWithoutIndexConstraintUnvalidated{ + TableID: tbl.TableID, + ConstraintID: constraintID, + ColumnIDs: colIDs, + } + if d.Predicate != nil { + uwi.Predicate = b.WrapExpression(tbl.TableID, d.Predicate) + } + b.Add(uwi) + b.LogEventForExistingTarget(uwi) } - b.Add(uwi) - b.LogEventForExistingTarget(uwi) b.Add(&scpb.ConstraintWithoutIndexName{ TableID: tbl.TableID, ConstraintID: constraintID, diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go index f92baae14ecc..b97abd8cf879 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/process.go @@ -197,5 +197,4 @@ func stmtSupportedInCurrentClusterVersion(b BuildCtx, n tree.Statement) bool { } minSupportedClusterVersion := supportedStatements[reflect.TypeOf(n)].minSupportedClusterVersion return b.EvalCtx().Settings.Version.IsActive(b, minSupportedClusterVersion) - } diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check index 8d931cb189ae..3cd0f25f3e65 100644 --- a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check +++ b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check @@ -4,7 +4,6 @@ CREATE TABLE t (i INT PRIMARY KEY) build ALTER TABLE t ADD CHECK (i > 0) ---- ---- - [[CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2}, PUBLIC], ABSENT] {columnIds: [1], constraintId: 2, expr: 'i > 0:::INT8', referencedColumnIds: [1], tableId: 104} diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check_unvalidated b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check_unvalidated new file mode 100644 index 000000000000..ac291db4324f --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_check_unvalidated @@ -0,0 +1,11 @@ +setup +CREATE TABLE t (i INT PRIMARY KEY) +---- + +build +ALTER TABLE t ADD CHECK (i > 0) NOT VALID +---- +- [[CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2}, PUBLIC], ABSENT] + {columnIds: [1], constraintId: 2, expr: 'i > 0:::INT8', referencedColumnIds: [1], tableId: 104} +- [[ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2}, PUBLIC], ABSENT] + {constraintId: 2, name: check_i, tableId: 104} diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_foreign_key b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_foreign_key new file mode 100644 index 000000000000..aa77fd74a243 --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_foreign_key @@ -0,0 +1,12 @@ +setup +CREATE TABLE t2 (i INT PRIMARY KEY); +CREATE TABLE t1 (i INT PRIMARY KEY); +---- + +build +ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i); +---- +- [[ForeignKeyConstraint:{DescID: 105, IndexID: 0, ConstraintID: 2, ReferencedDescID: 104}, PUBLIC], ABSENT] + {columnIds: [1], constraintId: 2, referencedColumnIds: [1], referencedTableId: 104, tableId: 105} +- [[ConstraintWithoutIndexName:{DescID: 105, Name: t1_i_fkey, ConstraintID: 2}, PUBLIC], ABSENT] + {constraintId: 2, name: t1_i_fkey, tableId: 105} diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_foreign_key_unvalidated b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_foreign_key_unvalidated new file mode 100644 index 000000000000..27171d7ef64a --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_foreign_key_unvalidated @@ -0,0 +1,12 @@ +setup +CREATE TABLE t2 (i INT PRIMARY KEY); +CREATE TABLE t1 (i INT PRIMARY KEY); +---- + +build +ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i) NOT VALID; +---- +- [[ForeignKeyConstraintUnvalidated:{DescID: 105, ConstraintID: 2, ReferencedDescID: 104}, PUBLIC], ABSENT] + {columnIds: [1], constraintId: 2, referencedColumnIds: [1], referencedTableId: 104, tableId: 105} +- [[ConstraintWithoutIndexName:{DescID: 105, Name: t1_i_fkey, ConstraintID: 2}, PUBLIC], ABSENT] + {constraintId: 2, name: t1_i_fkey, tableId: 105} diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_unique_without_index b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_unique_without_index new file mode 100644 index 000000000000..3d65dfaf783e --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_unique_without_index @@ -0,0 +1,11 @@ +setup +CREATE TABLE t (i INT PRIMARY KEY, j INT); +---- + +build +ALTER TABLE t ADD UNIQUE WITHOUT INDEX (j); +---- +- [[UniqueWithoutIndexConstraint:{DescID: 104, ConstraintID: 2}, PUBLIC], ABSENT] + {columnIds: [2], constraintId: 2, tableId: 104} +- [[ConstraintWithoutIndexName:{DescID: 104, Name: unique_j, ConstraintID: 2}, PUBLIC], ABSENT] + {constraintId: 2, name: unique_j, tableId: 104} diff --git a/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_unique_without_index_unvalidated b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_unique_without_index_unvalidated new file mode 100644 index 000000000000..b9352bd40e4a --- /dev/null +++ b/pkg/sql/schemachanger/scbuild/testdata/alter_table_add_unique_without_index_unvalidated @@ -0,0 +1,11 @@ +setup +CREATE TABLE t (i INT PRIMARY KEY, j INT); +---- + +build +ALTER TABLE t ADD UNIQUE WITHOUT INDEX (j) NOT VALID; +---- +- [[UniqueWithoutIndexConstraintUnvalidated:{DescID: 104, ConstraintID: 2}, PUBLIC], ABSENT] + {columnIds: [2], constraintId: 2, tableId: 104} +- [[ConstraintWithoutIndexName:{DescID: 104, Name: unique_j, ConstraintID: 2}, PUBLIC], ABSENT] + {constraintId: 2, name: unique_j, tableId: 104} diff --git a/pkg/sql/schemachanger/scdecomp/decomp.go b/pkg/sql/schemachanger/scdecomp/decomp.go index bf1e539817c7..c49519af15f7 100644 --- a/pkg/sql/schemachanger/scdecomp/decomp.go +++ b/pkg/sql/schemachanger/scdecomp/decomp.go @@ -601,20 +601,32 @@ func (w *walkCtx) walkIndex(tbl catalog.TableDescriptor, idx catalog.Index) { func (w *walkCtx) walkUniqueWithoutIndexConstraint( tbl catalog.TableDescriptor, c catalog.UniqueWithoutIndexConstraint, ) { - uwi := &scpb.UniqueWithoutIndexConstraint{ - TableID: tbl.GetID(), - ConstraintID: c.GetConstraintID(), - ColumnIDs: c.CollectKeyColumnIDs().Ordered(), - } + var expr *scpb.Expression + var err error if c.IsPartial() { - expr, err := w.newExpression(c.GetPredicate()) + expr, err = w.newExpression(c.GetPredicate()) if err != nil { panic(errors.NewAssertionErrorWithWrappedErrf(err, "unique without index constraint %q in table %q (%d)", c.GetName(), tbl.GetName(), tbl.GetID())) } - uwi.Predicate = expr } - w.ev(scpb.Status_PUBLIC, uwi) + if c.IsConstraintUnvalidated() && w.clusterVersion.IsActive(clusterversion.V23_1) { + uwi := &scpb.UniqueWithoutIndexConstraintUnvalidated{ + TableID: tbl.GetID(), + ConstraintID: c.GetConstraintID(), + ColumnIDs: c.CollectKeyColumnIDs().Ordered(), + Predicate: expr, + } + w.ev(scpb.Status_PUBLIC, uwi) + } else { + uwi := &scpb.UniqueWithoutIndexConstraint{ + TableID: tbl.GetID(), + ConstraintID: c.GetConstraintID(), + ColumnIDs: c.CollectKeyColumnIDs().Ordered(), + Predicate: expr, + } + w.ev(scpb.Status_PUBLIC, uwi) + } w.ev(scpb.Status_PUBLIC, &scpb.ConstraintWithoutIndexName{ TableID: tbl.GetID(), ConstraintID: c.GetConstraintID(), @@ -635,14 +647,22 @@ func (w *walkCtx) walkCheckConstraint(tbl catalog.TableDescriptor, c catalog.Che panic(errors.NewAssertionErrorWithWrappedErrf(err, "check constraint %q in table %q (%d)", c.GetName(), tbl.GetName(), tbl.GetID())) } - // TODO(postamar): proper handling of constraint status - w.ev(scpb.Status_PUBLIC, &scpb.CheckConstraint{ - TableID: tbl.GetID(), - ConstraintID: c.GetConstraintID(), - ColumnIDs: c.CollectReferencedColumnIDs().Ordered(), - Expression: *expr, - FromHashShardedColumn: c.IsHashShardingConstraint(), - }) + if c.IsConstraintUnvalidated() && w.clusterVersion.IsActive(clusterversion.V23_1) { + w.ev(scpb.Status_PUBLIC, &scpb.CheckConstraintUnvalidated{ + TableID: tbl.GetID(), + ConstraintID: c.GetConstraintID(), + ColumnIDs: c.CollectReferencedColumnIDs().Ordered(), + Expression: *expr, + }) + } else { + w.ev(scpb.Status_PUBLIC, &scpb.CheckConstraint{ + TableID: tbl.GetID(), + ConstraintID: c.GetConstraintID(), + ColumnIDs: c.CollectReferencedColumnIDs().Ordered(), + Expression: *expr, + FromHashShardedColumn: c.IsHashShardingConstraint(), + }) + } w.ev(scpb.Status_PUBLIC, &scpb.ConstraintWithoutIndexName{ TableID: tbl.GetID(), ConstraintID: c.GetConstraintID(), @@ -660,17 +680,29 @@ func (w *walkCtx) walkCheckConstraint(tbl catalog.TableDescriptor, c catalog.Che func (w *walkCtx) walkForeignKeyConstraint( tbl catalog.TableDescriptor, c catalog.ForeignKeyConstraint, ) { - // TODO(postamar): proper handling of constraint status - w.ev(scpb.Status_PUBLIC, &scpb.ForeignKeyConstraint{ - TableID: tbl.GetID(), - ConstraintID: c.GetConstraintID(), - ColumnIDs: c.ForeignKeyDesc().OriginColumnIDs, - ReferencedTableID: c.GetReferencedTableID(), - ReferencedColumnIDs: c.ForeignKeyDesc().ReferencedColumnIDs, - OnUpdateAction: c.OnUpdate(), - OnDeleteAction: c.OnDelete(), - CompositeKeyMatchMethod: c.Match(), - }) + if c.IsConstraintUnvalidated() && w.clusterVersion.IsActive(clusterversion.V23_1) { + w.ev(scpb.Status_PUBLIC, &scpb.ForeignKeyConstraintUnvalidated{ + TableID: tbl.GetID(), + ConstraintID: c.GetConstraintID(), + ColumnIDs: c.ForeignKeyDesc().OriginColumnIDs, + ReferencedTableID: c.GetReferencedTableID(), + ReferencedColumnIDs: c.ForeignKeyDesc().ReferencedColumnIDs, + OnUpdateAction: c.OnUpdate(), + OnDeleteAction: c.OnDelete(), + CompositeKeyMatchMethod: c.Match(), + }) + } else { + w.ev(scpb.Status_PUBLIC, &scpb.ForeignKeyConstraint{ + TableID: tbl.GetID(), + ConstraintID: c.GetConstraintID(), + ColumnIDs: c.ForeignKeyDesc().OriginColumnIDs, + ReferencedTableID: c.GetReferencedTableID(), + ReferencedColumnIDs: c.ForeignKeyDesc().ReferencedColumnIDs, + OnUpdateAction: c.OnUpdate(), + OnDeleteAction: c.OnDelete(), + CompositeKeyMatchMethod: c.Match(), + }) + } w.ev(scpb.Status_PUBLIC, &scpb.ConstraintWithoutIndexName{ TableID: tbl.GetID(), ConstraintID: c.GetConstraintID(), diff --git a/pkg/sql/schemachanger/scdecomp/testdata/table b/pkg/sql/schemachanger/scdecomp/testdata/table index 797f583a947b..d811c9994f6d 100644 --- a/pkg/sql/schemachanger/scdecomp/testdata/table +++ b/pkg/sql/schemachanger/scdecomp/testdata/table @@ -1,11 +1,13 @@ setup +SET experimental_enable_unique_without_index_constraints = true; CREATE TABLE parent (id INT PRIMARY KEY); CREATE TABLE tbl ( id INT PRIMARY KEY, name STRING NOT NULL, price DECIMAL(8,2), INDEX sec (name) STORING (price) WHERE (id > 0), - CONSTRAINT myfk FOREIGN KEY (id) REFERENCES parent (id) + CONSTRAINT myfk FOREIGN KEY (id) REFERENCES parent (id), + CONSTRAINT mycheck1 CHECK (id > 0) ); COMMENT ON TABLE tbl IS 'tbl is good table'; COMMENT ON INDEX tbl@tbl_pkey IS 'tbl_pkey is a primary key'; @@ -13,6 +15,10 @@ COMMENT ON COLUMN tbl.id IS 'id is a identifier'; COMMENT ON CONSTRAINT myfk ON tbl IS 'must have a parent'; ALTER TABLE tbl CONFIGURE ZONE USING gc.ttlseconds=10; COMMENT ON CONSTRAINT tbl_pkey ON tbl IS 'primary key'; +ALTER TABLE tbl ADD CONSTRAINT mycheck2 CHECK (id < 10) NOT VALID; +ALTER TABLE tbl ADD CONSTRAINT myuwi1 UNIQUE WITHOUT INDEX (price); +ALTER TABLE tbl ADD CONSTRAINT myuwi2 UNIQUE WITHOUT INDEX (price) NOT VALID; +ALTER TABLE tbl ADD CONSTRAINT myfk2 FOREIGN KEY (id) REFERENCES parent (id) NOT VALID; ---- decompose @@ -317,6 +323,45 @@ ElementState: tableId: 105 temporaryIndexId: 0 Status: PUBLIC +- UniqueWithoutIndexConstraint: + columnIds: + - 3 + constraintId: 5 + indexIdForValidation: 0 + predicate: null + tableId: 105 + Status: PUBLIC +- UniqueWithoutIndexConstraintUnvalidated: + columnIds: + - 3 + constraintId: 6 + predicate: null + tableId: 105 + Status: PUBLIC +- CheckConstraint: + columnIds: + - 1 + constraintId: 3 + expr: id > 0:::INT8 + fromHashShardedColumn: false + indexIdForValidation: 0 + referencedColumnIds: + - 1 + tableId: 105 + usesSequenceIds: [] + usesTypeIds: [] + Status: PUBLIC +- CheckConstraintUnvalidated: + columnIds: + - 1 + constraintId: 4 + expr: id < 10:::INT8 + referencedColumnIds: + - 1 + tableId: 105 + usesSequenceIds: [] + usesTypeIds: [] + Status: PUBLIC - ForeignKeyConstraint: columnIds: - 1 @@ -330,6 +375,18 @@ ElementState: referencedTableId: 104 tableId: 105 Status: PUBLIC +- ForeignKeyConstraintUnvalidated: + columnIds: + - 1 + compositeKeyMatchMethod: SIMPLE + constraintId: 7 + onDeleteAction: NO_ACTION + onUpdateAction: NO_ACTION + referencedColumnIds: + - 1 + referencedTableId: 104 + tableId: 105 + Status: PUBLIC - TableComment: comment: tbl is good table tableId: 105 @@ -614,6 +671,31 @@ ElementState: name: myfk tableId: 105 Status: PUBLIC +- ConstraintWithoutIndexName: + constraintId: 3 + name: mycheck1 + tableId: 105 + Status: PUBLIC +- ConstraintWithoutIndexName: + constraintId: 4 + name: mycheck2 + tableId: 105 + Status: PUBLIC +- ConstraintWithoutIndexName: + constraintId: 5 + name: myuwi1 + tableId: 105 + Status: PUBLIC +- ConstraintWithoutIndexName: + constraintId: 6 + name: myuwi2 + tableId: 105 + Status: PUBLIC +- ConstraintWithoutIndexName: + constraintId: 7 + name: myfk2 + tableId: 105 + Status: PUBLIC - ConstraintComment: comment: must have a parent constraintId: 2 diff --git a/pkg/sql/schemachanger/scdeps/sctestutils/sctestutils.go b/pkg/sql/schemachanger/scdeps/sctestutils/sctestutils.go index faf2a0becdd6..7b59944ccbb7 100644 --- a/pkg/sql/schemachanger/scdeps/sctestutils/sctestutils.go +++ b/pkg/sql/schemachanger/scdeps/sctestutils/sctestutils.go @@ -79,6 +79,7 @@ func WithBuilderDependenciesFromTestServer( // For setting up a builder inside tests we will ensure that the new schema // changer will allow non-fully implemented operations. planner.SessionData().NewSchemaChangerMode = sessiondatapb.UseNewSchemaChangerUnsafe + planner.SessionData().EnableUniqueWithoutIndexConstraints = true fn(scdeps.NewBuilderDependencies( execCfg.NodeInfo.LogicalClusterID(), execCfg.Codec, diff --git a/pkg/sql/schemachanger/scdeps/validator.go b/pkg/sql/schemachanger/scdeps/validator.go index 8c6009b2dc83..e63ac296a671 100644 --- a/pkg/sql/schemachanger/scdeps/validator.go +++ b/pkg/sql/schemachanger/scdeps/validator.go @@ -136,7 +136,7 @@ func (vd validator) makeHistoricalInternalExecTxnRunner() descs.HistoricalIntern if err := txn.KV().SetFixedTimestamp(ctx, now); err != nil { return err } - return fn(ctx, txn, txn.Descriptors()) + return fn(ctx, txn) }) }) } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go b/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go index c616dbc55e70..7e7d01b1cbbf 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go @@ -56,8 +56,8 @@ func (i *immediateVisitor) SetConstraintName(ctx context.Context, op scop.SetCon return nil } -func (i *immediateVisitor) MakeAbsentCheckConstraintWriteOnly( - ctx context.Context, op scop.MakeAbsentCheckConstraintWriteOnly, +func (i *immediateVisitor) AddCheckConstraint( + ctx context.Context, op scop.AddCheckConstraint, ) error { tbl, err := i.checkOutTable(ctx, op.TableID) if err != nil || tbl.Dropped() { @@ -67,21 +67,20 @@ func (i *immediateVisitor) MakeAbsentCheckConstraintWriteOnly( tbl.NextConstraintID = op.ConstraintID + 1 } - // We should have already validated that the check constraint - // is syntactically valid in the builder, so we just need to - // enqueue it to the descriptor's mutation slice. ck := &descpb.TableDescriptor_CheckConstraint{ Expr: string(op.CheckExpr), Name: tabledesc.ConstraintNamePlaceholder(op.ConstraintID), - Validity: descpb.ConstraintValidity_Validating, + Validity: op.Validity, ColumnIDs: op.ColumnIDs, FromHashShardedColumn: op.FromHashShardedColumn, ConstraintID: op.ConstraintID, } - enqueueNonIndexMutation(tbl, tbl.AddCheckMutation, ck, descpb.DescriptorMutation_ADD) - // Fast-forward the mutation state to WRITE_ONLY because this constraint - // is now considered as enforced. - tbl.Mutations[len(tbl.Mutations)-1].State = descpb.DescriptorMutation_WRITE_ONLY + if op.Validity == descpb.ConstraintValidity_Validating { + // A validating constraint needs to transition through an intermediate state + // so we enqueue a mutation for it and fast-forward it to WRITE_ONLY state. + enqueueNonIndexMutation(tbl, tbl.AddCheckMutation, ck, descpb.DescriptorMutation_ADD) + tbl.Mutations[len(tbl.Mutations)-1].State = descpb.DescriptorMutation_WRITE_ONLY + } tbl.Checks = append(tbl.Checks, ck) return nil } @@ -368,8 +367,8 @@ func (i *immediateVisitor) RemoveUniqueWithoutIndexConstraint( return nil } -func (i *immediateVisitor) MakeAbsentForeignKeyConstraintWriteOnly( - ctx context.Context, op scop.MakeAbsentForeignKeyConstraintWriteOnly, +func (i *immediateVisitor) AddForeignKeyConstraint( + ctx context.Context, op scop.AddForeignKeyConstraint, ) error { out, err := i.checkOutTable(ctx, op.TableID) if err != nil || out.Dropped() { @@ -379,21 +378,27 @@ func (i *immediateVisitor) MakeAbsentForeignKeyConstraintWriteOnly( out.NextConstraintID = op.ConstraintID + 1 } - // Enqueue a mutation in `out` to signal this mutation is now enforced. fk := &descpb.ForeignKeyConstraint{ OriginTableID: op.TableID, OriginColumnIDs: op.ColumnIDs, ReferencedColumnIDs: op.ReferencedColumnIDs, ReferencedTableID: op.ReferencedTableID, Name: tabledesc.ConstraintNamePlaceholder(op.ConstraintID), - Validity: descpb.ConstraintValidity_Validating, + Validity: op.Validity, OnDelete: op.OnDeleteAction, OnUpdate: op.OnUpdateAction, Match: op.CompositeKeyMatchMethod, ConstraintID: op.ConstraintID, } - enqueueNonIndexMutation(out, out.AddForeignKeyMutation, fk, descpb.DescriptorMutation_ADD) - out.Mutations[len(out.Mutations)-1].State = descpb.DescriptorMutation_WRITE_ONLY + if op.Validity == descpb.ConstraintValidity_Unvalidated { + // Unvalidated constraint doesn't need to transition through an intermediate + // state, so we don't enqueue a mutation for it. + out.OutboundFKs = append(out.OutboundFKs, *fk) + } else { + enqueueNonIndexMutation(out, out.AddForeignKeyMutation, fk, descpb.DescriptorMutation_ADD) + out.Mutations[len(out.Mutations)-1].State = descpb.DescriptorMutation_WRITE_ONLY + } + // Add an entry in "InboundFKs" in the referenced table as company. in, err := i.checkOutTable(ctx, op.ReferencedTableID) if err != nil { @@ -455,14 +460,16 @@ func (i *immediateVisitor) MakePublicForeignKeyConstraintValidated( ctx context.Context, op scop.MakePublicForeignKeyConstraintValidated, ) error { // A helper function to update the inbound FK in referenced table to DROPPING. - updateInboundFKAsDropping := func(referencedTableID descpb.ID) error { + // Note that the constraintID in the referencedTable might not match that in + // in the referencing table, so we pass in the FK name as identifier. + updateInboundFKAsDropping := func(referencedTableID descpb.ID, fkName string) error { foundInReferencedTable := false in, err := i.checkOutTable(ctx, referencedTableID) if err != nil { return err } for idx, inboundFk := range in.InboundFKs { - if inboundFk.OriginTableID == op.TableID && inboundFk.ConstraintID == op.ConstraintID { + if inboundFk.OriginTableID == op.TableID && inboundFk.Name == fkName { in.InboundFKs[idx].Validity = descpb.ConstraintValidity_Dropping foundInReferencedTable = true break @@ -487,15 +494,15 @@ func (i *immediateVisitor) MakePublicForeignKeyConstraintValidated( } fk.Validity = descpb.ConstraintValidity_Dropping enqueueNonIndexMutation(out, out.AddForeignKeyMutation, &fk, descpb.DescriptorMutation_DROP) - return updateInboundFKAsDropping(fk.ReferencedTableID) + return updateInboundFKAsDropping(fk.ReferencedTableID, fk.Name) } } return errors.AssertionFailedf("failed to find FK constraint %d in descriptor %v", op.ConstraintID, out) } -func (i *immediateVisitor) MakeAbsentUniqueWithoutIndexConstraintWriteOnly( - ctx context.Context, op scop.MakeAbsentUniqueWithoutIndexConstraintWriteOnly, +func (i *immediateVisitor) AddUniqueWithoutIndexConstraint( + ctx context.Context, op scop.AddUniqueWithoutIndexConstraint, ) error { tbl, err := i.checkOutTable(ctx, op.TableID) if err != nil || tbl.Dropped() { @@ -509,13 +516,17 @@ func (i *immediateVisitor) MakeAbsentUniqueWithoutIndexConstraintWriteOnly( TableID: op.TableID, ColumnIDs: op.ColumnIDs, Name: tabledesc.ConstraintNamePlaceholder(op.ConstraintID), - Validity: descpb.ConstraintValidity_Validating, + Validity: op.Validity, ConstraintID: op.ConstraintID, Predicate: string(op.PartialExpr), } + if op.Validity == descpb.ConstraintValidity_Unvalidated { + // Unvalidated constraint doesn't need to transition through an intermediate + // state, so we don't enqueue a mutation for it. + tbl.UniqueWithoutIndexConstraints = append(tbl.UniqueWithoutIndexConstraints, *uwi) + return nil + } enqueueNonIndexMutation(tbl, tbl.AddUniqueWithoutIndexMutation, uwi, descpb.DescriptorMutation_ADD) - // Fast-forward the mutation state to WRITE_ONLY because this constraint - // is now considered as enforced. tbl.Mutations[len(tbl.Mutations)-1].State = descpb.DescriptorMutation_WRITE_ONLY return nil } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/references.go b/pkg/sql/schemachanger/scexec/scmutationexec/references.go index a0b0160b7a87..57ae9b24068a 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/references.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/references.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/iterutil" + "github.com/cockroachdb/errors" ) func (i *immediateVisitor) RemoveSchemaParent( @@ -73,6 +74,29 @@ func (i *immediateVisitor) RemoveForeignKeyBackReference( // Exit early if the foreign key back-reference holder is getting dropped. return err } + // Retrieve foreign key name in origin table to identify it in the referenced + // table. + var name string + { + out, err := i.getDescriptor(ctx, op.OriginTableID) + if err != nil { + return err + } + tbl, err := catalog.AsTableDescriptor(out) + if err != nil { + return err + } + for _, fk := range tbl.OutboundForeignKeys() { + if fk.GetConstraintID() == op.OriginConstraintID { + name = fk.GetName() + break + } + } + if name == "" { + return errors.AssertionFailedf("foreign key with ID %d not found in origin table %q (%d)", + op.OriginConstraintID, out.GetName(), out.GetID()) + } + } // Attempt to remove back reference. // Note how we // 1. only check to remove from `in.InboundFKs` but not from `in.Mutations`: @@ -83,7 +107,7 @@ func (i *immediateVisitor) RemoveForeignKeyBackReference( // this is because if we roll back before the adding FK is published in `out`, // such a back-reference won't exist in `in` yet. for idx, fk := range in.InboundFKs { - if fk.OriginTableID == op.OriginTableID && fk.ConstraintID == op.OriginConstraintID { + if fk.OriginTableID == op.OriginTableID && fk.Name == name { in.InboundFKs = append(in.InboundFKs[:idx], in.InboundFKs[idx+1:]...) if len(in.InboundFKs) == 0 { in.InboundFKs = nil diff --git a/pkg/sql/schemachanger/scop/immediate_mutation.go b/pkg/sql/schemachanger/scop/immediate_mutation.go index 0523e3b03bd1..32952992e494 100644 --- a/pkg/sql/schemachanger/scop/immediate_mutation.go +++ b/pkg/sql/schemachanger/scop/immediate_mutation.go @@ -266,15 +266,16 @@ type RemoveColumnNotNull struct { ColumnID descpb.ColumnID } -// MakeAbsentCheckConstraintWriteOnly adds a non-existent check constraint -// to the table in the WRITE_ONLY state. -type MakeAbsentCheckConstraintWriteOnly struct { +// AddCheckConstraint adds a non-existent check constraint +// to the table. +type AddCheckConstraint struct { immediateMutationOp TableID descpb.ID ConstraintID descpb.ConstraintID ColumnIDs []descpb.ColumnID CheckExpr catpb.Expression FromHashShardedColumn bool + Validity descpb.ConstraintValidity } // MakeAbsentColumnNotNullWriteOnly adds a non-existent NOT NULL constraint, @@ -317,9 +318,9 @@ type MakeValidatedColumnNotNullPublic struct { ColumnID descpb.ColumnID } -// MakeAbsentForeignKeyConstraintWriteOnly adds a non-existent foreign key -// constraint to the table in the WRITE_ONLY state. -type MakeAbsentForeignKeyConstraintWriteOnly struct { +// AddForeignKeyConstraint adds a non-existent foreign key +// constraint to the table. +type AddForeignKeyConstraint struct { immediateMutationOp TableID descpb.ID ConstraintID descpb.ConstraintID @@ -329,6 +330,7 @@ type MakeAbsentForeignKeyConstraintWriteOnly struct { OnUpdateAction semenumpb.ForeignKeyAction OnDeleteAction semenumpb.ForeignKeyAction CompositeKeyMatchMethod semenumpb.Match + Validity descpb.ConstraintValidity } // MakeValidatedForeignKeyConstraintPublic moves a new, validated foreign key @@ -364,14 +366,15 @@ type RemoveForeignKeyBackReference struct { OriginConstraintID descpb.ConstraintID } -// MakeAbsentUniqueWithoutIndexConstraintWriteOnly adds a non-existent -// unique_without_index constraint to the table in the WRITE_ONLY state. -type MakeAbsentUniqueWithoutIndexConstraintWriteOnly struct { +// AddUniqueWithoutIndexConstraint adds a non-existent +// unique_without_index constraint to the table. +type AddUniqueWithoutIndexConstraint struct { immediateMutationOp TableID descpb.ID ConstraintID descpb.ConstraintID ColumnIDs []descpb.ColumnID PartialExpr catpb.Expression + Validity descpb.ConstraintValidity } // MakeValidatedUniqueWithoutIndexConstraintPublic moves a new, validated unique_without_index diff --git a/pkg/sql/schemachanger/scop/immediate_mutation_visitor_generated.go b/pkg/sql/schemachanger/scop/immediate_mutation_visitor_generated.go index 9a312702c1ff..15dfa957054e 100644 --- a/pkg/sql/schemachanger/scop/immediate_mutation_visitor_generated.go +++ b/pkg/sql/schemachanger/scop/immediate_mutation_visitor_generated.go @@ -53,18 +53,18 @@ type ImmediateMutationVisitor interface { RemoveSequenceOwner(context.Context, RemoveSequenceOwner) error RemoveCheckConstraint(context.Context, RemoveCheckConstraint) error RemoveColumnNotNull(context.Context, RemoveColumnNotNull) error - MakeAbsentCheckConstraintWriteOnly(context.Context, MakeAbsentCheckConstraintWriteOnly) error + AddCheckConstraint(context.Context, AddCheckConstraint) error MakeAbsentColumnNotNullWriteOnly(context.Context, MakeAbsentColumnNotNullWriteOnly) error MakePublicCheckConstraintValidated(context.Context, MakePublicCheckConstraintValidated) error MakePublicColumnNotNullValidated(context.Context, MakePublicColumnNotNullValidated) error MakeValidatedCheckConstraintPublic(context.Context, MakeValidatedCheckConstraintPublic) error MakeValidatedColumnNotNullPublic(context.Context, MakeValidatedColumnNotNullPublic) error - MakeAbsentForeignKeyConstraintWriteOnly(context.Context, MakeAbsentForeignKeyConstraintWriteOnly) error + AddForeignKeyConstraint(context.Context, AddForeignKeyConstraint) error MakeValidatedForeignKeyConstraintPublic(context.Context, MakeValidatedForeignKeyConstraintPublic) error MakePublicForeignKeyConstraintValidated(context.Context, MakePublicForeignKeyConstraintValidated) error RemoveForeignKeyConstraint(context.Context, RemoveForeignKeyConstraint) error RemoveForeignKeyBackReference(context.Context, RemoveForeignKeyBackReference) error - MakeAbsentUniqueWithoutIndexConstraintWriteOnly(context.Context, MakeAbsentUniqueWithoutIndexConstraintWriteOnly) error + AddUniqueWithoutIndexConstraint(context.Context, AddUniqueWithoutIndexConstraint) error MakeValidatedUniqueWithoutIndexConstraintPublic(context.Context, MakeValidatedUniqueWithoutIndexConstraintPublic) error MakePublicUniqueWithoutIndexConstraintValidated(context.Context, MakePublicUniqueWithoutIndexConstraintValidated) error RemoveUniqueWithoutIndexConstraint(context.Context, RemoveUniqueWithoutIndexConstraint) error @@ -272,8 +272,8 @@ func (op RemoveColumnNotNull) Visit(ctx context.Context, v ImmediateMutationVisi } // Visit is part of the ImmediateMutationOp interface. -func (op MakeAbsentCheckConstraintWriteOnly) Visit(ctx context.Context, v ImmediateMutationVisitor) error { - return v.MakeAbsentCheckConstraintWriteOnly(ctx, op) +func (op AddCheckConstraint) Visit(ctx context.Context, v ImmediateMutationVisitor) error { + return v.AddCheckConstraint(ctx, op) } // Visit is part of the ImmediateMutationOp interface. @@ -302,8 +302,8 @@ func (op MakeValidatedColumnNotNullPublic) Visit(ctx context.Context, v Immediat } // Visit is part of the ImmediateMutationOp interface. -func (op MakeAbsentForeignKeyConstraintWriteOnly) Visit(ctx context.Context, v ImmediateMutationVisitor) error { - return v.MakeAbsentForeignKeyConstraintWriteOnly(ctx, op) +func (op AddForeignKeyConstraint) Visit(ctx context.Context, v ImmediateMutationVisitor) error { + return v.AddForeignKeyConstraint(ctx, op) } // Visit is part of the ImmediateMutationOp interface. @@ -327,8 +327,8 @@ func (op RemoveForeignKeyBackReference) Visit(ctx context.Context, v ImmediateMu } // Visit is part of the ImmediateMutationOp interface. -func (op MakeAbsentUniqueWithoutIndexConstraintWriteOnly) Visit(ctx context.Context, v ImmediateMutationVisitor) error { - return v.MakeAbsentUniqueWithoutIndexConstraintWriteOnly(ctx, op) +func (op AddUniqueWithoutIndexConstraint) Visit(ctx context.Context, v ImmediateMutationVisitor) error { + return v.AddUniqueWithoutIndexConstraint(ctx, op) } // Visit is part of the ImmediateMutationOp interface. diff --git a/pkg/sql/schemachanger/scpb/elements.proto b/pkg/sql/schemachanger/scpb/elements.proto index a89c279d93ca..21b180a43af5 100644 --- a/pkg/sql/schemachanger/scpb/elements.proto +++ b/pkg/sql/schemachanger/scpb/elements.proto @@ -71,8 +71,11 @@ message ElementProto { SecondaryIndex secondary_index = 23 [(gogoproto.moretags) = "parent:\"Table, View\""]; TemporaryIndex temporary_index = 24 [(gogoproto.moretags) = "parent:\"Table, View\""]; UniqueWithoutIndexConstraint unique_without_index_constraint = 25 [(gogoproto.moretags) = "parent:\"Table\""]; + UniqueWithoutIndexConstraintUnvalidated unique_without_index_constraint_unvalidated = 171 [(gogoproto.moretags) = "parent:\"Table\""]; CheckConstraint check_constraint = 26 [(gogoproto.moretags) = "parent:\"Table\""]; + CheckConstraintUnvalidated check_constraint_unvalidated = 170 [(gogoproto.moretags) = "parent:\"Table\""]; ForeignKeyConstraint foreign_key_constraint = 27 [(gogoproto.moretags) = "parent:\"Table\""]; + ForeignKeyConstraintUnvalidated foreign_key_constraint_not_valid = 172 [(gogoproto.moretags) = "parent:\"Table\""]; TableComment table_comment = 28 [(gogoproto.moretags) = "parent:\"Table, View, Sequence\""]; RowLevelTTL row_level_ttl = 29 [(gogoproto.customname) = "RowLevelTTL", (gogoproto.moretags) = "parent:\"Table\""]; TableZoneConfig table_zone_config = 121 [(gogoproto.moretags) = "parent:\"Table, View\""]; @@ -139,7 +142,7 @@ message ElementProto { FunctionBody function_body = 164 [(gogoproto.moretags) = "parent:\"Function\""]; FunctionParamDefaultExpression function_param_default = 165 [(gogoproto.moretags) = "parent:\"Function\""]; - // Next element group start id: 170 + // Next element group start id: 180 } // TypeT is a wrapper for a types.T which contains its user-defined type ID @@ -355,6 +358,14 @@ message UniqueWithoutIndexConstraint { uint32 index_id_for_validation = 5 [(gogoproto.customname) = "IndexIDForValidation", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.IndexID"]; } +message UniqueWithoutIndexConstraintUnvalidated { + uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; + uint32 constraint_id = 2 [(gogoproto.customname) = "ConstraintID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ConstraintID"]; + repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"]; + // Predicate, if non-nil, means a partial uniqueness constraint. + Expression predicate = 4 [(gogoproto.customname) = "Predicate"]; +} + message CheckConstraint { uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; uint32 constraint_id = 2 [(gogoproto.customname) = "ConstraintID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ConstraintID"]; @@ -367,6 +378,13 @@ message CheckConstraint { uint32 index_id_for_validation = 6 [(gogoproto.customname) = "IndexIDForValidation", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.IndexID"]; } +message CheckConstraintUnvalidated { + uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; + uint32 constraint_id = 2 [(gogoproto.customname) = "ConstraintID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ConstraintID"]; + repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"]; + Expression embedded_expr = 4 [(gogoproto.nullable) = false, (gogoproto.embed) = true]; +} + message ForeignKeyConstraint { uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; uint32 constraint_id = 2 [(gogoproto.customname) = "ConstraintID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ConstraintID"]; @@ -381,6 +399,17 @@ message ForeignKeyConstraint { uint32 index_id_for_validation = 9 [(gogoproto.customname) = "IndexIDForValidation", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.IndexID"]; } +message ForeignKeyConstraintUnvalidated { + uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; + uint32 constraint_id = 2 [(gogoproto.customname) = "ConstraintID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ConstraintID"]; + repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"]; + uint32 referenced_table_id = 4 [(gogoproto.customname) = "ReferencedTableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; + repeated uint32 referenced_column_ids = 5 [(gogoproto.customname) = "ReferencedColumnIDs", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.ColumnID"]; + cockroach.sql.sem.semenumpb.ForeignKeyAction on_update_action = 6 [(gogoproto.customname) = "OnUpdateAction"]; + cockroach.sql.sem.semenumpb.ForeignKeyAction on_delete_action = 7 [(gogoproto.customname) = "OnDeleteAction"]; + cockroach.sql.sem.semenumpb.Match composite_key_match_method = 8 [(gogoproto.customname) = "CompositeKeyMatchMethod"]; +} + message EnumType { uint32 type_id = 1 [(gogoproto.customname) = "TypeID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; uint32 array_type_id = 2 [(gogoproto.customname) = "ArrayTypeID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"]; diff --git a/pkg/sql/schemachanger/scpb/elements_generated.go b/pkg/sql/schemachanger/scpb/elements_generated.go index 86dcc41976a5..e3f66c53e8f1 100644 --- a/pkg/sql/schemachanger/scpb/elements_generated.go +++ b/pkg/sql/schemachanger/scpb/elements_generated.go @@ -79,6 +79,37 @@ func FindCheckConstraint(b ElementStatusIterator) (current Status, target Target return current, target, element } +func (e CheckConstraintUnvalidated) element() {} + +// ForEachCheckConstraintUnvalidated iterates over elements of type CheckConstraintUnvalidated. +func ForEachCheckConstraintUnvalidated( + b ElementStatusIterator, fn func(current Status, target TargetStatus, e *CheckConstraintUnvalidated), +) { + if b == nil { + return + } + b.ForEachElementStatus(func(current Status, target TargetStatus, e Element) { + if elt, ok := e.(*CheckConstraintUnvalidated); ok { + fn(current, target, elt) + } + }) +} + +// FindCheckConstraintUnvalidated finds the first element of type CheckConstraintUnvalidated. +func FindCheckConstraintUnvalidated(b ElementStatusIterator) (current Status, target TargetStatus, element *CheckConstraintUnvalidated) { + if b == nil { + return current, target, element + } + b.ForEachElementStatus(func(c Status, t TargetStatus, e Element) { + if elt, ok := e.(*CheckConstraintUnvalidated); ok { + element = elt + current = c + target = t + } + }) + return current, target, element +} + func (e Column) element() {} // ForEachColumn iterates over elements of type Column. @@ -730,6 +761,37 @@ func FindForeignKeyConstraint(b ElementStatusIterator) (current Status, target T return current, target, element } +func (e ForeignKeyConstraintUnvalidated) element() {} + +// ForEachForeignKeyConstraintUnvalidated iterates over elements of type ForeignKeyConstraintUnvalidated. +func ForEachForeignKeyConstraintUnvalidated( + b ElementStatusIterator, fn func(current Status, target TargetStatus, e *ForeignKeyConstraintUnvalidated), +) { + if b == nil { + return + } + b.ForEachElementStatus(func(current Status, target TargetStatus, e Element) { + if elt, ok := e.(*ForeignKeyConstraintUnvalidated); ok { + fn(current, target, elt) + } + }) +} + +// FindForeignKeyConstraintUnvalidated finds the first element of type ForeignKeyConstraintUnvalidated. +func FindForeignKeyConstraintUnvalidated(b ElementStatusIterator) (current Status, target TargetStatus, element *ForeignKeyConstraintUnvalidated) { + if b == nil { + return current, target, element + } + b.ForEachElementStatus(func(c Status, t TargetStatus, e Element) { + if elt, ok := e.(*ForeignKeyConstraintUnvalidated); ok { + element = elt + current = c + target = t + } + }) + return current, target, element +} + func (e Function) element() {} // ForEachFunction iterates over elements of type Function. @@ -1815,6 +1877,37 @@ func FindUniqueWithoutIndexConstraint(b ElementStatusIterator) (current Status, return current, target, element } +func (e UniqueWithoutIndexConstraintUnvalidated) element() {} + +// ForEachUniqueWithoutIndexConstraintUnvalidated iterates over elements of type UniqueWithoutIndexConstraintUnvalidated. +func ForEachUniqueWithoutIndexConstraintUnvalidated( + b ElementStatusIterator, fn func(current Status, target TargetStatus, e *UniqueWithoutIndexConstraintUnvalidated), +) { + if b == nil { + return + } + b.ForEachElementStatus(func(current Status, target TargetStatus, e Element) { + if elt, ok := e.(*UniqueWithoutIndexConstraintUnvalidated); ok { + fn(current, target, elt) + } + }) +} + +// FindUniqueWithoutIndexConstraintUnvalidated finds the first element of type UniqueWithoutIndexConstraintUnvalidated. +func FindUniqueWithoutIndexConstraintUnvalidated(b ElementStatusIterator) (current Status, target TargetStatus, element *UniqueWithoutIndexConstraintUnvalidated) { + if b == nil { + return current, target, element + } + b.ForEachElementStatus(func(c Status, t TargetStatus, e Element) { + if elt, ok := e.(*UniqueWithoutIndexConstraintUnvalidated); ok { + element = elt + current = c + target = t + } + }) + return current, target, element +} + func (e UserPrivileges) element() {} // ForEachUserPrivileges iterates over elements of type UserPrivileges. diff --git a/pkg/sql/schemachanger/scpb/uml/table.puml b/pkg/sql/schemachanger/scpb/uml/table.puml index a74acc012794..9e98c7c7acc2 100644 --- a/pkg/sql/schemachanger/scpb/uml/table.puml +++ b/pkg/sql/schemachanger/scpb/uml/table.puml @@ -90,6 +90,13 @@ UniqueWithoutIndexConstraint : []ColumnIDs UniqueWithoutIndexConstraint : Predicate UniqueWithoutIndexConstraint : IndexIDForValidation +object UniqueWithoutIndexConstraintUnvalidated + +UniqueWithoutIndexConstraintUnvalidated : TableID +UniqueWithoutIndexConstraintUnvalidated : ConstraintID +UniqueWithoutIndexConstraintUnvalidated : []ColumnIDs +UniqueWithoutIndexConstraintUnvalidated : Predicate + object CheckConstraint CheckConstraint : TableID @@ -99,6 +106,13 @@ CheckConstraint : Expression CheckConstraint : FromHashShardedColumn CheckConstraint : IndexIDForValidation +object CheckConstraintUnvalidated + +CheckConstraintUnvalidated : TableID +CheckConstraintUnvalidated : ConstraintID +CheckConstraintUnvalidated : []ColumnIDs +CheckConstraintUnvalidated : Expression + object ForeignKeyConstraint ForeignKeyConstraint : TableID @@ -111,6 +125,17 @@ ForeignKeyConstraint : OnDeleteAction ForeignKeyConstraint : CompositeKeyMatchMethod ForeignKeyConstraint : IndexIDForValidation +object ForeignKeyConstraintUnvalidated + +ForeignKeyConstraintUnvalidated : TableID +ForeignKeyConstraintUnvalidated : ConstraintID +ForeignKeyConstraintUnvalidated : []ColumnIDs +ForeignKeyConstraintUnvalidated : ReferencedTableID +ForeignKeyConstraintUnvalidated : []ReferencedColumnIDs +ForeignKeyConstraintUnvalidated : OnUpdateAction +ForeignKeyConstraintUnvalidated : OnDeleteAction +ForeignKeyConstraintUnvalidated : CompositeKeyMatchMethod + object TableComment TableComment : TableID @@ -368,8 +393,11 @@ View <|-- SecondaryIndex Table <|-- TemporaryIndex View <|-- TemporaryIndex Table <|-- UniqueWithoutIndexConstraint +Table <|-- UniqueWithoutIndexConstraintUnvalidated Table <|-- CheckConstraint +Table <|-- CheckConstraintUnvalidated Table <|-- ForeignKeyConstraint +Table <|-- ForeignKeyConstraintUnvalidated Table <|-- TableComment View <|-- TableComment Sequence <|-- TableComment diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/BUILD.bazel b/pkg/sql/schemachanger/scplan/internal/opgen/BUILD.bazel index 838df49bb5e8..bd7ba8d0a1a8 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/BUILD.bazel +++ b/pkg/sql/schemachanger/scplan/internal/opgen/BUILD.bazel @@ -8,6 +8,7 @@ go_library( "op_gen.go", "opgen_alias_type.go", "opgen_check_constraint.go", + "opgen_check_constraint_unvalidated.go", "opgen_column.go", "opgen_column_comment.go", "opgen_column_default_expression.go", @@ -29,6 +30,7 @@ go_library( "opgen_enum_type.go", "opgen_enum_type_value.go", "opgen_foreign_key_constraint.go", + "opgen_foreign_key_constraint_unvalidated.go", "opgen_function.go", "opgen_function_body.go", "opgen_function_leakproof.go", @@ -64,6 +66,7 @@ go_library( "opgen_table_zone_config.go", "opgen_temporary_index.go", "opgen_unique_without_index_constraint.go", + "opgen_unique_without_index_constraint_unvalidated.go", "opgen_user_privileges.go", "opgen_view.go", "register.go", diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go index 787e6b7b0964..00f7a7878edd 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go @@ -11,6 +11,7 @@ package opgen import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" ) @@ -20,13 +21,14 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_WRITE_ONLY, - emit(func(this *scpb.CheckConstraint) *scop.MakeAbsentCheckConstraintWriteOnly { - return &scop.MakeAbsentCheckConstraintWriteOnly{ + emit(func(this *scpb.CheckConstraint) *scop.AddCheckConstraint { + return &scop.AddCheckConstraint{ TableID: this.TableID, ConstraintID: this.ConstraintID, ColumnIDs: this.ColumnIDs, CheckExpr: this.Expr, FromHashShardedColumn: this.FromHashShardedColumn, + Validity: descpb.ConstraintValidity_Validating, } }), emit(func(this *scpb.CheckConstraint) *scop.UpdateTableBackReferencesInTypes { diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint_unvalidated.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint_unvalidated.go new file mode 100644 index 000000000000..d198faf30b31 --- /dev/null +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint_unvalidated.go @@ -0,0 +1,83 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package opgen + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" +) + +func init() { + opRegistry.register((*scpb.CheckConstraintUnvalidated)(nil), + toPublic( + scpb.Status_ABSENT, + to(scpb.Status_PUBLIC, + emit(func(this *scpb.CheckConstraintUnvalidated) *scop.AddCheckConstraint { + return &scop.AddCheckConstraint{ + TableID: this.TableID, + ConstraintID: this.ConstraintID, + ColumnIDs: this.ColumnIDs, + CheckExpr: this.Expr, + Validity: descpb.ConstraintValidity_Unvalidated, + } + }), + emit(func(this *scpb.CheckConstraintUnvalidated) *scop.UpdateTableBackReferencesInTypes { + if len(this.UsesTypeIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInTypes{ + TypeIDs: this.UsesTypeIDs, + BackReferencedTableID: this.TableID, + } + }), + emit(func(this *scpb.CheckConstraintUnvalidated) *scop.UpdateTableBackReferencesInSequences { + if len(this.UsesSequenceIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInSequences{ + SequenceIDs: this.UsesSequenceIDs, + BackReferencedTableID: this.TableID, + } + }), + ), + ), + toAbsent( + scpb.Status_PUBLIC, + to(scpb.Status_ABSENT, + emit(func(this *scpb.CheckConstraintUnvalidated) *scop.RemoveCheckConstraint { + return &scop.RemoveCheckConstraint{ + TableID: this.TableID, + ConstraintID: this.ConstraintID, + } + }), + emit(func(this *scpb.CheckConstraintUnvalidated) *scop.UpdateTableBackReferencesInTypes { + if len(this.UsesTypeIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInTypes{ + TypeIDs: this.UsesTypeIDs, + BackReferencedTableID: this.TableID, + } + }), + emit(func(this *scpb.CheckConstraintUnvalidated) *scop.UpdateTableBackReferencesInSequences { + if len(this.UsesSequenceIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInSequences{ + SequenceIDs: this.UsesSequenceIDs, + BackReferencedTableID: this.TableID, + } + }), + ), + ), + ) +} diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint.go index 52d514a6ee72..bd9ea483f661 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint.go @@ -11,6 +11,7 @@ package opgen import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" ) @@ -20,8 +21,8 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_WRITE_ONLY, - emit(func(this *scpb.ForeignKeyConstraint) *scop.MakeAbsentForeignKeyConstraintWriteOnly { - return &scop.MakeAbsentForeignKeyConstraintWriteOnly{ + emit(func(this *scpb.ForeignKeyConstraint) *scop.AddForeignKeyConstraint { + return &scop.AddForeignKeyConstraint{ TableID: this.TableID, ConstraintID: this.ConstraintID, ColumnIDs: this.ColumnIDs, @@ -30,6 +31,7 @@ func init() { OnUpdateAction: this.OnUpdateAction, OnDeleteAction: this.OnDeleteAction, CompositeKeyMatchMethod: this.CompositeKeyMatchMethod, + Validity: descpb.ConstraintValidity_Validating, } }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint_unvalidated.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint_unvalidated.go new file mode 100644 index 000000000000..3d268b7d9b67 --- /dev/null +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_foreign_key_constraint_unvalidated.go @@ -0,0 +1,58 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package opgen + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" +) + +func init() { + opRegistry.register((*scpb.ForeignKeyConstraintUnvalidated)(nil), + toPublic( + scpb.Status_ABSENT, + to(scpb.Status_PUBLIC, + emit(func(this *scpb.ForeignKeyConstraintUnvalidated) *scop.AddForeignKeyConstraint { + return &scop.AddForeignKeyConstraint{ + TableID: this.TableID, + ConstraintID: this.ConstraintID, + ColumnIDs: this.ColumnIDs, + ReferencedTableID: this.ReferencedTableID, + ReferencedColumnIDs: this.ReferencedColumnIDs, + OnUpdateAction: this.OnUpdateAction, + OnDeleteAction: this.OnDeleteAction, + CompositeKeyMatchMethod: this.CompositeKeyMatchMethod, + Validity: descpb.ConstraintValidity_Unvalidated, + } + }), + ), + ), + toAbsent( + scpb.Status_PUBLIC, + to(scpb.Status_ABSENT, + emit(func(this *scpb.ForeignKeyConstraintUnvalidated) *scop.RemoveForeignKeyBackReference { + return &scop.RemoveForeignKeyBackReference{ + ReferencedTableID: this.ReferencedTableID, + OriginTableID: this.TableID, + OriginConstraintID: this.ConstraintID, + } + }), + emit(func(this *scpb.ForeignKeyConstraintUnvalidated) *scop.RemoveForeignKeyConstraint { + return &scop.RemoveForeignKeyConstraint{ + TableID: this.TableID, + ConstraintID: this.ConstraintID, + } + }), + ), + ), + ) +} diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go index 741021f58e69..1155d15c1ae4 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go @@ -12,6 +12,7 @@ package opgen import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" ) @@ -21,16 +22,17 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_WRITE_ONLY, - emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.MakeAbsentUniqueWithoutIndexConstraintWriteOnly { + emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.AddUniqueWithoutIndexConstraint { var partialExpr catpb.Expression if this.Predicate != nil { partialExpr = this.Predicate.Expr } - return &scop.MakeAbsentUniqueWithoutIndexConstraintWriteOnly{ + return &scop.AddUniqueWithoutIndexConstraint{ TableID: this.TableID, ConstraintID: this.ConstraintID, ColumnIDs: this.ColumnIDs, PartialExpr: partialExpr, + Validity: descpb.ConstraintValidity_Validating, } }), emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.UpdateTableBackReferencesInTypes { diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint_unvalidated.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint_unvalidated.go new file mode 100644 index 000000000000..db9516951028 --- /dev/null +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint_unvalidated.go @@ -0,0 +1,88 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package opgen + +import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" +) + +func init() { + opRegistry.register((*scpb.UniqueWithoutIndexConstraintUnvalidated)(nil), + toPublic( + scpb.Status_ABSENT, + to(scpb.Status_PUBLIC, + emit(func(this *scpb.UniqueWithoutIndexConstraintUnvalidated) *scop.AddUniqueWithoutIndexConstraint { + var partialExpr catpb.Expression + if this.Predicate != nil { + partialExpr = this.Predicate.Expr + } + return &scop.AddUniqueWithoutIndexConstraint{ + TableID: this.TableID, + ConstraintID: this.ConstraintID, + ColumnIDs: this.ColumnIDs, + PartialExpr: partialExpr, + Validity: descpb.ConstraintValidity_Unvalidated, + } + }), + emit(func(this *scpb.UniqueWithoutIndexConstraintUnvalidated) *scop.UpdateTableBackReferencesInTypes { + if this.Predicate == nil || this.Predicate.UsesTypeIDs == nil || len(this.Predicate.UsesTypeIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInTypes{ + TypeIDs: this.Predicate.UsesTypeIDs, + BackReferencedTableID: this.TableID, + } + }), + emit(func(this *scpb.UniqueWithoutIndexConstraintUnvalidated) *scop.UpdateTableBackReferencesInSequences { + if this.Predicate == nil || this.Predicate.UsesSequenceIDs == nil || len(this.Predicate.UsesSequenceIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInSequences{ + SequenceIDs: this.Predicate.UsesSequenceIDs, + BackReferencedTableID: this.TableID, + } + }), + ), + ), + toAbsent( + scpb.Status_PUBLIC, + to(scpb.Status_ABSENT, + emit(func(this *scpb.UniqueWithoutIndexConstraintUnvalidated) *scop.RemoveUniqueWithoutIndexConstraint { + return &scop.RemoveUniqueWithoutIndexConstraint{ + TableID: this.TableID, + ConstraintID: this.ConstraintID, + } + }), + emit(func(this *scpb.UniqueWithoutIndexConstraintUnvalidated) *scop.UpdateTableBackReferencesInTypes { + if this.Predicate == nil || this.Predicate.UsesTypeIDs == nil || len(this.Predicate.UsesTypeIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInTypes{ + TypeIDs: this.Predicate.UsesTypeIDs, + BackReferencedTableID: this.TableID, + } + }), + emit(func(this *scpb.UniqueWithoutIndexConstraintUnvalidated) *scop.UpdateTableBackReferencesInSequences { + if this.Predicate == nil || this.Predicate.UsesSequenceIDs == nil || len(this.Predicate.UsesSequenceIDs) == 0 { + return nil + } + return &scop.UpdateTableBackReferencesInSequences{ + SequenceIDs: this.Predicate.UsesSequenceIDs, + BackReferencedTableID: this.TableID, + } + }), + ), + ), + ) +} diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/assertions_test.go b/pkg/sql/schemachanger/scplan/internal/rules/current/assertions_test.go index bd4d57f704ba..3ef90b26a7e0 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/assertions_test.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/assertions_test.go @@ -185,11 +185,16 @@ func checkIsConstraintDependent(e scpb.Element) error { return nil } -// Assert that any element with a "ConstraintID" attr is either a constraint -// or a constraintDependent. -// If it's a constraint, then -// - it's either a NonIndexBackedConstraint or isIndex -// - it's either a CrossDescriptorConstraint or it does not a referencedDescID|ReferencedSequenceIDs|ReferencedTypeIDs attr +// Assert the following partitions about constraints: +// 1. An element `e` with ConstraintID attr is either a constraint +// or a constraint dependent. +// 2. A constraint is either index-backed or non-index-backed. +// +// TODO (xiang): Add test for cross-descriptor partition. We currently +// cannot have them until we added referenced.*ID attr for +// UniqueWithoutIndex[NotValid] element, which is required to support +// partial unique without index constraint with a predicate that references +// other descriptors. func checkConstraintPartitions(e scpb.Element) error { _, err := screl.Schema.GetAttribute(screl.ConstraintID, e) if err != nil { @@ -199,27 +204,10 @@ func checkConstraintPartitions(e scpb.Element) error { return errors.New("has ConstraintID attr but is not a constraint nor a constraint dependent") } if isConstraintDependent(e) { - // The checks below only apply to constraints, so skip constraint dependents. return nil } - if isNonIndexBackedConstraint(e) == isIndex(e) { - if isIndex(e) { - return errors.New("verifies both isIndex and isNonIndexBackedConstraint") - } else { - return errors.New("doesn't verify isIndex nor isNonIndexBackedConstraint") - } - } - _, err1 := screl.Schema.GetAttribute(screl.ReferencedDescID, e) - _, err2 := screl.Schema.GetAttribute(screl.ReferencedSequenceIDs, e) - _, err3 := screl.Schema.GetAttribute(screl.ReferencedTypeIDs, e) - if isCrossDescriptorConstraint(e) { - if err1 != nil && err2 != nil && err3 != nil { - return errors.New("verifies isCrossDescriptorConstraint but does not have a Referenced.*ID attr") - } - } else { - if err1 == nil || err2 == nil || err3 == nil { - return errors.New("doesn't verify isCrossDescriptorConstraint but has a Referenced.*ID attr") - } + if !isNonIndexBackedConstraint(e) && !isIndex(e) { + return errors.New("verifies isConstraint but does not verify isNonIndexBackedConstraint nor isIndex") } return nil } diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_constraint.go b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_constraint.go index 81e89f3a8b79..5936f5d333fa 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_constraint.go @@ -21,13 +21,27 @@ import ( // name, etc. appear once the constraint reaches a suitable state. func init() { registerDepRule( - "constraint dependent public right before constraint", + "constraint dependent public right before complex constraint", scgraph.SameStagePrecedence, - "dependent", "constraint", + "dependent", "complex-constraint", func(from, to NodeVars) rel.Clauses { return rel.Clauses{ from.TypeFilter(rulesVersionKey, isConstraintDependent), - to.TypeFilter(rulesVersionKey, isConstraint), + to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, isSubjectTo2VersionInvariant), + JoinOnConstraintID(from, to, "table-id", "constraint-id"), + StatusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_PUBLIC), + } + }, + ) + + registerDepRule( + "simple constraint public right before its dependents", + scgraph.SameStagePrecedence, + "simple-constraint", "dependent", + func(from, to NodeVars) rel.Clauses { + return rel.Clauses{ + from.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, Not(isNonIndexBackedCrossDescriptorConstraint)), + to.TypeFilter(rulesVersionKey, isConstraintDependent), JoinOnConstraintID(from, to, "table-id", "constraint-id"), StatusesToPublicOrTransient(from, scpb.Status_PUBLIC, to, scpb.Status_PUBLIC), } diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_index_and_constraint.go b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_index_and_constraint.go index 2cae27f166bd..17a2519d9ad0 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_index_and_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_add_index_and_constraint.go @@ -28,7 +28,7 @@ func init() { func(from, to NodeVars) rel.Clauses { return rel.Clauses{ from.Type((*scpb.PrimaryIndex)(nil)), - to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint), + to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, isSubjectTo2VersionInvariant), JoinOnDescID(from, to, "table-id"), JoinOn( from, screl.IndexID, diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_constraint.go b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_constraint.go index c3a696572ee6..24f003668b43 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_constraint.go @@ -19,9 +19,6 @@ import ( // These rules ensure that constraint-dependent elements, like an constraint's // name, etc. disappear once the constraint reaches a suitable state. -// TODO (xiang): The dep rules here are not complete, as they are aimed specifically -// for check constraints only. Complete them when we properly support the -// other constraints: UniqueWithoutIndex, ForeignKey, Unique, and Not Null. func init() { registerDepRuleForDrop( @@ -31,7 +28,7 @@ func init() { scpb.Status_VALIDATED, scpb.Status_ABSENT, func(from, to NodeVars) rel.Clauses { return rel.Clauses{ - from.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint), + from.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, isSubjectTo2VersionInvariant), to.TypeFilter(rulesVersionKey, isConstraintDependent), JoinOnConstraintID(from, to, "table-id", "constraint-id"), } @@ -46,7 +43,26 @@ func init() { func(from, to NodeVars) rel.Clauses { return rel.Clauses{ from.TypeFilter(rulesVersionKey, isConstraintDependent), - to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint), + to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, isSubjectTo2VersionInvariant), + JoinOnConstraintID(from, to, "table-id", "constraint-id"), + } + }, + ) +} + +// These rules apply to simple constraints and ensure that their dependents, like +// their names, comments, etc., disappear right before the simple constraint. +func init() { + + registerDepRuleForDrop( + "dependents removed right before simple constraint", + scgraph.SameStagePrecedence, + "dependents", "constraint", + scpb.Status_ABSENT, scpb.Status_ABSENT, + func(from, to NodeVars) rel.Clauses { + return rel.Clauses{ + from.TypeFilter(rulesVersionKey, isConstraintDependent), + to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, Not(isSubjectTo2VersionInvariant)), JoinOnConstraintID(from, to, "table-id", "constraint-id"), } }, diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go index 48c298daa725..0229b719bc07 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go @@ -88,7 +88,7 @@ func init() { func(from, to NodeVars) rel.Clauses { return rel.Clauses{ from.Type((*scpb.Table)(nil)), - to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, Not(isCrossDescriptorConstraint)), + to.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, isSubjectTo2VersionInvariant, Not(isNonIndexBackedCrossDescriptorConstraint)), JoinOnDescID(from, to, "desc-id"), StatusesToAbsent(from, scpb.Status_DROPPED, to, scpb.Status_VALIDATED), } @@ -239,7 +239,7 @@ func init() { "cross-desc-constraint", "referenced-descriptor", func(from, to NodeVars) rel.Clauses { return rel.Clauses{ - from.TypeFilter(rulesVersionKey, isCrossDescriptorConstraint), + from.TypeFilter(rulesVersionKey, isNonIndexBackedCrossDescriptorConstraint, isSubjectTo2VersionInvariant), to.TypeFilter(rulesVersionKey, isDescriptor), JoinReferencedDescID(from, to, "desc-id"), StatusesToAbsent(from, scpb.Status_ABSENT, to, scpb.Status_DROPPED), @@ -253,7 +253,7 @@ func init() { "cross-desc-constraint", "referencing-descriptor", func(from, to NodeVars) rel.Clauses { return rel.Clauses{ - from.TypeFilter(rulesVersionKey, isCrossDescriptorConstraint), + from.TypeFilter(rulesVersionKey, isNonIndexBackedCrossDescriptorConstraint, isSubjectTo2VersionInvariant), to.TypeFilter(rulesVersionKey, isDescriptor), JoinOnDescID(from, to, "desc-id"), StatusesToAbsent(from, scpb.Status_ABSENT, to, scpb.Status_DROPPED), diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go b/pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go index 7ee84882c1e2..43cdb8d94332 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go @@ -65,7 +65,15 @@ func isSubjectTo2VersionInvariant(e scpb.Element) bool { // TODO(ajwerner): This should include constraints and enum values but it // currently does not because we do not support dropping them unless we're // dropping the descriptor and we do not support adding them. - return isIndex(e) || isColumn(e) || isNonIndexBackedConstraint(e) + if isIndex(e) || isColumn(e) { + return true + } + switch e.(type) { + case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, *scpb.ForeignKeyConstraint, + *scpb.ColumnNotNull: + return true + } + return false } func isIndex(e scpb.Element) bool { @@ -133,6 +141,11 @@ func getExpression(element scpb.Element) (*scpb.Expression, error) { return nil, nil } return &e.Expression, nil + case *scpb.CheckConstraintUnvalidated: + if e == nil { + return nil, nil + } + return &e.Expression, nil case *scpb.FunctionParamDefaultExpression: if e == nil { return nil, nil @@ -184,36 +197,44 @@ func isIndexDependent(e scpb.Element) bool { return false } -// isNonIndexBackedConstraint a non-index-backed constraint. -func isNonIndexBackedConstraint(e scpb.Element) bool { - switch e.(type) { - case *scpb.CheckConstraint, *scpb.ForeignKeyConstraint, *scpb.UniqueWithoutIndexConstraint, - *scpb.ColumnNotNull: - return true - } - return false +// CRDB supports five constraints of two categories: +// - PK, Unique (index-backed) +// - Check, UniqueWithoutIndex, FK (non-index-backed) +func isConstraint(e scpb.Element) bool { + return isIndex(e) || isNonIndexBackedConstraint(e) } -func isConstraint(e scpb.Element) bool { +// isNonIndexBackedConstraint returns true if `e` is a non-index-backed constraint. +func isNonIndexBackedConstraint(e scpb.Element) bool { switch e.(type) { - case *scpb.PrimaryIndex, *scpb.SecondaryIndex, *scpb.TemporaryIndex: + case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, *scpb.ForeignKeyConstraint, + *scpb.ColumnNotNull: return true - case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, *scpb.ForeignKeyConstraint: + case *scpb.CheckConstraintUnvalidated, *scpb.UniqueWithoutIndexConstraintUnvalidated, + *scpb.ForeignKeyConstraintUnvalidated: return true } return false } -// isCrossDescriptorConstraint are constraints that might reference -// other descriptors: -// - FKs can reference other table -// - Checks can reference other sequences/types +// isNonIndexBackedCrossDescriptorConstraint returns true if `e` is a +// non-index-backed constraint and it can potentially reference another +// descriptor. +// +// This filter exists because in general we need to drop the constraint first +// before dropping referencing/referenced descriptor. Read rules that use +// this filter for more details. // -// Those constraints need to be dropped first when we are dropping either -// the referencing or reference descriptor. -func isCrossDescriptorConstraint(e scpb.Element) bool { +// TODO (xiang): UniqueWithoutIndex and UniqueWithoutIndexNotValid should +// also be treated as cross-descriptor constraint because its partial predicate +// can references other descriptors. +func isNonIndexBackedCrossDescriptorConstraint(e scpb.Element) bool { switch e.(type) { - case *scpb.ForeignKeyConstraint, *scpb.CheckConstraint: + case *scpb.CheckConstraint, *scpb.UniqueWithoutIndexConstraint, + *scpb.ForeignKeyConstraint: + return true + case *scpb.CheckConstraintUnvalidated, *scpb.UniqueWithoutIndexConstraintUnvalidated, + *scpb.ForeignKeyConstraintUnvalidated: return true } return false diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/op_drop.go b/pkg/sql/schemachanger/scplan/internal/rules/current/op_drop.go index e10be4805636..a410c0c1f162 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/op_drop.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/op_drop.go @@ -168,7 +168,7 @@ func init() { (*scpb.Table)(nil), (*scpb.View)(nil), ), - constraint.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint), + constraint.TypeFilter(rulesVersionKey, isNonIndexBackedConstraint, Not(isNonIndexBackedCrossDescriptorConstraint)), JoinOnDescID(relation, constraint, relationID), diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules b/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules index a7a242800857..7c7307930c72 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/deprules @@ -1871,19 +1871,19 @@ deprules - $column-constraint-Node[CurrentStatus] = WRITE_ONLY - joinTargetNode($column, $column-Target, $column-Node) - joinTargetNode($column-constraint, $column-constraint-Target, $column-constraint-Node) -- name: constraint dependent public right before constraint +- name: constraint dependent public right before complex constraint from: dependent-Node kind: SameStagePrecedence - to: constraint-Node + to: complex-constraint-Node query: - $dependent[Type] IN ['*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment'] - - $constraint[Type] IN ['*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint'] - - joinOnConstraintID($dependent, $constraint, $table-id, $constraint-id) - - ToPublicOrTransient($dependent-Target, $constraint-Target) + - $complex-constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint', '*scpb.ColumnNotNull'] + - joinOnConstraintID($dependent, $complex-constraint, $table-id, $constraint-id) + - ToPublicOrTransient($dependent-Target, $complex-constraint-Target) - $dependent-Node[CurrentStatus] = PUBLIC - - $constraint-Node[CurrentStatus] = PUBLIC + - $complex-constraint-Node[CurrentStatus] = PUBLIC - joinTargetNode($dependent, $dependent-Target, $dependent-Node) - - joinTargetNode($constraint, $constraint-Target, $constraint-Node) + - joinTargetNode($complex-constraint, $complex-constraint-Target, $complex-constraint-Node) - name: constraint no longer public before dependents from: constraint-Node kind: Precedence @@ -1943,7 +1943,7 @@ deprules kind: Precedence to: referenced-descriptor-Node query: - - $cross-desc-constraint[Type] IN ['*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint'] + - $cross-desc-constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint'] - $referenced-descriptor[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - joinReferencedDescID($cross-desc-constraint, $referenced-descriptor, $desc-id) - toAbsent($cross-desc-constraint-Target, $referenced-descriptor-Target) @@ -1956,7 +1956,7 @@ deprules kind: Precedence to: referencing-descriptor-Node query: - - $cross-desc-constraint[Type] IN ['*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint'] + - $cross-desc-constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint'] - $referencing-descriptor[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - joinOnDescID($cross-desc-constraint, $referencing-descriptor, $desc-id) - toAbsent($cross-desc-constraint-Target, $referencing-descriptor-Target) @@ -2077,7 +2077,7 @@ deprules kind: Precedence to: relation-Node query: - - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TableData', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.ColumnNotNull', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.IndexData', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.DatabaseData', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] + - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraint', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraint', '*scpb.ForeignKeyConstraintUnvalidated', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TableData', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.ColumnNotNull', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.IndexData', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.DatabaseData', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] - $relation[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - joinOnDescID($dependent, $relation, $relation-id) - ToPublicOrTransient($dependent-Target, $relation-Target) @@ -2247,13 +2247,67 @@ deprules - $index-Node[CurrentStatus] = TRANSIENT_ABSENT - joinTargetNode($dependent, $dependent-Target, $dependent-Node) - joinTargetNode($index, $index-Target, $index-Node) +- name: dependents removed right before simple constraint + from: dependents-Node + kind: SameStagePrecedence + to: constraint-Node + query: + - $dependents[Type] IN ['*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment'] + - $constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraintUnvalidated'] + - joinOnConstraintID($dependents, $constraint, $table-id, $constraint-id) + - toAbsent($dependents-Target, $constraint-Target) + - $dependents-Node[CurrentStatus] = ABSENT + - $constraint-Node[CurrentStatus] = ABSENT + - joinTargetNode($dependents, $dependents-Target, $dependents-Node) + - joinTargetNode($constraint, $constraint-Target, $constraint-Node) +- name: dependents removed right before simple constraint + from: dependents-Node + kind: SameStagePrecedence + to: constraint-Node + query: + - $dependents[Type] IN ['*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment'] + - $constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraintUnvalidated'] + - joinOnConstraintID($dependents, $constraint, $table-id, $constraint-id) + - transient($dependents-Target, $constraint-Target) + - $dependents-Node[CurrentStatus] = TRANSIENT_ABSENT + - $constraint-Node[CurrentStatus] = TRANSIENT_ABSENT + - joinTargetNode($dependents, $dependents-Target, $dependents-Node) + - joinTargetNode($constraint, $constraint-Target, $constraint-Node) +- name: dependents removed right before simple constraint + from: dependents-Node + kind: SameStagePrecedence + to: constraint-Node + query: + - $dependents[Type] IN ['*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment'] + - $constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraintUnvalidated'] + - joinOnConstraintID($dependents, $constraint, $table-id, $constraint-id) + - $dependents-Target[TargetStatus] = TRANSIENT_ABSENT + - $dependents-Node[CurrentStatus] = TRANSIENT_ABSENT + - $constraint-Target[TargetStatus] = ABSENT + - $constraint-Node[CurrentStatus] = ABSENT + - joinTargetNode($dependents, $dependents-Target, $dependents-Node) + - joinTargetNode($constraint, $constraint-Target, $constraint-Node) +- name: dependents removed right before simple constraint + from: dependents-Node + kind: SameStagePrecedence + to: constraint-Node + query: + - $dependents[Type] IN ['*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment'] + - $constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraintUnvalidated'] + - joinOnConstraintID($dependents, $constraint, $table-id, $constraint-id) + - $dependents-Target[TargetStatus] = ABSENT + - $dependents-Node[CurrentStatus] = ABSENT + - $constraint-Target[TargetStatus] = TRANSIENT_ABSENT + - $constraint-Node[CurrentStatus] = TRANSIENT_ABSENT + - joinTargetNode($dependents, $dependents-Target, $dependents-Node) + - joinTargetNode($constraint, $constraint-Target, $constraint-Node) - name: descriptor drop right before removing dependent with attr ref from: referenced-descriptor-Node kind: SameStagePrecedence to: referencing-via-attr-Node query: - $referenced-descriptor[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - - $referencing-via-attr[Type] IN ['*scpb.ColumnFamily', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.SchemaComment', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] + - $referencing-via-attr[Type] IN ['*scpb.ColumnFamily', '*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraintUnvalidated', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.SchemaComment', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] - joinReferencedDescID($referencing-via-attr, $referenced-descriptor, $desc-id) - toAbsent($referenced-descriptor-Target, $referencing-via-attr-Target) - $referenced-descriptor-Node[CurrentStatus] = DROPPED @@ -2268,7 +2322,7 @@ deprules - $referenced-descriptor[Type] = '*scpb.Sequence' - $referenced-descriptor[DescID] = $seqID - $referencing-via-expr[ReferencedSequenceIDs] CONTAINS $seqID - - $referencing-via-expr[Type] IN ['*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SecondaryIndexPartial', '*scpb.FunctionParamDefaultExpression'] + - $referencing-via-expr[Type] IN ['*scpb.CheckConstraintUnvalidated', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SecondaryIndexPartial', '*scpb.FunctionParamDefaultExpression'] - toAbsent($referenced-descriptor-Target, $referencing-via-expr-Target) - $referenced-descriptor-Node[CurrentStatus] = DROPPED - $referencing-via-expr-Node[CurrentStatus] = ABSENT @@ -2282,7 +2336,7 @@ deprules - $referenced-descriptor[Type] IN ['*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType'] - $referenced-descriptor[DescID] = $fromDescID - $referencing-via-type[ReferencedTypeIDs] CONTAINS $fromDescID - - $referencing-via-type[Type] IN ['*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SecondaryIndexPartial', '*scpb.FunctionParamDefaultExpression'] + - $referencing-via-type[Type] IN ['*scpb.CheckConstraintUnvalidated', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SecondaryIndexPartial', '*scpb.FunctionParamDefaultExpression'] - toAbsent($referenced-descriptor-Target, $referencing-via-type-Target) - $referenced-descriptor-Node[CurrentStatus] = DROPPED - $referencing-via-type-Node[CurrentStatus] = ABSENT @@ -2294,7 +2348,7 @@ deprules to: dependent-Node query: - $descriptor[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] + - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraintUnvalidated', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] - joinOnDescID($descriptor, $dependent, $desc-id) - toAbsent($descriptor-Target, $dependent-Target) - $descriptor-Node[CurrentStatus] = DROPPED @@ -2333,7 +2387,7 @@ deprules to: dependent-Node query: - $relation[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TableData', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.ColumnNotNull', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.IndexData', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.DatabaseData', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] + - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraint', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraint', '*scpb.ForeignKeyConstraintUnvalidated', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TableData', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.ColumnNotNull', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.IndexData', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.DatabaseData', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] - joinOnDescID($relation, $dependent, $relation-id) - ToPublicOrTransient($relation-Target, $dependent-Target) - $relation-Node[CurrentStatus] = DESCRIPTOR_ADDED @@ -2691,7 +2745,7 @@ deprules kind: Precedence to: descriptor-Node query: - - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.ColumnNotNull', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] + - $dependent[Type] IN ['*scpb.ColumnFamily', '*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.UniqueWithoutIndexConstraintUnvalidated', '*scpb.CheckConstraint', '*scpb.CheckConstraintUnvalidated', '*scpb.ForeignKeyConstraint', '*scpb.ForeignKeyConstraintUnvalidated', '*scpb.TableComment', '*scpb.RowLevelTTL', '*scpb.TableZoneConfig', '*scpb.TablePartitioning', '*scpb.TableLocalityGlobal', '*scpb.TableLocalityPrimaryRegion', '*scpb.TableLocalitySecondaryRegion', '*scpb.TableLocalityRegionalByRow', '*scpb.ColumnName', '*scpb.ColumnType', '*scpb.ColumnDefaultExpression', '*scpb.ColumnOnUpdateExpression', '*scpb.SequenceOwner', '*scpb.ColumnComment', '*scpb.ColumnNotNull', '*scpb.IndexName', '*scpb.IndexPartitioning', '*scpb.SecondaryIndexPartial', '*scpb.IndexComment', '*scpb.IndexColumn', '*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment', '*scpb.Namespace', '*scpb.Owner', '*scpb.UserPrivileges', '*scpb.DatabaseRegionConfig', '*scpb.DatabaseRoleSetting', '*scpb.DatabaseComment', '*scpb.SchemaParent', '*scpb.SchemaComment', '*scpb.ObjectParent', '*scpb.EnumTypeValue', '*scpb.CompositeTypeAttrType', '*scpb.CompositeTypeAttrName', '*scpb.FunctionName', '*scpb.FunctionVolatility', '*scpb.FunctionLeakProof', '*scpb.FunctionNullInputBehavior', '*scpb.FunctionBody', '*scpb.FunctionParamDefaultExpression'] - $descriptor[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType', '*scpb.Function'] - joinOnDescID($dependent, $descriptor, $desc-id) - toAbsent($dependent-Target, $descriptor-Target) @@ -2887,7 +2941,7 @@ deprules to: constraint-Node query: - $descriptor[Type] = '*scpb.Table' - - $constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraint', '*scpb.ColumnNotNull'] + - $constraint[Type] = '*scpb.ColumnNotNull' - joinOnDescID($descriptor, $constraint, $desc-id) - toAbsent($descriptor-Target, $constraint-Target) - $descriptor-Node[CurrentStatus] = DROPPED @@ -3102,6 +3156,19 @@ deprules - isIndexKeyColumnKey(*scpb.IndexColumn)($index-column) - joinTargetNode($index, $index-Target, $index-Node) - joinTargetNode($column, $column-Target, $column-Node) +- name: simple constraint public right before its dependents + from: simple-constraint-Node + kind: SameStagePrecedence + to: dependent-Node + query: + - $simple-constraint[Type] = '*scpb.ColumnNotNull' + - $dependent[Type] IN ['*scpb.ConstraintWithoutIndexName', '*scpb.ConstraintComment'] + - joinOnConstraintID($simple-constraint, $dependent, $table-id, $constraint-id) + - ToPublicOrTransient($simple-constraint-Target, $dependent-Target) + - $simple-constraint-Node[CurrentStatus] = PUBLIC + - $dependent-Node[CurrentStatus] = PUBLIC + - joinTargetNode($simple-constraint, $simple-constraint-Target, $simple-constraint-Node) + - joinTargetNode($dependent, $dependent-Target, $dependent-Node) - name: swapped primary index public before column from: index-Node kind: Precedence diff --git a/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/oprules b/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/oprules index bf6b67837417..7d75810491d8 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/oprules +++ b/pkg/sql/schemachanger/scplan/internal/rules/current/testdata/oprules @@ -143,7 +143,7 @@ oprules from: constraint-Node query: - $relation[Type] IN ['*scpb.Table', '*scpb.View'] - - $constraint[Type] IN ['*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint', '*scpb.ColumnNotNull'] + - $constraint[Type] = '*scpb.ColumnNotNull' - joinOnDescID($relation, $constraint, $relation-id) - joinTarget($relation, $relation-Target) - $relation-Target[TargetStatus] = ABSENT diff --git a/pkg/sql/schemachanger/scplan/testdata/alter_table_add_check b/pkg/sql/schemachanger/scplan/testdata/alter_table_add_check index 3ca67250a759..39424720aedd 100644 --- a/pkg/sql/schemachanger/scplan/testdata/alter_table_add_check +++ b/pkg/sql/schemachanger/scplan/testdata/alter_table_add_check @@ -9,12 +9,13 @@ StatementPhase stage 1 of 1 with 1 MutationType op transitions: [[CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2}, PUBLIC], ABSENT] -> WRITE_ONLY ops: - *scop.MakeAbsentCheckConstraintWriteOnly + *scop.AddCheckConstraint CheckExpr: i > 0:::INT8 ColumnIDs: - 1 ConstraintID: 2 TableID: 104 + Validity: 2 PreCommitPhase stage 1 of 2 with 1 MutationType op transitions: [[CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2}, PUBLIC], WRITE_ONLY] -> ABSENT @@ -25,12 +26,13 @@ PreCommitPhase stage 2 of 2 with 3 MutationType ops transitions: [[CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2}, PUBLIC], ABSENT] -> WRITE_ONLY ops: - *scop.MakeAbsentCheckConstraintWriteOnly + *scop.AddCheckConstraint CheckExpr: i > 0:::INT8 ColumnIDs: - 1 ConstraintID: 2 TableID: 104 + Validity: 2 *scop.SetJobStateOnDescriptor DescriptorID: 104 Initialize: true @@ -91,4 +93,4 @@ ALTER TABLE t ADD CHECK (i > 0) - from: [ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2}, PUBLIC] to: [CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2}, PUBLIC] kind: SameStagePrecedence - rule: constraint dependent public right before constraint + rule: constraint dependent public right before complex constraint diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_table b/pkg/sql/schemachanger/scplan/testdata/drop_table index 8cf6e09b015a..57d74d0bdfd1 100644 --- a/pkg/sql/schemachanger/scplan/testdata/drop_table +++ b/pkg/sql/schemachanger/scplan/testdata/drop_table @@ -29,7 +29,7 @@ COMMENT ON CONSTRAINT fk_customers ON defaultdb.shipments IS 'customer is not go ops DROP TABLE defaultdb.shipments CASCADE; ---- -StatementPhase stage 1 of 1 with 5 MutationType ops +StatementPhase stage 1 of 1 with 7 MutationType ops transitions: [[ForeignKeyConstraint:{DescID: 109, IndexID: 0, ConstraintID: 2, ReferencedDescID: 104}, ABSENT], PUBLIC] -> VALIDATED [[ConstraintWithoutIndexName:{DescID: 109, Name: fk_customers, ConstraintID: 2}, ABSENT], PUBLIC] -> ABSENT @@ -55,9 +55,15 @@ StatementPhase stage 1 of 1 with 5 MutationType ops [[ColumnName:{DescID: 111, Name: tableoid, ColumnID: 4294967294}, ABSENT], PUBLIC] -> ABSENT [[ColumnType:{DescID: 111, ColumnFamilyID: 0, ColumnID: 4294967294}, ABSENT], PUBLIC] -> ABSENT ops: + *scop.MakePublicForeignKeyConstraintValidated + ConstraintID: 2 + TableID: 109 *scop.RemoveConstraintComment ConstraintID: 2 TableID: 109 + *scop.MakePublicForeignKeyConstraintValidated + ConstraintID: 3 + TableID: 109 *scop.MarkDescriptorAsDropped DescriptorID: 111 *scop.RemoveBackReferencesInRelations @@ -101,7 +107,7 @@ PreCommitPhase stage 1 of 2 with 1 MutationType op ops: *scop.UndoAllInTxnImmediateMutationOpSideEffects {} -PreCommitPhase stage 2 of 2 with 47 MutationType ops +PreCommitPhase stage 2 of 2 with 49 MutationType ops transitions: [[Namespace:{DescID: 109, Name: shipments, ReferencedDescID: 100}, ABSENT], PUBLIC] -> ABSENT [[Owner:{DescID: 109}, ABSENT], PUBLIC] -> ABSENT @@ -181,9 +187,15 @@ PreCommitPhase stage 2 of 2 with 47 MutationType ops [[ColumnName:{DescID: 111, Name: tableoid, ColumnID: 4294967294}, ABSENT], PUBLIC] -> ABSENT [[ColumnType:{DescID: 111, ColumnFamilyID: 0, ColumnID: 4294967294}, ABSENT], PUBLIC] -> ABSENT ops: + *scop.MakePublicForeignKeyConstraintValidated + ConstraintID: 2 + TableID: 109 *scop.RemoveConstraintComment ConstraintID: 2 TableID: 109 + *scop.MakePublicForeignKeyConstraintValidated + ConstraintID: 3 + TableID: 109 *scop.MarkDescriptorAsDropped DescriptorID: 110 *scop.RemoveObjectParent @@ -1481,11 +1493,14 @@ CREATE TABLE defaultdb.greeter ( ops DROP TABLE defaultdb.greeter ---- -StatementPhase stage 1 of 1 with no ops +StatementPhase stage 1 of 1 with 1 MutationType op transitions: [[CheckConstraint:{DescID: 114, ReferencedTypeIDs: [112 113], IndexID: 0, ConstraintID: 2}, ABSENT], PUBLIC] -> VALIDATED [[ConstraintWithoutIndexName:{DescID: 114, Name: check, ConstraintID: 2}, ABSENT], PUBLIC] -> ABSENT - no ops + ops: + *scop.MakePublicCheckConstraintValidated + ConstraintID: 2 + TableID: 114 PreCommitPhase stage 1 of 2 with 1 MutationType op transitions: [[CheckConstraint:{DescID: 114, ReferencedTypeIDs: [112 113], IndexID: 0, ConstraintID: 2}, ABSENT], VALIDATED] -> PUBLIC @@ -1493,7 +1508,7 @@ PreCommitPhase stage 1 of 2 with 1 MutationType op ops: *scop.UndoAllInTxnImmediateMutationOpSideEffects {} -PreCommitPhase stage 2 of 2 with 23 MutationType ops +PreCommitPhase stage 2 of 2 with 24 MutationType ops transitions: [[Namespace:{DescID: 114, Name: greeter, ReferencedDescID: 100}, ABSENT], PUBLIC] -> ABSENT [[Owner:{DescID: 114}, ABSENT], PUBLIC] -> ABSENT @@ -1534,6 +1549,9 @@ PreCommitPhase stage 2 of 2 with 23 MutationType ops [[CheckConstraint:{DescID: 114, ReferencedTypeIDs: [112 113], IndexID: 0, ConstraintID: 2}, ABSENT], PUBLIC] -> ABSENT [[ConstraintWithoutIndexName:{DescID: 114, Name: check, ConstraintID: 2}, ABSENT], PUBLIC] -> ABSENT ops: + *scop.MakePublicCheckConstraintValidated + ConstraintID: 2 + TableID: 114 *scop.RemoveCheckConstraint ConstraintID: 2 TableID: 114 diff --git a/pkg/sql/schemachanger/screl/attr.go b/pkg/sql/schemachanger/screl/attr.go index 48ba02405ae2..4d9bb152e8c6 100644 --- a/pkg/sql/schemachanger/screl/attr.go +++ b/pkg/sql/schemachanger/screl/attr.go @@ -178,6 +178,10 @@ var elementSchemaOptions = []rel.SchemaOption{ rel.EntityAttr(DescID, "TableID"), rel.EntityAttr(ConstraintID, "ConstraintID"), ), + rel.EntityMapping(t((*scpb.UniqueWithoutIndexConstraintUnvalidated)(nil)), + rel.EntityAttr(DescID, "TableID"), + rel.EntityAttr(ConstraintID, "ConstraintID"), + ), rel.EntityMapping(t((*scpb.CheckConstraint)(nil)), rel.EntityAttr(DescID, "TableID"), rel.EntityAttr(ConstraintID, "ConstraintID"), @@ -185,12 +189,23 @@ var elementSchemaOptions = []rel.SchemaOption{ rel.EntityAttr(ReferencedTypeIDs, "UsesTypeIDs"), rel.EntityAttr(IndexID, "IndexIDForValidation"), ), + rel.EntityMapping(t((*scpb.CheckConstraintUnvalidated)(nil)), + rel.EntityAttr(DescID, "TableID"), + rel.EntityAttr(ConstraintID, "ConstraintID"), + rel.EntityAttr(ReferencedSequenceIDs, "UsesSequenceIDs"), + rel.EntityAttr(ReferencedTypeIDs, "UsesTypeIDs"), + ), rel.EntityMapping(t((*scpb.ForeignKeyConstraint)(nil)), rel.EntityAttr(DescID, "TableID"), rel.EntityAttr(ReferencedDescID, "ReferencedTableID"), rel.EntityAttr(ConstraintID, "ConstraintID"), rel.EntityAttr(IndexID, "IndexIDForValidation"), ), + rel.EntityMapping(t((*scpb.ForeignKeyConstraintUnvalidated)(nil)), + rel.EntityAttr(DescID, "TableID"), + rel.EntityAttr(ReferencedDescID, "ReferencedTableID"), + rel.EntityAttr(ConstraintID, "ConstraintID"), + ), rel.EntityMapping(t((*scpb.RowLevelTTL)(nil)), rel.EntityAttr(DescID, "TableID"), ), diff --git a/pkg/sql/schemachanger/screl/scalars.go b/pkg/sql/schemachanger/screl/scalars.go index 82ab6df42a08..95330515b9de 100644 --- a/pkg/sql/schemachanger/screl/scalars.go +++ b/pkg/sql/schemachanger/screl/scalars.go @@ -124,7 +124,8 @@ func MinVersion(el scpb.Element) clusterversion.Key { *scpb.Function, *scpb.FunctionName, *scpb.FunctionVolatility, *scpb.FunctionLeakProof, *scpb.FunctionNullInputBehavior, *scpb.FunctionBody, *scpb.FunctionParamDefaultExpression: return clusterversion.V23_1 - case *scpb.ColumnNotNull: + case *scpb.ColumnNotNull, *scpb.CheckConstraintUnvalidated, + *scpb.UniqueWithoutIndexConstraintUnvalidated, *scpb.ForeignKeyConstraintUnvalidated: return clusterversion.V23_1 default: panic(errors.AssertionFailedf("unknown element %T", el)) diff --git a/pkg/sql/schemachanger/sctest_generated_test.go b/pkg/sql/schemachanger/sctest_generated_test.go index 3ee99c25c063..00129cbff071 100644 --- a/pkg/sql/schemachanger/sctest_generated_test.go +++ b/pkg/sql/schemachanger/sctest_generated_test.go @@ -120,6 +120,31 @@ func TestRollback_add_column_no_default(t *testing.T) { defer log.Scope(t).Close(t) sctest.Rollback(t, "pkg/sql/schemachanger/testdata/end_to_end/add_column_no_default", sctest.SingleNodeCluster) } +func TestEndToEndSideEffects_alter_table_add_check_unvalidated(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.EndToEndSideEffects(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated", sctest.SingleNodeCluster) +} +func TestExecuteWithDMLInjection_alter_table_add_check_unvalidated(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.ExecuteWithDMLInjection(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated", sctest.SingleNodeCluster) +} +func TestGenerateSchemaChangeCorpus_alter_table_add_check_unvalidated(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.GenerateSchemaChangeCorpus(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated", sctest.SingleNodeCluster) +} +func TestPause_alter_table_add_check_unvalidated(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.Pause(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated", sctest.SingleNodeCluster) +} +func TestRollback_alter_table_add_check_unvalidated(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + sctest.Rollback(t, "pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated", sctest.SingleNodeCluster) +} func TestEndToEndSideEffects_alter_table_add_check_vanilla(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated new file mode 100644 index 000000000000..0aa1883245fd --- /dev/null +++ b/pkg/sql/schemachanger/testdata/end_to_end/alter_table_add_check_unvalidated @@ -0,0 +1,80 @@ +setup +CREATE TABLE t (i INT PRIMARY KEY); +INSERT INTO t VALUES (0); +---- +... ++object {100 101 t} -> 104 + + +test +ALTER TABLE t ADD CHECK (i > 0) NOT VALID; +---- +begin transaction #1 +# begin StatementPhase +checking for feature: ALTER TABLE +increment telemetry for sql.schema.alter_table +increment telemetry for sql.schema.alter_table.add_constraint +write *eventpb.AlterTable to event log: + mutationId: 1 + sql: + descriptorId: 104 + statement: ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHECK (‹i› > ‹0›) NOT VALID + tag: ALTER TABLE + user: root + tableName: defaultdb.public.t +## StatementPhase stage 1 of 1 with 2 MutationType ops +upsert descriptor #104 + table: + + checks: + + - columnIds: + + - 1 + + constraintId: 2 + + expr: i > 0:::INT8 + + name: check_i + + validity: Unvalidated + columns: + - id: 1 + ... + name: t + nextColumnId: 2 + - nextConstraintId: 2 + + nextConstraintId: 3 + nextFamilyId: 1 + nextIndexId: 2 + ... + time: {} + unexposedParentSchemaId: 101 + - version: "1" + + version: "2" +# end StatementPhase +# begin PreCommitPhase +## PreCommitPhase stage 1 of 2 with 1 MutationType op +undo all catalog changes within txn #1 +persist all catalog changes to storage +## PreCommitPhase stage 2 of 2 with 2 MutationType ops +upsert descriptor #104 + table: + + checks: + + - columnIds: + + - 1 + + constraintId: 2 + + expr: i > 0:::INT8 + + name: check_i + + validity: Unvalidated + columns: + - id: 1 + ... + name: t + nextColumnId: 2 + - nextConstraintId: 2 + + nextConstraintId: 3 + nextFamilyId: 1 + nextIndexId: 2 + ... + time: {} + unexposedParentSchemaId: 101 + - version: "1" + + version: "2" +persist all catalog changes to storage +# end PreCommitPhase +commit transaction #1 diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_unvalidated b/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_unvalidated new file mode 100644 index 000000000000..7c7cc1290860 --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_unvalidated @@ -0,0 +1,30 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +INSERT INTO t VALUES (0); + +/* test */ +EXPLAIN (ddl) ALTER TABLE t ADD CHECK (i > 0) NOT VALID; +---- +Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHECK (‹i› > ‹0›) NOT VALID; + ├── StatementPhase + │ └── Stage 1 of 1 in StatementPhase + │ ├── 2 elements transitioning toward PUBLIC + │ │ ├── ABSENT → PUBLIC CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ └── ABSENT → PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ └── 2 Mutation operations + │ ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104,"Validity":1} + │ └── SetConstraintName {"ConstraintID":2,"Name":"check_i","TableID":104} + └── PreCommitPhase + ├── Stage 1 of 2 in PreCommitPhase + │ ├── 2 elements transitioning toward PUBLIC + │ │ ├── PUBLIC → ABSENT CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ └── PUBLIC → ABSENT ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ └── 1 Mutation operation + │ └── UndoAllInTxnImmediateMutationOpSideEffects + └── Stage 2 of 2 in PreCommitPhase + ├── 2 elements transitioning toward PUBLIC + │ ├── ABSENT → PUBLIC CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ └── ABSENT → PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + └── 2 Mutation operations + ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104,"Validity":1} + └── SetConstraintName {"ConstraintID":2,"Name":"check_i","TableID":104} diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_vanilla b/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_vanilla index 14d256a4d305..fc4103302277 100644 --- a/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_vanilla +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_vanilla @@ -11,7 +11,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2} │ └── 1 Mutation operation - │ └── MakeAbsentCheckConstraintWriteOnly {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104} + │ └── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104,"Validity":2} ├── PreCommitPhase │ ├── Stage 1 of 2 in PreCommitPhase │ │ ├── 1 element transitioning toward PUBLIC @@ -22,7 +22,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2} │ └── 3 Mutation operations - │ ├── MakeAbsentCheckConstraintWriteOnly {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104} + │ ├── AddCheckConstraint {"CheckExpr":"i \u003e 0:::INT8","ConstraintID":2,"TableID":104,"Validity":2} │ ├── SetJobStateOnDescriptor {"DescriptorID":104,"Initialize":true} │ └── CreateSchemaChangerJob {"RunningStatus":"PostCommitPhase ..."} └── PostCommitPhase diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_with_seq_and_udt b/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_with_seq_and_udt index fa287f7a365e..b772bfb84161 100644 --- a/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_with_seq_and_udt +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_add_check_with_seq_and_udt @@ -12,7 +12,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 107, ReferencedTypeIDs: [105 106], IndexID: 0, ConstraintID: 2, ReferencedSequenceIDs: [104]} │ └── 3 Mutation operations - │ ├── MakeAbsentCheckConstraintWriteOnly {"CheckExpr":"(i \u003e nextval(104...","ConstraintID":2,"TableID":107} + │ ├── AddCheckConstraint {"CheckExpr":"(i \u003e nextval(104...","ConstraintID":2,"TableID":107,"Validity":2} │ ├── UpdateTableBackReferencesInTypes {"BackReferencedTableID":107} │ └── UpdateTableBackReferencesInSequences {"BackReferencedTableID":107} ├── PreCommitPhase @@ -25,7 +25,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHEC │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY CheckConstraint:{DescID: 107, ReferencedTypeIDs: [105 106], IndexID: 0, ConstraintID: 2, ReferencedSequenceIDs: [104]} │ └── 8 Mutation operations - │ ├── MakeAbsentCheckConstraintWriteOnly {"CheckExpr":"(i \u003e nextval(104...","ConstraintID":2,"TableID":107} + │ ├── AddCheckConstraint {"CheckExpr":"(i \u003e nextval(104...","ConstraintID":2,"TableID":107,"Validity":2} │ ├── UpdateTableBackReferencesInTypes {"BackReferencedTableID":107} │ ├── UpdateTableBackReferencesInSequences {"BackReferencedTableID":107} │ ├── SetJobStateOnDescriptor {"DescriptorID":104,"Initialize":true} diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_add_foreign_key b/pkg/sql/schemachanger/testdata/explain/alter_table_add_foreign_key index 10fe4e9f0bc0..f01f6f97a85f 100644 --- a/pkg/sql/schemachanger/testdata/explain/alter_table_add_foreign_key +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_add_foreign_key @@ -11,7 +11,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t1› ADD CON │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY ForeignKeyConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2, ReferencedDescID: 105} │ └── 1 Mutation operation - │ └── MakeAbsentForeignKeyConstraintWriteOnly {"ConstraintID":2,"ReferencedTableID":105,"TableID":104} + │ └── AddForeignKeyConstraint {"ConstraintID":2,"ReferencedTableID":105,"TableID":104,"Validity":2} ├── PreCommitPhase │ ├── Stage 1 of 2 in PreCommitPhase │ │ ├── 1 element transitioning toward PUBLIC @@ -22,7 +22,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t1› ADD CON │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY ForeignKeyConstraint:{DescID: 104, IndexID: 0, ConstraintID: 2, ReferencedDescID: 105} │ └── 4 Mutation operations - │ ├── MakeAbsentForeignKeyConstraintWriteOnly {"ConstraintID":2,"ReferencedTableID":105,"TableID":104} + │ ├── AddForeignKeyConstraint {"ConstraintID":2,"ReferencedTableID":105,"TableID":104,"Validity":2} │ ├── SetJobStateOnDescriptor {"DescriptorID":104,"Initialize":true} │ ├── SetJobStateOnDescriptor {"DescriptorID":105,"Initialize":true} │ └── CreateSchemaChangerJob {"RunningStatus":"PostCommitPhase ..."} diff --git a/pkg/sql/schemachanger/testdata/explain/alter_table_add_unique_without_index b/pkg/sql/schemachanger/testdata/explain/alter_table_add_unique_without_index index e2bb7f8bc22b..886e161a8cbe 100644 --- a/pkg/sql/schemachanger/testdata/explain/alter_table_add_unique_without_index +++ b/pkg/sql/schemachanger/testdata/explain/alter_table_add_unique_without_index @@ -11,7 +11,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CONS │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY UniqueWithoutIndexConstraint:{DescID: 104, ConstraintID: 2} │ └── 1 Mutation operation - │ └── MakeAbsentUniqueWithoutIndexConstraintWriteOnly {"ConstraintID":2,"TableID":104} + │ └── AddUniqueWithoutIndexConstraint {"ConstraintID":2,"TableID":104,"Validity":2} ├── PreCommitPhase │ ├── Stage 1 of 2 in PreCommitPhase │ │ ├── 1 element transitioning toward PUBLIC @@ -22,7 +22,7 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CONS │ ├── 1 element transitioning toward PUBLIC │ │ └── ABSENT → WRITE_ONLY UniqueWithoutIndexConstraint:{DescID: 104, ConstraintID: 2} │ └── 3 Mutation operations - │ ├── MakeAbsentUniqueWithoutIndexConstraintWriteOnly {"ConstraintID":2,"TableID":104} + │ ├── AddUniqueWithoutIndexConstraint {"ConstraintID":2,"TableID":104,"Validity":2} │ ├── SetJobStateOnDescriptor {"DescriptorID":104,"Initialize":true} │ └── CreateSchemaChangerJob {"RunningStatus":"PostCommitPhase ..."} └── PostCommitPhase diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_unvalidated b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_unvalidated new file mode 100644 index 000000000000..54c2d0acc07a --- /dev/null +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_unvalidated @@ -0,0 +1,77 @@ +/* setup */ +CREATE TABLE t (i INT PRIMARY KEY); +INSERT INTO t VALUES (0); + +/* test */ +EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > 0) NOT VALID; +---- +• Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› ADD CHECK (‹i› > ‹0›) NOT VALID; +│ +├── • StatementPhase +│ │ +│ └── • Stage 1 of 1 in StatementPhase +│ │ +│ ├── • 2 elements transitioning toward PUBLIC +│ │ │ +│ │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} +│ │ │ ABSENT → PUBLIC +│ │ │ +│ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} +│ │ ABSENT → PUBLIC +│ │ +│ └── • 2 Mutation operations +│ │ +│ ├── • AddCheckConstraint +│ │ CheckExpr: i > 0:::INT8 +│ │ ColumnIDs: +│ │ - 1 +│ │ ConstraintID: 2 +│ │ TableID: 104 +│ │ Validity: 1 +│ │ +│ └── • SetConstraintName +│ ConstraintID: 2 +│ Name: check_i +│ TableID: 104 +│ +└── • PreCommitPhase + │ + ├── • Stage 1 of 2 in PreCommitPhase + │ │ + │ ├── • 2 elements transitioning toward PUBLIC + │ │ │ + │ │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ │ PUBLIC → ABSENT + │ │ │ + │ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ │ PUBLIC → ABSENT + │ │ + │ └── • 1 Mutation operation + │ │ + │ └── • UndoAllInTxnImmediateMutationOpSideEffects + │ {} + │ + └── • Stage 2 of 2 in PreCommitPhase + │ + ├── • 2 elements transitioning toward PUBLIC + │ │ + │ ├── • CheckConstraintUnvalidated:{DescID: 104, ConstraintID: 2} + │ │ ABSENT → PUBLIC + │ │ + │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} + │ ABSENT → PUBLIC + │ + └── • 2 Mutation operations + │ + ├── • AddCheckConstraint + │ CheckExpr: i > 0:::INT8 + │ ColumnIDs: + │ - 1 + │ ConstraintID: 2 + │ TableID: 104 + │ Validity: 1 + │ + └── • SetConstraintName + ConstraintID: 2 + Name: check_i + TableID: 104 diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_vanilla b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_vanilla index d5353238abb5..101b147d5492 100644 --- a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_vanilla +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_vanilla @@ -21,12 +21,13 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > 0) │ │ │ └── • 1 Mutation operation │ │ -│ └── • MakeAbsentCheckConstraintWriteOnly +│ └── • AddCheckConstraint │ CheckExpr: i > 0:::INT8 │ ColumnIDs: │ - 1 │ ConstraintID: 2 │ TableID: 104 +│ Validity: 2 │ ├── • PreCommitPhase │ │ @@ -54,12 +55,13 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > 0) │ │ │ └── • 3 Mutation operations │ │ -│ ├── • MakeAbsentCheckConstraintWriteOnly +│ ├── • AddCheckConstraint │ │ CheckExpr: i > 0:::INT8 │ │ ColumnIDs: │ │ - 1 │ │ ConstraintID: 2 │ │ TableID: 104 +│ │ Validity: 2 │ │ │ ├── • SetJobStateOnDescriptor │ │ DescriptorID: 104 @@ -106,7 +108,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > 0) │ │ │ rule: "CheckConstraint transitions to PUBLIC uphold 2-version invariant: VALIDATED->PUBLIC" │ │ │ │ │ └── • SameStagePrecedence dependency from PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} - │ │ rule: "constraint dependent public right before constraint" + │ │ rule: "constraint dependent public right before complex constraint" │ │ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: check_i, ConstraintID: 2} │ ABSENT → PUBLIC diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_with_seq_and_udt b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_with_seq_and_udt index 7ef1c09073c8..d9ed6731285c 100644 --- a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_with_seq_and_udt +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_check_with_seq_and_udt @@ -22,13 +22,14 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > nextval('s') OR j::typ = 'a' │ │ │ └── • 3 Mutation operations │ │ -│ ├── • MakeAbsentCheckConstraintWriteOnly +│ ├── • AddCheckConstraint │ │ CheckExpr: (i > nextval(104:::REGCLASS)) OR (j::@100105 = b'@':::@100105) │ │ ColumnIDs: │ │ - 1 │ │ - 2 │ │ ConstraintID: 2 │ │ TableID: 107 +│ │ Validity: 2 │ │ │ ├── • UpdateTableBackReferencesInTypes │ │ BackReferencedTableID: 107 @@ -67,13 +68,14 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > nextval('s') OR j::typ = 'a' │ │ │ └── • 8 Mutation operations │ │ -│ ├── • MakeAbsentCheckConstraintWriteOnly +│ ├── • AddCheckConstraint │ │ CheckExpr: (i > nextval(104:::REGCLASS)) OR (j::@100105 = b'@':::@100105) │ │ ColumnIDs: │ │ - 1 │ │ - 2 │ │ ConstraintID: 2 │ │ TableID: 107 +│ │ Validity: 2 │ │ │ ├── • UpdateTableBackReferencesInTypes │ │ BackReferencedTableID: 107 @@ -147,7 +149,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD CHECK (i > nextval('s') OR j::typ = 'a' │ │ │ rule: "CheckConstraint transitions to PUBLIC uphold 2-version invariant: VALIDATED->PUBLIC" │ │ │ │ │ └── • SameStagePrecedence dependency from PUBLIC ConstraintWithoutIndexName:{DescID: 107, Name: check_i_j, ConstraintID: 2} - │ │ rule: "constraint dependent public right before constraint" + │ │ rule: "constraint dependent public right before complex constraint" │ │ │ └── • ConstraintWithoutIndexName:{DescID: 107, Name: check_i_j, ConstraintID: 2} │ ABSENT → PUBLIC diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_foreign_key b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_foreign_key index 2a44850c363e..b3941af0a06e 100644 --- a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_foreign_key +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_foreign_key @@ -21,7 +21,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i); │ │ │ └── • 1 Mutation operation │ │ -│ └── • MakeAbsentForeignKeyConstraintWriteOnly +│ └── • AddForeignKeyConstraint │ ColumnIDs: │ - 1 │ ConstraintID: 2 @@ -29,6 +29,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i); │ - 1 │ ReferencedTableID: 105 │ TableID: 104 +│ Validity: 2 │ ├── • PreCommitPhase │ │ @@ -56,7 +57,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i); │ │ │ └── • 4 Mutation operations │ │ -│ ├── • MakeAbsentForeignKeyConstraintWriteOnly +│ ├── • AddForeignKeyConstraint │ │ ColumnIDs: │ │ - 1 │ │ ConstraintID: 2 @@ -64,6 +65,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i); │ │ - 1 │ │ ReferencedTableID: 105 │ │ TableID: 104 +│ │ Validity: 2 │ │ │ ├── • SetJobStateOnDescriptor │ │ DescriptorID: 104 @@ -116,7 +118,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t1 ADD FOREIGN KEY (i) REFERENCES t2(i); │ │ │ rule: "ForeignKeyConstraint transitions to PUBLIC uphold 2-version invariant: VALIDATED->PUBLIC" │ │ │ │ │ └── • SameStagePrecedence dependency from PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: t1_i_fkey, ConstraintID: 2} - │ │ rule: "constraint dependent public right before constraint" + │ │ rule: "constraint dependent public right before complex constraint" │ │ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: t1_i_fkey, ConstraintID: 2} │ ABSENT → PUBLIC diff --git a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_unique_without_index b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_unique_without_index index 2fe495788b0c..6d113045ede4 100644 --- a/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_unique_without_index +++ b/pkg/sql/schemachanger/testdata/explain_verbose/alter_table_add_unique_without_index @@ -21,11 +21,12 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD UNIQUE WITHOUT INDEX (j); │ │ │ └── • 1 Mutation operation │ │ -│ └── • MakeAbsentUniqueWithoutIndexConstraintWriteOnly +│ └── • AddUniqueWithoutIndexConstraint │ ColumnIDs: │ - 2 │ ConstraintID: 2 │ TableID: 104 +│ Validity: 2 │ ├── • PreCommitPhase │ │ @@ -53,11 +54,12 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD UNIQUE WITHOUT INDEX (j); │ │ │ └── • 3 Mutation operations │ │ -│ ├── • MakeAbsentUniqueWithoutIndexConstraintWriteOnly +│ ├── • AddUniqueWithoutIndexConstraint │ │ ColumnIDs: │ │ - 2 │ │ ConstraintID: 2 │ │ TableID: 104 +│ │ Validity: 2 │ │ │ ├── • SetJobStateOnDescriptor │ │ DescriptorID: 104 @@ -105,7 +107,7 @@ EXPLAIN (ddl, verbose) ALTER TABLE t ADD UNIQUE WITHOUT INDEX (j); │ │ │ rule: "UniqueWithoutIndexConstraint transitions to PUBLIC uphold 2-version invariant: VALIDATED->PUBLIC" │ │ │ │ │ └── • SameStagePrecedence dependency from PUBLIC ConstraintWithoutIndexName:{DescID: 104, Name: unique_j, ConstraintID: 2} - │ │ rule: "constraint dependent public right before constraint" + │ │ rule: "constraint dependent public right before complex constraint" │ │ │ └── • ConstraintWithoutIndexName:{DescID: 104, Name: unique_j, ConstraintID: 2} │ ABSENT → PUBLIC diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go index 5cbb8001195d..92e6f9ef9f0d 100644 --- a/pkg/storage/engine.go +++ b/pkg/storage/engine.go @@ -951,8 +951,9 @@ type Engine interface { fs.FS // CreateCheckpoint creates a checkpoint of the engine in the given directory, // which must not exist. The directory should be on the same file system so - // that hard links can be used. - CreateCheckpoint(dir string) error + // that hard links can be used. If spans is not empty, the checkpoint excludes + // SSTs that don't overlap with any of these key spans. + CreateCheckpoint(dir string, spans []roachpb.Span) error // SetMinVersion is used to signal to the engine the current minimum // version that it must maintain compatibility with. diff --git a/pkg/storage/engine_test.go b/pkg/storage/engine_test.go index b3ae15ed3f92..14ef62c9f0d0 100644 --- a/pkg/storage/engine_test.go +++ b/pkg/storage/engine_test.go @@ -1008,12 +1008,12 @@ func TestCreateCheckpoint(t *testing.T) { dir = filepath.Join(dir, "checkpoint") assert.NoError(t, err) - assert.NoError(t, db.CreateCheckpoint(dir)) + assert.NoError(t, db.CreateCheckpoint(dir, nil)) assert.DirExists(t, dir) m, err := filepath.Glob(dir + "/*") assert.NoError(t, err) assert.True(t, len(m) > 0) - if err := db.CreateCheckpoint(dir); !testutils.IsError(err, "exists") { + if err := db.CreateCheckpoint(dir, nil); !testutils.IsError(err, "exists") { t.Fatal(err) } } diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index e555d148fea3..1b32fecf23f1 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -1886,9 +1886,36 @@ func (p *Pebble) Stat(name string) (os.FileInfo, error) { return p.fs.Stat(name) } +func checkpointSpansNote(spans []roachpb.Span) []byte { + note := "CRDB spans:\n" + for _, span := range spans { + note += span.String() + "\n" + } + return []byte(note) +} + // CreateCheckpoint implements the Engine interface. -func (p *Pebble) CreateCheckpoint(dir string) error { - return p.db.Checkpoint(dir, pebble.WithFlushedWAL()) +func (p *Pebble) CreateCheckpoint(dir string, spans []roachpb.Span) error { + opts := []pebble.CheckpointOption{ + pebble.WithFlushedWAL(), + } + if l := len(spans); l > 0 { + s := make([]pebble.CheckpointSpan, 0, l) + for _, span := range spans { + s = append(s, pebble.CheckpointSpan{Start: span.Key, End: span.EndKey}) + } + opts = append(opts, pebble.WithRestrictToSpans(s)) + } + if err := p.db.Checkpoint(dir, opts...); err != nil { + return err + } + + // TODO(#90543, cockroachdb/pebble#2285): move spans info to Pebble manifest. + if len(spans) > 0 { + return fs.SafeWriteToFile(p.fs, dir, p.fs.PathJoin(dir, "checkpoint.txt"), + checkpointSpansNote(spans)) + } + return nil } // SetMinVersion implements the Engine interface.