From 10c2cf9699d7876a812769373eb6f00082249c08 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 13 Jul 2022 14:53:10 -0700 Subject: [PATCH 1/7] colflow: prevent deadlocks when many queries spill to disk at same time This commit fixes a long-standing issue which could cause memory-intensive queries to deadlock on acquiring the file descriptors quota when vectorized execution spills to disk. This bug has been present since the introduction of disk-spilling (over two and a half years ago, introduced in #45318 and partially mitigated in #45892), but we haven't seen this in any user reports, only in `tpch_concurrency` roachtest runs, so the severity seems pretty minor. Consider the following query plan: ``` Node 1 Node 2 TableReader TableReader | | HashRouter HashRouter | \ ___________ / | | \/__________ | | / \ | HashAggregator HashAggregator ``` and let's imagine that each hash aggregator has to spill to disk. This would require acquiring the file descriptors quota. Now, imagine that because of that hash aggregators' spilling, each of the hash routers has slow outputs causing them to spill too. As a result, this query plan can require `A + 2 * R` number of FDs of a single node to succeed where `A` is the quota for a single hash aggregator (equal to 16 - with the default value of `COCKROACH_VEC_MAX_OPEN_FDS` environment variable which is 256) and `R` is the quota for a single router output (2). This means that we can estimate that 20 FDs from each node are needed for the query to finish execution with 16 FDs being acquired first. Now imagine that this query is run with concurrency of 16. We can end up in such a situation that all hash aggregators have spilled, fully exhausting the global node limit on each node, so whenever the hash router outputs need to spill, they block forever since no FDs will ever be released, until a query is canceled or a node is shutdown. In other words, we have a deadlock. This commit fixes this situation by introducing a retry mechanism to exponentially backoff when trying to acquire the FD quota, until a time out. The randomizations provided by the `retry` package should be sufficient so that some of the queries succeed while others result in an error. Unfortunately, I don't see a way to prevent this deadlock from occurring in the first place without possible increase in latency in some case. The difficult thing is that we currently acquire FDs only once we need them, meaning once a particular component spills to disk. We could acquire the maximum number of FDs that a query might need up-front, before the query execution starts, but that could lead to starvation of the queries that ultimately won't spill to disk. This seems like a much worse impact than receiving timeout errors on some analytical queries when run with high concurrency. We're not an OLAP database, so this behavior seems ok. Release note (bug fix): Previously, CockroachDB could deadlock when evaluating analytical queries f multiple queries had to spill to disk at the same time. This is now fixed by making some of the queries error out instead. --- pkg/sql/colexec/colbuilder/execplan.go | 1 + pkg/sql/colexec/colexecdisk/external_sort.go | 4 + .../colexecdisk/hash_based_partitioner.go | 3 + pkg/sql/colflow/BUILD.bazel | 6 ++ pkg/sql/colflow/vectorized_flow.go | 99 ++++++++++++++----- .../colflow/vectorized_flow_deadlock_test.go | 77 +++++++++++++++ pkg/sql/execinfra/server_config.go | 9 ++ 7 files changed, 175 insertions(+), 24 deletions(-) create mode 100644 pkg/sql/colflow/vectorized_flow_deadlock_test.go diff --git a/pkg/sql/colexec/colbuilder/execplan.go b/pkg/sql/colexec/colbuilder/execplan.go index b28c1504bed3..b560fcfb5dff 100644 --- a/pkg/sql/colexec/colbuilder/execplan.go +++ b/pkg/sql/colexec/colbuilder/execplan.go @@ -436,6 +436,7 @@ func (r opResult) createDiskBackedSort( args.DiskQueueCfg, args.FDSemaphore, diskAccount, + flowCtx.TestingKnobs().VecFDsToAcquire, ) r.ToClose = append(r.ToClose, es.(colexecop.Closer)) return es diff --git a/pkg/sql/colexec/colexecdisk/external_sort.go b/pkg/sql/colexec/colexecdisk/external_sort.go index af250af7b7a2..728cce659880 100644 --- a/pkg/sql/colexec/colexecdisk/external_sort.go +++ b/pkg/sql/colexec/colexecdisk/external_sort.go @@ -232,6 +232,7 @@ func NewExternalSorter( diskQueueCfg colcontainer.DiskQueueCfg, fdSemaphore semaphore.Semaphore, diskAcc *mon.BoundAccount, + testingVecFDsToAcquire int, ) colexecop.Operator { if diskQueueCfg.BufferSizeBytes > 0 && maxNumberPartitions == 0 { // With the default limit of 256 file descriptors, this results in 16 @@ -243,6 +244,9 @@ func NewExternalSorter( if maxNumberPartitions < colexecop.ExternalSorterMinPartitions { maxNumberPartitions = colexecop.ExternalSorterMinPartitions } + if testingVecFDsToAcquire > 0 { + maxNumberPartitions = testingVecFDsToAcquire + } if memoryLimit == 1 { // If memory limit is 1, we're likely in a "force disk spill" // scenario, but we don't want to artificially limit batches when we diff --git a/pkg/sql/colexec/colexecdisk/hash_based_partitioner.go b/pkg/sql/colexec/colexecdisk/hash_based_partitioner.go index 57236774916c..6b4d75515b66 100644 --- a/pkg/sql/colexec/colexecdisk/hash_based_partitioner.go +++ b/pkg/sql/colexec/colexecdisk/hash_based_partitioner.go @@ -308,6 +308,9 @@ func calculateMaxNumberActivePartitions( if maxNumberActivePartitions < numRequiredActivePartitions { maxNumberActivePartitions = numRequiredActivePartitions } + if toAcquire := flowCtx.TestingKnobs().VecFDsToAcquire; toAcquire > 0 { + maxNumberActivePartitions = toAcquire + } return maxNumberActivePartitions } diff --git a/pkg/sql/colflow/BUILD.bazel b/pkg/sql/colflow/BUILD.bazel index fc7abb1c484a..d790e2f1ccb1 100644 --- a/pkg/sql/colflow/BUILD.bazel +++ b/pkg/sql/colflow/BUILD.bazel @@ -36,6 +36,8 @@ go_library( "//pkg/sql/execinfrapb", "//pkg/sql/execstats", "//pkg/sql/flowinfra", + "//pkg/sql/pgwire/pgcode", + "//pkg/sql/pgwire/pgerror", "//pkg/sql/rowenc", "//pkg/sql/rowexec", "//pkg/sql/sessiondatapb", @@ -48,6 +50,7 @@ go_library( "//pkg/util/mon", "//pkg/util/optional", "//pkg/util/randutil", + "//pkg/util/retry", "//pkg/util/syncutil", "//pkg/util/timeutil", "//pkg/util/tracing", @@ -69,6 +72,7 @@ go_test( "main_test.go", "routers_test.go", "stats_test.go", + "vectorized_flow_deadlock_test.go", "vectorized_flow_planning_test.go", "vectorized_flow_shutdown_test.go", "vectorized_flow_space_test.go", @@ -120,6 +124,8 @@ go_test( "//pkg/testutils/testcluster", "//pkg/util/admission", "//pkg/util/buildutil", + "//pkg/util/cancelchecker", + "//pkg/util/envutil", "//pkg/util/hlc", "//pkg/util/humanizeutil", "//pkg/util/leaktest", diff --git a/pkg/sql/colflow/vectorized_flow.go b/pkg/sql/colflow/vectorized_flow.go index 0d9439ae8596..5afcd810b241 100644 --- a/pkg/sql/colflow/vectorized_flow.go +++ b/pkg/sql/colflow/vectorized_flow.go @@ -37,6 +37,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/execstats" "github.com/cockroachdb/cockroach/pkg/sql/flowinfra" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/rowexec" "github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -47,6 +49,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/metric" "github.com/cockroachdb/cockroach/pkg/util/mon" "github.com/cockroachdb/cockroach/pkg/util/optional" + "github.com/cockroachdb/cockroach/pkg/util/retry" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" @@ -55,39 +58,84 @@ import ( "github.com/marusama/semaphore" ) -// countingSemaphore is a semaphore that keeps track of the semaphore count from -// its perspective. -// Note that it effectively implements the execinfra.Releasable interface but -// due to the method name conflict doesn't. -type countingSemaphore struct { +// fdCountingSemaphore is a semaphore that keeps track of the number of file +// descriptors currently used by the vectorized engine. +// +// Note that it effectively implements the execreleasable.Releasable interface +// but due to the method name conflict doesn't. +type fdCountingSemaphore struct { semaphore.Semaphore - globalCount *metric.Gauge - count int64 + globalCount *metric.Gauge + count int64 + testingAcquireMaxRetries int } -var countingSemaphorePool = sync.Pool{ +var fdCountingSemaphorePool = sync.Pool{ New: func() interface{} { - return &countingSemaphore{} + return &fdCountingSemaphore{} }, } -func newCountingSemaphore(sem semaphore.Semaphore, globalCount *metric.Gauge) *countingSemaphore { - s := countingSemaphorePool.Get().(*countingSemaphore) +func newFDCountingSemaphore( + sem semaphore.Semaphore, globalCount *metric.Gauge, testingAcquireMaxRetries int, +) *fdCountingSemaphore { + s := fdCountingSemaphorePool.Get().(*fdCountingSemaphore) s.Semaphore = sem s.globalCount = globalCount + s.testingAcquireMaxRetries = testingAcquireMaxRetries return s } -func (s *countingSemaphore) Acquire(ctx context.Context, n int) error { - if err := s.Semaphore.Acquire(ctx, n); err != nil { - return err +var errAcquireTimeout = pgerror.New( + pgcode.ConfigurationLimitExceeded, + "acquiring of file descriptors timed out, consider increasing "+ + "COCKROACH_VEC_MAX_OPEN_FDS environment variable", +) + +func (s *fdCountingSemaphore) Acquire(ctx context.Context, n int) error { + if s.TryAcquire(n) { + return nil } - atomic.AddInt64(&s.count, int64(n)) - s.globalCount.Inc(int64(n)) - return nil + // Currently there is not enough capacity in the semaphore to acquire the + // desired number, so we set up a retry loop that exponentially backs off, + // until either the semaphore opens up or we time out (most likely due to a + // deadlock). + // + // The latter situation is possible when multiple queries already hold some + // of the quota and each of them needs more to proceed resulting in a + // deadlock. We get out of such a deadlock by randomly erroring out one of + // the queries (which would release some quota back to the semaphore) making + // it possible for other queries to proceed. + // + // Note that we've already tried to acquire the quota above (which failed), + // so the initial backoff time of 100ms seems ok (we are spilling to disk + // after all, so the query is likely to experience significant latency). The + // current choice of options is such that we'll spend on the order of 25s + // in the retry loop before timing out. + maxRetries := s.testingAcquireMaxRetries + if maxRetries <= 0 { + // Make sure that the retry loop is finite. + maxRetries = 8 + } + opts := retry.Options{ + InitialBackoff: 100 * time.Millisecond, + Multiplier: 2.0, + RandomizationFactor: 0.25, + MaxRetries: maxRetries, + } + for r := retry.StartWithCtx(ctx, opts); r.Next(); { + if s.TryAcquire(n) { + return nil + } + } + if ctx.Err() != nil { + return ctx.Err() + } + log.Warning(ctx, "acquiring of file descriptors for disk-spilling timed out") + return errAcquireTimeout } -func (s *countingSemaphore) TryAcquire(n int) bool { +func (s *fdCountingSemaphore) TryAcquire(n int) bool { success := s.Semaphore.TryAcquire(n) if !success { return false @@ -97,7 +145,7 @@ func (s *countingSemaphore) TryAcquire(n int) bool { return success } -func (s *countingSemaphore) Release(n int) int { +func (s *fdCountingSemaphore) Release(n int) int { atomic.AddInt64(&s.count, int64(-n)) s.globalCount.Dec(int64(n)) return s.Semaphore.Release(n) @@ -106,12 +154,12 @@ func (s *countingSemaphore) Release(n int) int { // ReleaseToPool should be named Release and should implement the // execinfra.Releasable interface, but that would lead to a conflict with // semaphore.Semaphore.Release method. -func (s *countingSemaphore) ReleaseToPool() { +func (s *fdCountingSemaphore) ReleaseToPool() { if unreleased := atomic.LoadInt64(&s.count); unreleased != 0 { colexecerror.InternalError(errors.Newf("unexpectedly %d count on the semaphore when releasing it to the pool", unreleased)) } - *s = countingSemaphore{} - countingSemaphorePool.Put(s) + *s = fdCountingSemaphore{} + fdCountingSemaphorePool.Put(s) } type vectorizedFlow struct { @@ -130,7 +178,7 @@ type vectorizedFlow struct { // of the number of resources held in a semaphore.Semaphore requested from the // context of this flow so that these can be released unconditionally upon // Cleanup. - countingSemaphore *countingSemaphore + countingSemaphore *fdCountingSemaphore tempStorage struct { syncutil.Mutex @@ -198,8 +246,11 @@ func (f *vectorizedFlow) Setup( if err := diskQueueCfg.EnsureDefaults(); err != nil { return ctx, nil, err } - f.countingSemaphore = newCountingSemaphore(f.Cfg.VecFDSemaphore, f.Cfg.Metrics.VecOpenFDs) flowCtx := f.GetFlowCtx() + f.countingSemaphore = newFDCountingSemaphore( + f.Cfg.VecFDSemaphore, f.Cfg.Metrics.VecOpenFDs, + flowCtx.TestingKnobs().VecFDsAcquireMaxRetriesCount, + ) f.creator = newVectorizedFlowCreator( helper, vectorizedRemoteComponentCreator{}, diff --git a/pkg/sql/colflow/vectorized_flow_deadlock_test.go b/pkg/sql/colflow/vectorized_flow_deadlock_test.go new file mode 100644 index 000000000000..c823f50dc856 --- /dev/null +++ b/pkg/sql/colflow/vectorized_flow_deadlock_test.go @@ -0,0 +1,77 @@ +// Copyright 2022 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 colflow_test + +import ( + "context" + "strconv" + "strings" + "testing" + "time" + + "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/sql/execinfra" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" + "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/cockroach/pkg/util/cancelchecker" + "github.com/cockroachdb/cockroach/pkg/util/envutil" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/timeutil" + "github.com/stretchr/testify/require" +) + +// TestVectorizedFlowDeadlocksWhenSpilling is a regression test for the +// vectorized flow being deadlocked when multiple operators have to spill to +// disk exhausting the file descriptor limit. +func TestVectorizedFlowDeadlocksWhenSpilling(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + skip.UnderStressRace(t, "the test is too slow under stressrace") + + vecFDsLimit := 8 + envutil.TestSetEnv(t, "COCKROACH_VEC_MAX_OPEN_FDS", strconv.Itoa(vecFDsLimit)) + serverArgs := base.TestServerArgs{ + Knobs: base.TestingKnobs{DistSQL: &execinfra.TestingKnobs{ + // Set the testing knob so that the first operator to spill would + // use up the whole FD limit. + VecFDsToAcquire: vecFDsLimit, + // Allow just one retry to speed up the test. + VecFDsAcquireMaxRetriesCount: 1, + }}, + } + tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ServerArgs: serverArgs}) + ctx := context.Background() + defer tc.Stopper().Stop(ctx) + conn := tc.Conns[0] + + _, err := conn.ExecContext(ctx, "CREATE TABLE t (a, b) AS SELECT i, i FROM generate_series(1, 10000) AS g(i)") + require.NoError(t, err) + // Lower the workmem budget so that all buffering operators have to spill to + // disk. + _, err = conn.ExecContext(ctx, "SET distsql_workmem = '1KiB'") + require.NoError(t, err) + + queryCtx, queryCtxCancel := context.WithDeadline(ctx, timeutil.Now().Add(10*time.Second)) + defer queryCtxCancel() + // Run a query with a hash joiner feeding into a hash aggregator, with both + // operators spilling to disk. We expect that the hash aggregator won't be + // able to spill though since the FD limit has been used up, and we'd like + // to see the query timing out (when acquiring the file descriptor quota) + // rather than being canceled due to the context deadline. + query := "SELECT max(a) FROM (SELECT t1.a, t1.b FROM t AS t1 INNER HASH JOIN t AS t2 ON t1.a = t2.b) GROUP BY b" + _, err = conn.ExecContext(queryCtx, query) + // We expect an error that is different from the query cancellation (which + // is what SQL layer returns on a context cancellation). + require.NotNil(t, err) + require.False(t, strings.Contains(err.Error(), cancelchecker.QueryCanceledError.Error())) +} diff --git a/pkg/sql/execinfra/server_config.go b/pkg/sql/execinfra/server_config.go index 85bc8384f937..3c7009864a64 100644 --- a/pkg/sql/execinfra/server_config.go +++ b/pkg/sql/execinfra/server_config.go @@ -244,6 +244,15 @@ type TestingKnobs struct { // Cannot be set together with ForceDiskSpill. MemoryLimitBytes int64 + // VecFDsToAcquire, if positive, indicates the number of file descriptors + // that should be acquired by a single disk-spilling operator in the + // vectorized engine. + VecFDsToAcquire int + // VecFDsAcquireMaxRetriesCount, if positive, determines the maximum number + // of retries done when acquiring the file descriptors for a disk-spilling + // operator in the vectorized engine. + VecFDsAcquireMaxRetriesCount int + // TableReaderBatchBytesLimit, if not 0, overrides the limit that the // TableReader will set on the size of results it wants to get for individual // requests. From 6189bb3ce7be13edbc13c47cd6d608246aaf1562 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 14 Jul 2022 07:47:13 -0700 Subject: [PATCH 2/7] roachtest: remove some debugging printouts in tpch_concurrency This was added to track down the deadlock fixed in the previous commit, so we no longer need it. Release note: None --- pkg/cmd/roachtest/tests/tpch_concurrency.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/pkg/cmd/roachtest/tests/tpch_concurrency.go b/pkg/cmd/roachtest/tests/tpch_concurrency.go index 1eb8e8d79b09..02a8b2ddc6e9 100644 --- a/pkg/cmd/roachtest/tests/tpch_concurrency.go +++ b/pkg/cmd/roachtest/tests/tpch_concurrency.go @@ -13,7 +13,6 @@ package tests import ( "context" "fmt" - "strings" "time" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster" @@ -107,22 +106,6 @@ func registerTPCHConcurrency(r registry.Registry) { // Run each query once on each connection. for queryNum := 1; queryNum <= tpch.NumQueries; queryNum++ { t.Status("running Q", queryNum) - // To aid during the debugging later, we'll print the DistSQL - // diagram of the query. - rows, err := conn.Query("EXPLAIN (DISTSQL) " + tpch.QueriesByNumber[queryNum]) - if err != nil { - return err - } - defer rows.Close() - for rows.Next() { - var line string - if err = rows.Scan(&line); err != nil { - t.Fatal(err) - } - if strings.Contains(line, "Diagram:") { - t.Status(line) - } - } // The way --max-ops flag works is as follows: the global ops // counter is incremented **after** each worker completes a // single operation, so it is possible for all connections start From 43b509f2ff5ac611917bd4ae434fb3beb114af62 Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Thu, 14 Jul 2022 12:07:41 -0400 Subject: [PATCH 3/7] sql/schemachanger/scplan/internal/opgen: track op type information This change reworks opgen to require the emit functions to specify the type of op they'll emit. This allows us to statically determine the op type and then use that to determine whether there are any remaining ops which might fail. Release note: None --- .../testdata/logic_test/new_schema_changer | 8 +-- .../scplan/internal/opgen/op_funcs.go | 53 +++++++++++++++---- .../scplan/internal/opgen/op_gen.go | 2 +- .../scplan/internal/opgen/opgen_alias_type.go | 12 ++--- .../internal/opgen/opgen_check_constraint.go | 8 +-- .../scplan/internal/opgen/opgen_column.go | 18 +++---- .../internal/opgen/opgen_column_comment.go | 8 +-- .../opgen/opgen_column_default_expression.go | 12 ++--- .../internal/opgen/opgen_column_family.go | 4 +- .../internal/opgen/opgen_column_name.go | 4 +- .../opgen_column_on_update_expression.go | 12 ++--- .../internal/opgen/opgen_column_type.go | 8 +-- .../opgen/opgen_constraint_comment.go | 8 +-- .../internal/opgen/opgen_constraint_name.go | 4 +- .../scplan/internal/opgen/opgen_database.go | 14 ++--- .../internal/opgen/opgen_database_comment.go | 8 +-- .../opgen/opgen_database_role_setting.go | 4 +- .../scplan/internal/opgen/opgen_enum_type.go | 12 ++--- .../internal/opgen/opgen_enum_type_value.go | 4 +- .../opgen/opgen_foreign_key_constraint.go | 6 +-- .../internal/opgen/opgen_index_column.go | 8 +-- .../internal/opgen/opgen_index_comment.go | 8 +-- .../scplan/internal/opgen/opgen_index_name.go | 4 +- .../opgen/opgen_index_partitioning.go | 2 +- .../scplan/internal/opgen/opgen_namespace.go | 4 +- .../internal/opgen/opgen_object_parent.go | 2 +- .../scplan/internal/opgen/opgen_owner.go | 4 +- .../internal/opgen/opgen_primary_index.go | 22 ++++---- .../internal/opgen/opgen_row_level_ttl.go | 4 +- .../scplan/internal/opgen/opgen_schema.go | 12 ++--- .../internal/opgen/opgen_schema_comment.go | 6 +-- .../internal/opgen/opgen_schema_parent.go | 4 +- .../internal/opgen/opgen_secondary_index.go | 24 ++++----- .../opgen/opgen_secondary_index_partial.go | 8 +-- .../scplan/internal/opgen/opgen_sequence.go | 14 ++--- .../internal/opgen/opgen_sequence_owner.go | 6 +-- .../scplan/internal/opgen/opgen_table.go | 14 ++--- .../internal/opgen/opgen_table_comment.go | 8 +-- .../opgen/opgen_table_locality_global.go | 2 +- .../opgen_table_locality_primary_region.go | 2 +- .../opgen_table_locality_regional_by_row.go | 2 +- .../opgen_table_locality_secondary_region.go | 4 +- .../internal/opgen/opgen_temporary_index.go | 10 ++-- .../opgen_unique_without_index_constraint.go | 4 +- .../internal/opgen/opgen_user_privileges.go | 4 +- .../scplan/internal/opgen/opgen_view.go | 20 +++---- .../scplan/internal/opgen/target.go | 38 +++++++++++-- .../scplan/internal/scgraph/edge.go | 24 +++++++-- .../scplan/internal/scgraph/graph.go | 7 ++- .../scplan/internal/scgraph/graph_test.go | 5 +- .../schemachanger/scplan/testdata/alter_table | 40 +++++++------- .../scplan/testdata/create_index | 24 ++++----- .../schemachanger/scplan/testdata/drop_index | 18 +++---- 53 files changed, 324 insertions(+), 243 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/new_schema_changer b/pkg/sql/logictest/testdata/logic_test/new_schema_changer index fd59d000e2fe..4397faf76b18 100644 --- a/pkg/sql/logictest/testdata/logic_test/new_schema_changer +++ b/pkg/sql/logictest/testdata/logic_test/new_schema_changer @@ -91,7 +91,7 @@ EXPLAIN (DDL, VERBOSE) ALTER TABLE foo ADD COLUMN j INT │ │ oid: 20 │ │ width: 64 │ │ -│ └── • scop.AddColumnToIndex +│ └── • AddColumnToIndex │ ColumnID: 2 │ IndexID: 1 │ Kind: 2 @@ -174,6 +174,7 @@ EXPLAIN (DDL, VERBOSE) ALTER TABLE foo ADD COLUMN j INT JobID: 1 RunningStatus: all stages completed + statement ok ALTER TABLE foo ADD COLUMN j INT @@ -1969,8 +1970,8 @@ Schema change plan for DROP INDEX ‹test›.public.‹parent›@‹idx› CASCA │ ├── RemoveAllTableComments {"TableID":266} │ ├── MakeDroppedIndexDeleteOnly {"IndexID":2,"TableID":265} │ ├── DrainDescriptorName {"Namespace":{"DatabaseID":104,"DescriptorID":266,"Name":"child","SchemaID":105}} - │ ├── scop.RemoveColumnFromIndex {"ColumnID":2,"IndexID":2,"TableID":265} - │ ├── scop.RemoveColumnFromIndex {"ColumnID":1,"IndexID":2,"Kind":1,"TableID":265} + │ ├── RemoveColumnFromIndex {"ColumnID":2,"IndexID":2,"TableID":265} + │ ├── RemoveColumnFromIndex {"ColumnID":1,"IndexID":2,"Kind":1,"TableID":265} │ ├── SetJobStateOnDescriptor {"DescriptorID":265} │ ├── SetJobStateOnDescriptor {"DescriptorID":266} │ └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"PostCommitNonRev..."} @@ -1995,6 +1996,7 @@ Schema change plan for DROP INDEX ‹test›.public.‹parent›@‹idx› CASCA └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"all stages compl..."} + statement ok SET use_declarative_schema_changer = 'on' diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go b/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go index 742acfc2b8ce..a102fdf63b6d 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/op_funcs.go @@ -100,12 +100,19 @@ func makeTargetsWithElementMap(cs scpb.CurrentState) targetsWithElementMap { // given an element value. type opsFunc func(element scpb.Element, md *targetsWithElementMap) []scop.Op -func makeOpsFunc(el scpb.Element, fns []interface{}) (opsFunc, error) { +func makeOpsFunc(el scpb.Element, fns []interface{}) (opsFunc, scop.Type, error) { + var opType scop.Type var funcValues []reflect.Value for _, fn := range fns { - if err := checkOpFunc(el, fn); err != nil { - return nil, err + typ, err := checkOpFunc(el, fn) + if err != nil { + return nil, 0, err } + if len(funcValues) > 0 && typ != opType { + return nil, 0, errors.Errorf("conflicting operation types for %T: %s != %s", + el, opType, typ) + } + opType = typ funcValues = append(funcValues, reflect.ValueOf(fn)) } return func(element scpb.Element, md *targetsWithElementMap) []scop.Op { @@ -124,16 +131,21 @@ func makeOpsFunc(el scpb.Element, fns []interface{}) (opsFunc, error) { } } return ret - }, nil + }, opType, nil } -var opType = reflect.TypeOf((*scop.Op)(nil)).Elem() +var ( + opInterfaceType = reflect.TypeOf((*scop.Op)(nil)).Elem() + mutationOpInterfaceType = reflect.TypeOf((*scop.MutationOp)(nil)).Elem() + validationOpInterfaceType = reflect.TypeOf((*scop.ValidationOp)(nil)).Elem() + backfillOpInterfaceType = reflect.TypeOf((*scop.BackfillOp)(nil)).Elem() +) -func checkOpFunc(el scpb.Element, fn interface{}) error { +func checkOpFunc(el scpb.Element, fn interface{}) (opType scop.Type, _ error) { fnV := reflect.ValueOf(fn) fnT := fnV.Type() if fnT.Kind() != reflect.Func { - return errors.Errorf( + return 0, errors.Errorf( "%v is a %s, expected %s", fnT, fnT.Kind(), reflect.Func, ) } @@ -141,14 +153,33 @@ func checkOpFunc(el scpb.Element, fn interface{}) error { if !(fnT.NumIn() == 1 && fnT.In(0) == elType) && !(fnT.NumIn() == 2 && fnT.In(0) == elType && fnT.In(1) == reflect.TypeOf((*targetsWithElementMap)(nil))) { - return errors.Errorf( + return 0, errors.Errorf( "expected %v to be a func with one argument of type %s", fnT, elType, ) } - if fnT.NumOut() != 1 || !fnT.Out(0).Implements(opType) { + returnTypeError := func() error { return errors.Errorf( - "expected %v to be a func with one return value of type %s", fnT, opType, + "expected %v to be a func with one return value of a "+ + "pointer type which implements %s", fnT, opType, ) } - return nil + if fnT.NumOut() != 1 { + return 0, returnTypeError() + } + out := fnT.Out(0) + if out.Kind() != reflect.Ptr || !out.Implements(opInterfaceType) { + return 0, returnTypeError() + } + switch { + case out.Implements(mutationOpInterfaceType): + opType = scop.MutationType + case out.Implements(validationOpInterfaceType): + opType = scop.ValidationType + case out.Implements(backfillOpInterfaceType): + opType = scop.BackfillType + default: + return 0, errors.AssertionFailedf("%s implemented %s but does not conform to any known type", + out, opInterfaceType) + } + return opType, nil } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/op_gen.go b/pkg/sql/schemachanger/scplan/internal/opgen/op_gen.go index df3b82111fb5..688022d636d0 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/op_gen.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/op_gen.go @@ -78,7 +78,7 @@ func (r *registry) buildGraph(cs scpb.CurrentState) (_ *scgraph.Graph, err error ops = e.ops(e.n.Element(), &md) } if err := g.AddOpEdges( - e.n.Target, e.from, e.to, e.revertible, e.minPhase, ops..., + e.n.Target, e.from, e.to, e.revertible, e.canFail, e.minPhase, ops..., ); err != nil { return nil, err } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_alias_type.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_alias_type.go index 216a8d499cc0..da1f1441887f 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_alias_type.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_alias_type.go @@ -21,12 +21,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.AliasType) scop.Op { + emit(func(this *scpb.AliasType) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.AliasType) scop.Op { + emit(func(this *scpb.AliasType) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.TypeID, } @@ -36,7 +36,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.AliasType, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.AliasType, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.TypeID, Reason: statementForDropJob(this, md).Statement, @@ -45,17 +45,17 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.AliasType) scop.Op { + emit(func(this *scpb.AliasType) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.TypeID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.AliasType, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.AliasType, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.AliasType) scop.Op { + emit(func(this *scpb.AliasType) *scop.DeleteDescriptor { return &scop.DeleteDescriptor{ DescriptorID: this.TypeID, } 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 037b86148b43..df2104452e0c 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_check_constraint.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.CheckConstraint) scop.Op { + emit(func(this *scpb.CheckConstraint) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,13 +30,13 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.CheckConstraint) scop.Op { + emit(func(this *scpb.CheckConstraint) *scop.RemoveCheckConstraint { return &scop.RemoveCheckConstraint{ TableID: this.TableID, ConstraintID: this.ConstraintID, } }), - emit(func(this *scpb.CheckConstraint) scop.Op { + emit(func(this *scpb.CheckConstraint) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -45,7 +45,7 @@ func init() { BackReferencedTableID: this.TableID, } }), - emit(func(this *scpb.CheckConstraint) scop.Op { + emit(func(this *scpb.CheckConstraint) *scop.UpdateBackReferencesInSequences { if len(this.UsesSequenceIDs) == 0 { return nil } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column.go index e5e6bc89afb7..a44b411b3361 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column.go @@ -21,17 +21,17 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_DELETE_ONLY, - emit(func(this *scpb.Column) scop.Op { + emit(func(this *scpb.Column) *scop.MakeAddedColumnDeleteOnly { return &scop.MakeAddedColumnDeleteOnly{ Column: *protoutil.Clone(this).(*scpb.Column), } }), - emit(func(this *scpb.Column, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Column, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), to(scpb.Status_WRITE_ONLY, - emit(func(this *scpb.Column) scop.Op { + emit(func(this *scpb.Column) *scop.MakeAddedColumnDeleteAndWriteOnly { return &scop.MakeAddedColumnDeleteAndWriteOnly{ TableID: this.TableID, ColumnID: this.ColumnID, @@ -39,14 +39,14 @@ func init() { }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.Column, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Column, md *targetsWithElementMap) *scop.MakeColumnPublic { return &scop.MakeColumnPublic{ EventBase: newLogEventBase(this, md), TableID: this.TableID, ColumnID: this.ColumnID, } }), - emit(func(this *scpb.Column) scop.Op { + emit(func(this *scpb.Column) *scop.RefreshStats { return &scop.RefreshStats{ TableID: this.TableID, } @@ -56,19 +56,19 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_WRITE_ONLY, - emit(func(this *scpb.Column) scop.Op { + emit(func(this *scpb.Column) *scop.MakeDroppedColumnDeleteAndWriteOnly { return &scop.MakeDroppedColumnDeleteAndWriteOnly{ TableID: this.TableID, ColumnID: this.ColumnID, } }), - emit(func(this *scpb.Column, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Column, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), to(scpb.Status_DELETE_ONLY, revertible(false), - emit(func(this *scpb.Column) scop.Op { + emit(func(this *scpb.Column) *scop.MakeDroppedColumnDeleteOnly { return &scop.MakeDroppedColumnDeleteOnly{ TableID: this.TableID, ColumnID: this.ColumnID, @@ -76,7 +76,7 @@ func init() { }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.Column, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Column, md *targetsWithElementMap) *scop.MakeColumnAbsent { return &scop.MakeColumnAbsent{ EventBase: newLogEventBase(this, md), TableID: this.TableID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_comment.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_comment.go index 2809793f63a5..6b2e9d73fe03 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_comment.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_comment.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ColumnComment) scop.Op { + emit(func(this *scpb.ColumnComment) *scop.UpsertColumnComment { return &scop.UpsertColumnComment{ TableID: this.TableID, ColumnID: this.ColumnID, @@ -28,7 +28,7 @@ func init() { PGAttributeNum: this.PgAttributeNum, } }), - emit(func(this *scpb.ColumnComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.ColumnComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), @@ -36,14 +36,14 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.ColumnComment) scop.Op { + emit(func(this *scpb.ColumnComment) *scop.RemoveColumnComment { return &scop.RemoveColumnComment{ TableID: this.TableID, ColumnID: this.ColumnID, PgAttributeNum: this.PgAttributeNum, } }), - emit(func(this *scpb.ColumnComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.ColumnComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_default_expression.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_default_expression.go index 9fe938ed97a6..679e864acd44 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_default_expression.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_default_expression.go @@ -21,12 +21,12 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ColumnDefaultExpression) scop.Op { + emit(func(this *scpb.ColumnDefaultExpression) *scop.AddColumnDefaultExpression { return &scop.AddColumnDefaultExpression{ Default: *protoutil.Clone(this).(*scpb.ColumnDefaultExpression), } }), - emit(func(this *scpb.ColumnDefaultExpression) scop.Op { + emit(func(this *scpb.ColumnDefaultExpression) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -35,7 +35,7 @@ func init() { BackReferencedTableID: this.TableID, } }), - emit(func(this *scpb.ColumnDefaultExpression) scop.Op { + emit(func(this *scpb.ColumnDefaultExpression) *scop.UpdateBackReferencesInSequences { if len(this.UsesSequenceIDs) == 0 { return nil } @@ -50,13 +50,13 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.ColumnDefaultExpression) scop.Op { + emit(func(this *scpb.ColumnDefaultExpression) *scop.RemoveColumnDefaultExpression { return &scop.RemoveColumnDefaultExpression{ TableID: this.TableID, ColumnID: this.ColumnID, } }), - emit(func(this *scpb.ColumnDefaultExpression) scop.Op { + emit(func(this *scpb.ColumnDefaultExpression) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -65,7 +65,7 @@ func init() { BackReferencedTableID: this.TableID, } }), - emit(func(this *scpb.ColumnDefaultExpression) scop.Op { + emit(func(this *scpb.ColumnDefaultExpression) *scop.UpdateBackReferencesInSequences { if len(this.UsesSequenceIDs) == 0 { return nil } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_family.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_family.go index e9535ef2e42e..252ad04259e9 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_family.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_family.go @@ -21,7 +21,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ColumnFamily) scop.Op { + emit(func(this *scpb.ColumnFamily) *scop.AddColumnFamily { return &scop.AddColumnFamily{ TableID: this.TableID, FamilyID: this.FamilyID, @@ -34,7 +34,7 @@ func init() { scpb.Status_PUBLIC, to(scpb.Status_ABSENT, revertible(false), - emit(func(this *scpb.ColumnFamily) scop.Op { + emit(func(this *scpb.ColumnFamily) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_name.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_name.go index 5c0eeb440a90..a7097dd33ccf 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_name.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_name.go @@ -22,7 +22,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ColumnName) scop.Op { + emit(func(this *scpb.ColumnName) *scop.SetColumnName { return &scop.SetColumnName{ TableID: this.TableID, ColumnID: this.ColumnID, @@ -34,7 +34,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.ColumnName) scop.Op { + emit(func(this *scpb.ColumnName) *scop.SetColumnName { return &scop.SetColumnName{ TableID: this.TableID, ColumnID: this.ColumnID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_on_update_expression.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_on_update_expression.go index 5642a7b6f6cb..942de9d2e8ad 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_on_update_expression.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_on_update_expression.go @@ -21,12 +21,12 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ColumnOnUpdateExpression) scop.Op { + emit(func(this *scpb.ColumnOnUpdateExpression) *scop.AddColumnOnUpdateExpression { return &scop.AddColumnOnUpdateExpression{ OnUpdate: *protoutil.Clone(this).(*scpb.ColumnOnUpdateExpression), } }), - emit(func(this *scpb.ColumnOnUpdateExpression) scop.Op { + emit(func(this *scpb.ColumnOnUpdateExpression) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -35,7 +35,7 @@ func init() { BackReferencedTableID: this.TableID, } }), - emit(func(this *scpb.ColumnOnUpdateExpression) scop.Op { + emit(func(this *scpb.ColumnOnUpdateExpression) *scop.UpdateBackReferencesInSequences { if len(this.UsesSequenceIDs) == 0 { return nil } @@ -50,13 +50,13 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.ColumnOnUpdateExpression) scop.Op { + emit(func(this *scpb.ColumnOnUpdateExpression) *scop.RemoveColumnOnUpdateExpression { return &scop.RemoveColumnOnUpdateExpression{ TableID: this.TableID, ColumnID: this.ColumnID, } }), - emit(func(this *scpb.ColumnOnUpdateExpression) scop.Op { + emit(func(this *scpb.ColumnOnUpdateExpression) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -65,7 +65,7 @@ func init() { BackReferencedTableID: this.TableID, } }), - emit(func(this *scpb.ColumnOnUpdateExpression) scop.Op { + emit(func(this *scpb.ColumnOnUpdateExpression) *scop.UpdateBackReferencesInSequences { if len(this.UsesSequenceIDs) == 0 { return nil } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_type.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_type.go index 10b078ae0781..6bdd332ee3cc 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_type.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_type.go @@ -23,12 +23,12 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ColumnType) scop.Op { + emit(func(this *scpb.ColumnType) *scop.SetAddedColumnType { return &scop.SetAddedColumnType{ ColumnType: *protoutil.Clone(this).(*scpb.ColumnType), } }), - emit(func(this *scpb.ColumnType) scop.Op { + emit(func(this *scpb.ColumnType) *scop.UpdateTableBackReferencesInTypes { if ids := referencedTypeIDs(this); len(ids) > 0 { return &scop.UpdateTableBackReferencesInTypes{ TypeIDs: ids, @@ -43,7 +43,7 @@ func init() { scpb.Status_PUBLIC, to(scpb.Status_ABSENT, revertible(false), - emit(func(this *scpb.ColumnType) scop.Op { + emit(func(this *scpb.ColumnType) *scop.RemoveDroppedColumnType { if ids := referencedTypeIDs(this); len(ids) > 0 { return &scop.RemoveDroppedColumnType{ TableID: this.TableID, @@ -52,7 +52,7 @@ func init() { } return nil }), - emit(func(this *scpb.ColumnType) scop.Op { + emit(func(this *scpb.ColumnType) *scop.UpdateTableBackReferencesInTypes { if ids := referencedTypeIDs(this); len(ids) > 0 { return &scop.UpdateTableBackReferencesInTypes{ TypeIDs: ids, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_comment.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_comment.go index d4320c36b864..af35a029dccb 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_comment.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_comment.go @@ -20,14 +20,14 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ConstraintComment) scop.Op { + emit(func(this *scpb.ConstraintComment) *scop.UpsertConstraintComment { return &scop.UpsertConstraintComment{ TableID: this.TableID, ConstraintID: this.ConstraintID, Comment: this.Comment, } }), - emit(func(this *scpb.ConstraintComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.ConstraintComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), @@ -35,13 +35,13 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.ConstraintComment) scop.Op { + emit(func(this *scpb.ConstraintComment) *scop.RemoveConstraintComment { return &scop.RemoveConstraintComment{ TableID: this.TableID, ConstraintID: this.ConstraintID, } }), - emit(func(this *scpb.ConstraintComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.ConstraintComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_name.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_name.go index ed152b7af3cd..fe806801ea37 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_name.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_constraint_name.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ConstraintName) scop.Op { + emit(func(this *scpb.ConstraintName) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,7 +30,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.ConstraintName) scop.Op { + emit(func(this *scpb.ConstraintName) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database.go index 24267f2ef24e..a5e92248ca10 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database.go @@ -21,12 +21,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.Database) scop.Op { + emit(func(this *scpb.Database) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.Database) scop.Op { + emit(func(this *scpb.Database) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.DatabaseID, } @@ -36,7 +36,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.Database, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Database, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.DatabaseID, Reason: statementForDropJob(this, md).Statement, @@ -45,23 +45,23 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.Database) scop.Op { + emit(func(this *scpb.Database) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.DatabaseID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.Database, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Database, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.Database, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Database, md *targetsWithElementMap) *scop.CreateGcJobForDatabase { return &scop.CreateGcJobForDatabase{ DatabaseID: this.DatabaseID, StatementForDropJob: statementForDropJob(this, md), } }), - emit(func(this *scpb.Database) scop.Op { + emit(func(this *scpb.Database) *scop.DeleteDescriptor { return &scop.DeleteDescriptor{ DescriptorID: this.DatabaseID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_comment.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_comment.go index 05aafbd1dc33..de73a1fc46b1 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_comment.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_comment.go @@ -20,13 +20,13 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.DatabaseComment) scop.Op { + emit(func(this *scpb.DatabaseComment) *scop.UpsertDatabaseComment { return &scop.UpsertDatabaseComment{ DatabaseID: this.DatabaseID, Comment: this.Comment, } }), - emit(func(this *scpb.DatabaseComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.DatabaseComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), @@ -34,12 +34,12 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.DatabaseComment) scop.Op { + emit(func(this *scpb.DatabaseComment) *scop.RemoveDatabaseComment { return &scop.RemoveDatabaseComment{ DatabaseID: this.DatabaseID, } }), - emit(func(this *scpb.DatabaseComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.DatabaseComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_role_setting.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_role_setting.go index 6941e3629e0f..3244dd55e7ca 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_role_setting.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_database_role_setting.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.DatabaseRoleSetting) scop.Op { + emit(func(this *scpb.DatabaseRoleSetting) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,7 +30,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.DatabaseRoleSetting) scop.Op { + emit(func(this *scpb.DatabaseRoleSetting) *scop.RemoveDatabaseRoleSettings { return &scop.RemoveDatabaseRoleSettings{ DatabaseID: this.DatabaseID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type.go index b3ece53f8620..d749d40be3e6 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type.go @@ -21,12 +21,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.EnumType) scop.Op { + emit(func(this *scpb.EnumType) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.EnumType) scop.Op { + emit(func(this *scpb.EnumType) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.TypeID, } @@ -36,7 +36,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.EnumType, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.EnumType, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.TypeID, Reason: statementForDropJob(this, md).Statement, @@ -45,17 +45,17 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.EnumType) scop.Op { + emit(func(this *scpb.EnumType) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.TypeID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.EnumType, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.EnumType, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.EnumType) scop.Op { + emit(func(this *scpb.EnumType) *scop.DeleteDescriptor { return &scop.DeleteDescriptor{ DescriptorID: this.TypeID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type_value.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type_value.go index 3f6124da8745..cea682e4060b 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type_value.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_enum_type_value.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.EnumTypeValue) scop.Op { + emit(func(this *scpb.EnumTypeValue) *scop.NotImplemented { return notImplemented(this) }), ), @@ -28,7 +28,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.EnumTypeValue) scop.Op { + emit(func(this *scpb.EnumTypeValue) *scop.NotImplemented { return notImplemented(this) }), ), 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 288295dd8f66..fa71fd44bfe1 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 @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ForeignKeyConstraint) scop.Op { + emit(func(this *scpb.ForeignKeyConstraint) *scop.NotImplemented { return notImplemented(this) }), ), @@ -35,14 +35,14 @@ func init() { // table do not match. We therefore have to identify the constraint // by name in the origin table descriptor to be able to remove the // back-reference. - emit(func(this *scpb.ForeignKeyConstraint) scop.Op { + emit(func(this *scpb.ForeignKeyConstraint) *scop.RemoveForeignKeyBackReference { return &scop.RemoveForeignKeyBackReference{ ReferencedTableID: this.ReferencedTableID, OriginTableID: this.TableID, OriginConstraintID: this.ConstraintID, } }), - emit(func(this *scpb.ForeignKeyConstraint) scop.Op { + emit(func(this *scpb.ForeignKeyConstraint) *scop.RemoveForeignKeyConstraint { return &scop.RemoveForeignKeyConstraint{ TableID: this.TableID, ConstraintID: this.ConstraintID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_column.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_column.go index 9a9a4e309650..0ae827350eaf 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_column.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_column.go @@ -19,8 +19,8 @@ func init() { opRegistry.register((*scpb.IndexColumn)(nil), toPublic( scpb.Status_ABSENT, - to(scpb.Status_PUBLIC, emit(func(column *scpb.IndexColumn) scop.Op { - return scop.AddColumnToIndex{ + to(scpb.Status_PUBLIC, emit(func(column *scpb.IndexColumn) *scop.AddColumnToIndex { + return &scop.AddColumnToIndex{ TableID: column.TableID, ColumnID: column.ColumnID, IndexID: column.IndexID, @@ -34,8 +34,8 @@ func init() { scpb.Status_PUBLIC, to(scpb.Status_ABSENT, revertible(false), - emit(func(column *scpb.IndexColumn) scop.Op { - return scop.RemoveColumnFromIndex{ + emit(func(column *scpb.IndexColumn) *scop.RemoveColumnFromIndex { + return &scop.RemoveColumnFromIndex{ TableID: column.TableID, ColumnID: column.ColumnID, IndexID: column.IndexID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_comment.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_comment.go index 0ec0eda4fa31..e186f52abe8a 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_comment.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_comment.go @@ -20,14 +20,14 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.IndexComment) scop.Op { + emit(func(this *scpb.IndexComment) *scop.UpsertIndexComment { return &scop.UpsertIndexComment{ TableID: this.TableID, IndexID: this.IndexID, Comment: this.Comment, } }), - emit(func(this *scpb.IndexComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.IndexComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), @@ -35,13 +35,13 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.IndexComment) scop.Op { + emit(func(this *scpb.IndexComment) *scop.RemoveIndexComment { return &scop.RemoveIndexComment{ TableID: this.TableID, IndexID: this.IndexID, } }), - emit(func(this *scpb.IndexComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.IndexComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_name.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_name.go index 264bca5aaa59..01326f3c95af 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_name.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_name.go @@ -22,7 +22,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.IndexName) scop.Op { + emit(func(this *scpb.IndexName) *scop.SetIndexName { return &scop.SetIndexName{ TableID: this.TableID, IndexID: this.IndexID, @@ -34,7 +34,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.IndexName) scop.Op { + emit(func(this *scpb.IndexName) *scop.SetIndexName { return &scop.SetIndexName{ TableID: this.TableID, IndexID: this.IndexID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_partitioning.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_partitioning.go index e3fc17ed90f0..bea597fbe76b 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_partitioning.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_index_partitioning.go @@ -31,7 +31,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.IndexPartitioning) scop.Op { + emit(func(this *scpb.IndexPartitioning) *scop.AddIndexPartitionInfo { return &scop.AddIndexPartitionInfo{ Partitioning: *protoutil.Clone(this).(*scpb.IndexPartitioning), } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_namespace.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_namespace.go index 806edf6f9571..cc3731332cdc 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_namespace.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_namespace.go @@ -22,7 +22,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.Namespace) scop.Op { + emit(func(this *scpb.Namespace) *scop.NotImplemented { return notImplemented(this) }), ), @@ -32,7 +32,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.Namespace) scop.Op { + emit(func(this *scpb.Namespace) *scop.DrainDescriptorName { return &scop.DrainDescriptorName{ Namespace: *protoutil.Clone(this).(*scpb.Namespace), } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_object_parent.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_object_parent.go index 40defb9882c8..781b54d6fc7c 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_object_parent.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_object_parent.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.ObjectParent) scop.Op { + emit(func(this *scpb.ObjectParent) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_owner.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_owner.go index 0fa6f14f777d..aeee0aefb3c8 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_owner.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_owner.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.Owner) scop.Op { + emit(func(this *scpb.Owner) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,7 +30,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.Owner) scop.Op { + emit(func(this *scpb.Owner) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go index aea3bbcbbab7..d46620e4e798 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_primary_index.go @@ -21,14 +21,14 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_BACKFILL_ONLY, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.MakeAddedIndexBackfilling { return &scop.MakeAddedIndexBackfilling{ Index: *protoutil.Clone(&this.Index).(*scpb.Index), } }), ), to(scpb.Status_BACKFILLED, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.BackfillIndex { return &scop.BackfillIndex{ TableID: this.TableID, SourceIndexID: this.SourceIndexID, @@ -37,7 +37,7 @@ func init() { }), ), to(scpb.Status_DELETE_ONLY, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.MakeBackfillingIndexDeleteOnly { return &scop.MakeBackfillingIndexDeleteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -45,7 +45,7 @@ func init() { }), ), to(scpb.Status_MERGE_ONLY, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.MakeAddedIndexDeleteAndWriteOnly { return &scop.MakeAddedIndexDeleteAndWriteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -54,7 +54,7 @@ func init() { ), equiv(scpb.Status_WRITE_ONLY), to(scpb.Status_MERGED, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.MergeIndex { return &scop.MergeIndex{ TableID: this.TableID, TemporaryIndexID: this.TemporaryIndexID, @@ -63,7 +63,7 @@ func init() { }), ), to(scpb.Status_VALIDATED, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.ValidateUniqueIndex { return &scop.ValidateUniqueIndex{ TableID: this.TableID, IndexID: this.IndexID, @@ -71,7 +71,7 @@ func init() { }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.PrimaryIndex, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.PrimaryIndex, md *targetsWithElementMap) *scop.MakeAddedPrimaryIndexPublic { return &scop.MakeAddedPrimaryIndexPublic{ EventBase: newLogEventBase(this, md), TableID: this.TableID, @@ -83,7 +83,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_VALIDATED, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.MakeDroppedPrimaryIndexDeleteAndWriteOnly { // Most of this logic is taken from MakeMutationComplete(). return &scop.MakeDroppedPrimaryIndexDeleteAndWriteOnly{ TableID: this.TableID, @@ -97,7 +97,7 @@ func init() { equiv(scpb.Status_MERGE_ONLY), equiv(scpb.Status_MERGED), to(scpb.Status_DELETE_ONLY, - emit(func(this *scpb.PrimaryIndex) scop.Op { + emit(func(this *scpb.PrimaryIndex) *scop.MakeDroppedIndexDeleteOnly { return &scop.MakeDroppedIndexDeleteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -107,14 +107,14 @@ func init() { equiv(scpb.Status_BACKFILLED), equiv(scpb.Status_BACKFILL_ONLY), to(scpb.Status_ABSENT, - emit(func(this *scpb.PrimaryIndex, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.PrimaryIndex, md *targetsWithElementMap) *scop.CreateGcJobForIndex { return &scop.CreateGcJobForIndex{ TableID: this.TableID, IndexID: this.IndexID, StatementForDropJob: statementForDropJob(this, md), } }), - emit(func(this *scpb.PrimaryIndex, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.PrimaryIndex, md *targetsWithElementMap) *scop.MakeIndexAbsent { return &scop.MakeIndexAbsent{ EventBase: newLogEventBase(this, md), TableID: this.TableID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_row_level_ttl.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_row_level_ttl.go index 8406ccb26fdc..9a44509898f0 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_row_level_ttl.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_row_level_ttl.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.RowLevelTTL) scop.Op { + emit(func(this *scpb.RowLevelTTL) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,7 +30,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.RowLevelTTL) scop.Op { + emit(func(this *scpb.RowLevelTTL) *scop.DeleteSchedule { return &scop.DeleteSchedule{ ScheduleID: this.RowLevelTTL.ScheduleID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema.go index bece3c58a79a..c864bd95bb2d 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema.go @@ -21,12 +21,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.Schema) scop.Op { + emit(func(this *scpb.Schema) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.Schema) scop.Op { + emit(func(this *scpb.Schema) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.SchemaID, } @@ -35,7 +35,7 @@ func init() { ), toAbsent(scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.Schema, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Schema, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.SchemaID, Reason: statementForDropJob(this, md).Statement, @@ -44,17 +44,17 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.Schema) scop.Op { + emit(func(this *scpb.Schema) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.SchemaID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.Schema, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Schema, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.Schema) scop.Op { + emit(func(this *scpb.Schema) *scop.DeleteDescriptor { return &scop.DeleteDescriptor{ DescriptorID: this.SchemaID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_comment.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_comment.go index ad424e567af0..7c4da90bc9f0 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_comment.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_comment.go @@ -20,13 +20,13 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.SchemaComment) scop.Op { + emit(func(this *scpb.SchemaComment) *scop.UpsertSchemaComment { return &scop.UpsertSchemaComment{ SchemaID: this.SchemaID, Comment: this.Comment, } }), - emit(func(this *scpb.SchemaComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.SchemaComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), @@ -34,7 +34,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.SchemaComment) scop.Op { + emit(func(this *scpb.SchemaComment) *scop.RemoveSchemaComment { return &scop.RemoveSchemaComment{ SchemaID: this.SchemaID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_parent.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_parent.go index 4af3ff0eaaa8..ef87f37dcdde 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_parent.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_schema_parent.go @@ -21,7 +21,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.SchemaParent) scop.Op { + emit(func(this *scpb.SchemaParent) *scop.NotImplemented { return notImplemented(this) }), ), @@ -31,7 +31,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.SchemaParent) scop.Op { + emit(func(this *scpb.SchemaParent) *scop.RemoveSchemaParent { return &scop.RemoveSchemaParent{ Parent: *protoutil.Clone(this).(*scpb.SchemaParent), } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index.go index b81d71107312..054c51664508 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index.go @@ -21,7 +21,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_BACKFILL_ONLY, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeAddedIndexBackfilling { return &scop.MakeAddedIndexBackfilling{ Index: *protoutil.Clone(&this.Index).(*scpb.Index), IsSecondaryIndex: true, @@ -29,7 +29,7 @@ func init() { }), ), to(scpb.Status_BACKFILLED, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.BackfillIndex { return &scop.BackfillIndex{ TableID: this.TableID, SourceIndexID: this.SourceIndexID, @@ -38,7 +38,7 @@ func init() { }), ), to(scpb.Status_DELETE_ONLY, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeBackfillingIndexDeleteOnly { return &scop.MakeBackfillingIndexDeleteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -46,7 +46,7 @@ func init() { }), ), to(scpb.Status_MERGE_ONLY, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeAddedIndexDeleteAndWriteOnly { return &scop.MakeAddedIndexDeleteAndWriteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -55,7 +55,7 @@ func init() { ), equiv(scpb.Status_WRITE_ONLY), to(scpb.Status_MERGED, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MergeIndex { return &scop.MergeIndex{ TableID: this.TableID, TemporaryIndexID: this.TemporaryIndexID, @@ -64,7 +64,7 @@ func init() { }), ), to(scpb.Status_VALIDATED, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.ValidateUniqueIndex { // TODO(ajwerner): Should this say something other than // ValidateUniqueIndex for a non-unique index? return &scop.ValidateUniqueIndex{ @@ -74,7 +74,7 @@ func init() { }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeAddedSecondaryIndexPublic { return &scop.MakeAddedSecondaryIndexPublic{ TableID: this.TableID, IndexID: this.IndexID, @@ -85,7 +85,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_VALIDATED, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeDroppedNonPrimaryIndexDeleteAndWriteOnly { // Most of this logic is taken from MakeMutationComplete(). return &scop.MakeDroppedNonPrimaryIndexDeleteAndWriteOnly{ TableID: this.TableID, @@ -99,7 +99,7 @@ func init() { equiv(scpb.Status_MERGE_ONLY), equiv(scpb.Status_MERGED), to(scpb.Status_DELETE_ONLY, - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeDroppedIndexDeleteOnly { return &scop.MakeDroppedIndexDeleteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -109,17 +109,17 @@ func init() { equiv(scpb.Status_BACKFILLED), equiv(scpb.Status_BACKFILL_ONLY), to(scpb.Status_ABSENT, - emit(func(this *scpb.SecondaryIndex, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.SecondaryIndex, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.SecondaryIndex, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.SecondaryIndex, md *targetsWithElementMap) *scop.CreateGcJobForIndex { return &scop.CreateGcJobForIndex{ TableID: this.TableID, IndexID: this.IndexID, StatementForDropJob: statementForDropJob(this, md), } }), - emit(func(this *scpb.SecondaryIndex) scop.Op { + emit(func(this *scpb.SecondaryIndex) *scop.MakeIndexAbsent { return &scop.MakeIndexAbsent{ TableID: this.TableID, IndexID: this.IndexID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index_partial.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index_partial.go index 6759026fafbd..4435993cfae2 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index_partial.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_secondary_index_partial.go @@ -20,14 +20,14 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.SecondaryIndexPartial) scop.Op { + emit(func(this *scpb.SecondaryIndexPartial) *scop.SetAddedIndexPartialPredicate { return &scop.SetAddedIndexPartialPredicate{ TableID: this.TableID, IndexID: this.IndexID, Expr: this.Expr, } }), - emit(func(this *scpb.SecondaryIndexPartial) scop.Op { + emit(func(this *scpb.SecondaryIndexPartial) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -42,13 +42,13 @@ func init() { scpb.Status_PUBLIC, to(scpb.Status_ABSENT, revertible(false), - emit(func(this *scpb.SecondaryIndexPartial) scop.Op { + emit(func(this *scpb.SecondaryIndexPartial) *scop.RemoveDroppedIndexPartialPredicate { return &scop.RemoveDroppedIndexPartialPredicate{ TableID: this.TableID, IndexID: this.IndexID, } }), - emit(func(this *scpb.SecondaryIndexPartial) scop.Op { + emit(func(this *scpb.SecondaryIndexPartial) *scop.UpdateTableBackReferencesInTypes { if len(this.UsesTypeIDs) == 0 { return nil } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence.go index f0170bdd2a85..a3487080d13d 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence.go @@ -22,12 +22,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.Sequence) scop.Op { + emit(func(this *scpb.Sequence) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.Sequence) scop.Op { + emit(func(this *scpb.Sequence) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.SequenceID, } @@ -36,7 +36,7 @@ func init() { ), toAbsent(scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.Sequence, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Sequence, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.SequenceID, Reason: statementForDropJob(this, md).Statement, @@ -45,22 +45,22 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.Sequence) scop.Op { + emit(func(this *scpb.Sequence) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.SequenceID, } }), - emit(func(this *scpb.Sequence) scop.Op { + emit(func(this *scpb.Sequence) *scop.RemoveAllTableComments { return &scop.RemoveAllTableComments{ TableID: this.SequenceID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.Sequence, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Sequence, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.Sequence, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Sequence, md *targetsWithElementMap) *scop.CreateGcJobForTable { return &scop.CreateGcJobForTable{ TableID: this.SequenceID, StatementForDropJob: statementForDropJob(this, md), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence_owner.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence_owner.go index 24ea29f61693..96236b3feeaf 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence_owner.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence_owner.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.SequenceOwner) scop.Op { + emit(func(this *scpb.SequenceOwner) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,14 +30,14 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.SequenceOwner) scop.Op { + emit(func(this *scpb.SequenceOwner) *scop.RemoveSequenceOwner { return &scop.RemoveSequenceOwner{ OwnedSequenceID: this.SequenceID, TableID: this.TableID, ColumnID: this.ColumnID, } }), - emit(func(this *scpb.SequenceOwner) scop.Op { + emit(func(this *scpb.SequenceOwner) *scop.RemoveOwnerBackReferenceInSequence { return &scop.RemoveOwnerBackReferenceInSequence{ SequenceID: this.SequenceID, } diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table.go index 309cb2d9f355..f8d1bd2912a3 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table.go @@ -21,12 +21,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.Table) scop.Op { + emit(func(this *scpb.Table) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.Table) scop.Op { + emit(func(this *scpb.Table) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.TableID, } @@ -36,7 +36,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.Table, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Table, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.TableID, Reason: statementForDropJob(this, md).Statement, @@ -45,22 +45,22 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.Table) scop.Op { + emit(func(this *scpb.Table) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.TableID, } }), - emit(func(this *scpb.Table) scop.Op { + emit(func(this *scpb.Table) *scop.RemoveAllTableComments { return &scop.RemoveAllTableComments{ TableID: this.TableID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.Table, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Table, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.Table, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.Table, md *targetsWithElementMap) *scop.CreateGcJobForTable { return &scop.CreateGcJobForTable{ TableID: this.TableID, StatementForDropJob: statementForDropJob(this, md), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_comment.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_comment.go index 6e3ca1832f0f..b85ea0fa0000 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_comment.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_comment.go @@ -20,13 +20,13 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.TableComment) scop.Op { + emit(func(this *scpb.TableComment) *scop.UpsertTableComment { return &scop.UpsertTableComment{ TableID: this.TableID, Comment: this.Comment, } }), - emit(func(this *scpb.TableComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.TableComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), @@ -34,12 +34,12 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_ABSENT, - emit(func(this *scpb.TableComment) scop.Op { + emit(func(this *scpb.TableComment) *scop.RemoveTableComment { return &scop.RemoveTableComment{ TableID: this.TableID, } }), - emit(func(this *scpb.TableComment, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.TableComment, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_global.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_global.go index a1885e384eaf..b48050eaa538 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_global.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_global.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.TableLocalityGlobal) scop.Op { + emit(func(this *scpb.TableLocalityGlobal) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_primary_region.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_primary_region.go index 9666db9e21b8..ff245869274a 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_primary_region.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_primary_region.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.TableLocalityPrimaryRegion) scop.Op { + emit(func(this *scpb.TableLocalityPrimaryRegion) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_regional_by_row.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_regional_by_row.go index df231d89563c..a1f01b505597 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_regional_by_row.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_regional_by_row.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.TableLocalityRegionalByRow) scop.Op { + emit(func(this *scpb.TableLocalityRegionalByRow) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_secondary_region.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_secondary_region.go index d3b64f64f9de..e6f04687b5c7 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_secondary_region.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_locality_secondary_region.go @@ -21,7 +21,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.TableLocalitySecondaryRegion) scop.Op { + emit(func(this *scpb.TableLocalitySecondaryRegion) *scop.NotImplemented { return notImplemented(this) }), ), @@ -32,7 +32,7 @@ func init() { // TODO(postamar): remove revertibility constraint when possible revertible(false), // TODO(postamar): implement table locality update - emit(func(this *scpb.TableLocalitySecondaryRegion) scop.Op { + emit(func(this *scpb.TableLocalitySecondaryRegion) *scop.RemoveBackReferenceInTypes { return &scop.RemoveBackReferenceInTypes{ TypeIDs: []catid.DescID{this.RegionEnumTypeID}, BackReferencedDescID: this.TableID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go index 0dd7a79364f8..d26309e42306 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_temporary_index.go @@ -21,7 +21,7 @@ func init() { toTransientAbsent( scpb.Status_ABSENT, to(scpb.Status_DELETE_ONLY, - emit(func(this *scpb.TemporaryIndex) scop.Op { + emit(func(this *scpb.TemporaryIndex) *scop.MakeAddedTempIndexDeleteOnly { return &scop.MakeAddedTempIndexDeleteOnly{ Index: *protoutil.Clone(&this.Index).(*scpb.Index), IsSecondaryIndex: this.IsUsingSecondaryEncoding, @@ -29,7 +29,7 @@ func init() { }), ), to(scpb.Status_WRITE_ONLY, - emit(func(this *scpb.TemporaryIndex) scop.Op { + emit(func(this *scpb.TemporaryIndex) *scop.MakeAddedIndexDeleteAndWriteOnly { return &scop.MakeAddedIndexDeleteAndWriteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -41,7 +41,7 @@ func init() { scpb.Status_WRITE_ONLY, to(scpb.Status_DELETE_ONLY, revertible(false), - emit(func(this *scpb.TemporaryIndex) scop.Op { + emit(func(this *scpb.TemporaryIndex) *scop.MakeDroppedIndexDeleteOnly { return &scop.MakeDroppedIndexDeleteOnly{ TableID: this.TableID, IndexID: this.IndexID, @@ -49,13 +49,13 @@ func init() { }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.TemporaryIndex) scop.Op { + emit(func(this *scpb.TemporaryIndex) *scop.CreateGcJobForIndex { return &scop.CreateGcJobForIndex{ TableID: this.TableID, IndexID: this.IndexID, } }), - emit(func(this *scpb.TemporaryIndex) scop.Op { + emit(func(this *scpb.TemporaryIndex) *scop.MakeIndexAbsent { return &scop.MakeIndexAbsent{ TableID: this.TableID, IndexID: this.IndexID, 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 a61cebca8322..085943adf809 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 @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.UniqueWithoutIndexConstraint) scop.Op { + emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,7 +30,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.UniqueWithoutIndexConstraint) scop.Op { + emit(func(this *scpb.UniqueWithoutIndexConstraint) *scop.NotImplemented { return notImplemented(this) }), ), diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_user_privileges.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_user_privileges.go index ddbdcd9d6cd5..7ac2ebb572d9 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_user_privileges.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_user_privileges.go @@ -20,7 +20,7 @@ func init() { toPublic( scpb.Status_ABSENT, to(scpb.Status_PUBLIC, - emit(func(this *scpb.UserPrivileges) scop.Op { + emit(func(this *scpb.UserPrivileges) *scop.NotImplemented { return notImplemented(this) }), ), @@ -30,7 +30,7 @@ func init() { to(scpb.Status_ABSENT, // TODO(postamar): remove revertibility constraint when possible revertible(false), - emit(func(this *scpb.UserPrivileges) scop.Op { + emit(func(this *scpb.UserPrivileges) *scop.RemoveUserPrivileges { return &scop.RemoveUserPrivileges{ DescID: this.DescriptorID, User: this.UserName, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_view.go b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_view.go index a72675ff7e26..cc73526268af 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/opgen_view.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/opgen_view.go @@ -21,12 +21,12 @@ func init() { scpb.Status_ABSENT, equiv(scpb.Status_DROPPED), to(scpb.Status_OFFLINE, - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.NotImplemented { return notImplemented(this) }), ), to(scpb.Status_PUBLIC, - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.MarkDescriptorAsPublic { return &scop.MarkDescriptorAsPublic{ DescID: this.ViewID, } @@ -36,7 +36,7 @@ func init() { toAbsent( scpb.Status_PUBLIC, to(scpb.Status_OFFLINE, - emit(func(this *scpb.View, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.View, md *targetsWithElementMap) *scop.MarkDescriptorAsOffline { return &scop.MarkDescriptorAsOffline{ DescID: this.ViewID, Reason: statementForDropJob(this, md).Statement, @@ -45,12 +45,12 @@ func init() { ), to(scpb.Status_DROPPED, revertible(false), - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.MarkDescriptorAsDropped { return &scop.MarkDescriptorAsDropped{ DescID: this.ViewID, } }), - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.RemoveBackReferenceInTypes { if len(this.UsesTypeIDs) == 0 { return nil } @@ -59,7 +59,7 @@ func init() { TypeIDs: this.UsesTypeIDs, } }), - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.RemoveViewBackReferencesInRelations { if len(this.UsesRelationIDs) == 0 { return nil } @@ -68,17 +68,17 @@ func init() { RelationIDs: this.UsesRelationIDs, } }), - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.RemoveAllTableComments { return &scop.RemoveAllTableComments{ TableID: this.ViewID, } }), ), to(scpb.Status_ABSENT, - emit(func(this *scpb.View, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.View, md *targetsWithElementMap) *scop.LogEvent { return newLogEventOp(this, md) }), - emit(func(this *scpb.View, md *targetsWithElementMap) scop.Op { + emit(func(this *scpb.View, md *targetsWithElementMap) *scop.CreateGcJobForTable { if !this.IsMaterialized { return nil @@ -88,7 +88,7 @@ func init() { StatementForDropJob: statementForDropJob(this, md), } }), - emit(func(this *scpb.View) scop.Op { + emit(func(this *scpb.View) *scop.DeleteDescriptor { if !this.IsMaterialized { return &scop.DeleteDescriptor{ DescriptorID: this.ViewID, diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/target.go b/pkg/sql/schemachanger/scplan/internal/opgen/target.go index 60b1192ac922..20cffebde8c4 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/target.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/target.go @@ -31,7 +31,9 @@ type target struct { type transition struct { from, to scpb.Status revertible bool + canFail bool ops opsFunc + opType scop.Type minPhase scop.Phase } @@ -70,35 +72,61 @@ func makeTarget(e scpb.Element, spec targetSpec) (t target, err error) { func makeTransitions(e scpb.Element, spec targetSpec) (ret []transition, err error) { tbs := makeTransitionBuildState(spec.from) + + // lastTransitionWhichCanFail tracks the index in ret of the last transition + // corresponding to a backfill or validation. We'll use this to add an + // annotation to the transitions indicating whether such an operation exists + // in the current or any subsequent transitions. + lastTransitionWhichCanFail := -1 for i, s := range spec.transitionSpecs { var t transition if s.from == scpb.Status_UNKNOWN { t.from = tbs.from t.to = s.to if err := tbs.withTransition(s, i == 0 /* isFirst */); err != nil { - return nil, errors.Wrapf(err, "invalid transition %s -> %s", t.from, t.to) + return nil, errors.Wrapf( + err, "invalid transition %s -> %s", t.from, t.to, + ) } if len(s.emitFns) > 0 { - t.ops, err = makeOpsFunc(e, s.emitFns) + t.ops, t.opType, err = makeOpsFunc(e, s.emitFns) if err != nil { - return nil, errors.Wrapf(err, "making ops func for transition %s -> %s", t.from, t.to) + return nil, errors.Wrapf( + err, "making ops func for transition %s -> %s", t.from, t.to, + ) } } } else { t.from = s.from t.to = tbs.from if err := tbs.withEquivTransition(s); err != nil { - return nil, errors.Wrapf(err, "invalid no-op transition %s -> %s", t.from, t.to) + return nil, errors.Wrapf( + err, "invalid no-op transition %s -> %s", t.from, t.to, + ) } } t.revertible = tbs.isRevertible t.minPhase = tbs.currentMinPhase + if t.opType != scop.MutationType { + lastTransitionWhichCanFail = i + } ret = append(ret, t) } + // Mark the transitions which can fail or precede something which can fail. + for i := range ret { + if i <= lastTransitionWhichCanFail { + ret[i].canFail = true + } else { + break + } + } + // Check that the final status has been reached. if tbs.from != spec.to { - return nil, errors.Errorf("expected %s as the final status, instead found %s", spec.to, tbs.from) + return nil, errors.Errorf( + "expected %s as the final status, instead found %s", spec.to, tbs.from, + ) } return ret, nil diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go b/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go index 86724458a73f..85555c85eb80 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go +++ b/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go @@ -29,11 +29,20 @@ type Edge interface { // OpEdge represents an edge changing the state of a target with an op. type OpEdge struct { - from, to *screl.Node - op []scop.Op - typ scop.Type + from, to *screl.Node + op []scop.Op + typ scop.Type + + // canFail indicates that a backfill or validation operation for this + // target has yet to be run as of the originating node for this edge. + canFail bool + + // revertible indicates that no operation which destroys information + // permanently or publishes new information externally has yet been + // run for this target. revertible bool - minPhase scop.Phase + + minPhase scop.Phase } // From implements the Edge interface. @@ -45,9 +54,14 @@ func (oe *OpEdge) To() *screl.Node { return oe.to } // Op returns the scop.Op for execution that is associated with the op edge. func (oe *OpEdge) Op() []scop.Op { return oe.op } -// Revertible returns if the dependency edge is revertible +// Revertible returns if the dependency edge is revertible. func (oe *OpEdge) Revertible() bool { return oe.revertible } +// CanFail returns if the dependency edge corresponds to a target in a state +// which has yet to undergo all of its validation and backfill operations +// before reaching its targeted status. +func (oe *OpEdge) CanFail() bool { return oe.revertible } + // Type returns the types of operations associated with this edge. func (oe *OpEdge) Type() scop.Type { return oe.typ diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go b/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go index 41a920f36d37..253e79e10f1b 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go +++ b/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go @@ -202,12 +202,17 @@ func (g *Graph) GetOpEdgeFrom(n *screl.Node) (*OpEdge, bool) { // AddOpEdges adds an op edges connecting the nodes for two statuses of a target. func (g *Graph) AddOpEdges( - t *scpb.Target, from, to scpb.Status, revertible bool, minPhase scop.Phase, ops ...scop.Op, + t *scpb.Target, + from, to scpb.Status, + revertible, canFail bool, + minPhase scop.Phase, + ops ...scop.Op, ) (err error) { oe := &OpEdge{ op: ops, revertible: revertible, + canFail: canFail, minPhase: minPhase, } if oe.from, err = g.getOrCreateNode(t, from); err != nil { diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go b/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go index 8d6d73a3eeaf..34cc493a39c3 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go +++ b/pkg/sql/schemachanger/scplan/internal/scgraph/graph_test.go @@ -111,12 +111,13 @@ func TestGraphRanks(t *testing.T) { require.NoError(t, err) // Setup op edges for all the nodes. for idx := range tc.addNode { + const revertible, canFail = true, false if tc.addNode[idx] { require.NoError(t, graph.AddOpEdges( &ts.Targets[idx], scpb.Status_ABSENT, scpb.Status_PUBLIC, - true, + revertible, canFail, scop.StatementPhase, &scop.MakeColumnAbsent{}, )) @@ -125,7 +126,7 @@ func TestGraphRanks(t *testing.T) { &ts.Targets[idx], scpb.Status_PUBLIC, scpb.Status_ABSENT, - true, + revertible, canFail, scop.StatementPhase, &scop.MakeColumnAbsent{}, )) diff --git a/pkg/sql/schemachanger/scplan/testdata/alter_table b/pkg/sql/schemachanger/scplan/testdata/alter_table index 3b5bf1c56346..7e5a83105cee 100644 --- a/pkg/sql/schemachanger/scplan/testdata/alter_table +++ b/pkg/sql/schemachanger/scplan/testdata/alter_table @@ -56,7 +56,7 @@ StatementPhase stage 1 of 1 with 6 MutationType ops TypeIDs: - 105 - 106 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 1 Kind: 2 @@ -202,20 +202,20 @@ StatementPhase stage 1 of 1 with 11 MutationType ops IsUnique: true SourceIndexID: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 3 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 3 Kind: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 2 Kind: 2 @@ -352,7 +352,7 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], VALIDATED] -> DELETE_ONLY [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 1 IndexID: 1 TableID: 104 @@ -520,32 +520,32 @@ StatementPhase stage 1 of 1 with 18 MutationType ops IsUnique: true SourceIndexID: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 3 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 3 Kind: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 2 Kind: 2 Ordinal: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 3 Kind: 2 Ordinal: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 2 Kind: 2 @@ -705,7 +705,7 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], VALIDATED] -> DELETE_ONLY [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 1 IndexID: 1 TableID: 104 @@ -821,20 +821,20 @@ StatementPhase stage 1 of 1 with 10 MutationType ops IsUnique: true SourceIndexID: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 3 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 3 Kind: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 2 Kind: 2 @@ -973,7 +973,7 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], VALIDATED] -> DELETE_ONLY [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 1 IndexID: 1 TableID: 104 @@ -1077,7 +1077,7 @@ StatementPhase stage 1 of 1 with 10 MutationType ops family: IntFamily oid: 20 width: 64 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 1 Kind: 2 @@ -1117,7 +1117,7 @@ StatementPhase stage 1 of 1 with 10 MutationType ops family: IntFamily oid: 20 width: 64 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 1 Kind: 2 diff --git a/pkg/sql/schemachanger/scplan/testdata/create_index b/pkg/sql/schemachanger/scplan/testdata/create_index index 71f5a7255fea..9356ebb107ce 100644 --- a/pkg/sql/schemachanger/scplan/testdata/create_index +++ b/pkg/sql/schemachanger/scplan/testdata/create_index @@ -29,30 +29,30 @@ StatementPhase stage 1 of 1 with 8 MutationType ops SourceIndexID: 1 TableID: 104 IsSecondaryIndex: true - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 3 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 3 Ordinal: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 3 Kind: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 2 Ordinal: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 2 Kind: 2 @@ -272,30 +272,30 @@ StatementPhase stage 1 of 1 with 8 MutationType ops SourceIndexID: 1 TableID: 104 IsSecondaryIndex: true - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 3 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 3 Ordinal: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 3 Kind: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 1 IndexID: 2 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 2 IndexID: 2 Ordinal: 1 TableID: 104 - scop.AddColumnToIndex + *scop.AddColumnToIndex ColumnID: 3 IndexID: 2 Kind: 2 diff --git a/pkg/sql/schemachanger/scplan/testdata/drop_index b/pkg/sql/schemachanger/scplan/testdata/drop_index index ec190d9eb33f..60aca0cd1634 100644 --- a/pkg/sql/schemachanger/scplan/testdata/drop_index +++ b/pkg/sql/schemachanger/scplan/testdata/drop_index @@ -54,11 +54,11 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops *scop.MakeDroppedIndexDeleteOnly IndexID: 2 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 1 IndexID: 2 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 3 IndexID: 2 Kind: 1 @@ -210,11 +210,11 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 6 MutationType ops *scop.MakeDroppedIndexDeleteOnly IndexID: 4 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 4 IndexID: 4 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 3 IndexID: 4 Kind: 1 @@ -415,16 +415,16 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 9 MutationType ops *scop.MakeDroppedIndexDeleteOnly IndexID: 6 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 5 IndexID: 6 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 1 IndexID: 6 Ordinal: 1 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 3 IndexID: 6 Kind: 1 @@ -634,11 +634,11 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 10 MutationType ops DescriptorID: 105 Name: v SchemaID: 101 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 2 IndexID: 8 TableID: 104 - scop.RemoveColumnFromIndex + *scop.RemoveColumnFromIndex ColumnID: 3 IndexID: 8 Kind: 1 From 20acd1218bac09150f5bf9827405119dc7a3173b Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Thu, 14 Jul 2022 02:17:20 -0400 Subject: [PATCH 4/7] sql/schemachanger/scplan: allow plan to move to NonRevertible earlier This is critical for DROP COLUMN. In `DROP COLUMN` we cannot perform the primary index swap until the dropping column reaches `DELETE_ONLY`, which is not revertible. The primary index swap itself is revertible. Given this fact, we need a mechanism to be able to "promote" revertible operations (operations which don't destroy information or cause irrevocable visible side effects) to be grouped with or after non-revertible operations. This commit makes that happen naturally. Release note: None --- .../testdata/end_to_end/create_index | 79 ++++----- .../testdata/logic_test/new_schema_changer | 2 - .../scplan/internal/opgen/target.go | 10 +- .../internal/rules/dep_index_and_column.go | 41 +++++ .../scplan/internal/rules/testdata/deprules | 29 ++++ .../scplan/internal/scgraph/edge.go | 2 +- .../scplan/internal/scgraph/graph.go | 1 - .../scplan/internal/scstage/build.go | 80 +++++++-- .../schemachanger/scplan/testdata/alter_table | 162 +++++++++--------- .../scplan/testdata/create_index | 64 +++---- pkg/sql/schemachanger/screl/scalars.go | 9 + .../testdata/alter_table_add_column | 69 +++----- .../alter_table_add_column_default_seq | 69 +++----- pkg/sql/schemachanger/testdata/create_index | 77 +++------ 14 files changed, 368 insertions(+), 326 deletions(-) diff --git a/pkg/ccl/schemachangerccl/testdata/end_to_end/create_index b/pkg/ccl/schemachangerccl/testdata/end_to_end/create_index index 82039a6e3176..0b8689c8e4ee 100644 --- a/pkg/ccl/schemachangerccl/testdata/end_to_end/create_index +++ b/pkg/ccl/schemachangerccl/testdata/end_to_end/create_index @@ -285,7 +285,7 @@ notified job registry to adopt jobs: [1] begin transaction #2 commit transaction #2 begin transaction #3 -## PostCommitPhase stage 1 of 7 with 3 MutationType ops +## PostCommitPhase stage 1 of 6 with 3 MutationType ops upsert descriptor #104 ... - ABSENT @@ -314,14 +314,14 @@ upsert descriptor #104 unexposedParentSchemaId: 101 - version: "2" + version: "3" -update progress of schema change job #1: "PostCommitPhase stage 2 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 2 of 6 with 1 BackfillType op pending" commit transaction #3 begin transaction #4 -## PostCommitPhase stage 2 of 7 with 1 BackfillType op +## PostCommitPhase stage 2 of 6 with 1 BackfillType op backfill indexes [2] from index #1 in table #104 commit transaction #4 begin transaction #5 -## PostCommitPhase stage 3 of 7 with 3 MutationType ops +## PostCommitPhase stage 3 of 6 with 3 MutationType ops upsert descriptor #104 ... - PUBLIC @@ -350,10 +350,10 @@ upsert descriptor #104 unexposedParentSchemaId: 101 - version: "3" + version: "4" -update progress of schema change job #1: "PostCommitPhase stage 4 of 7 with 1 MutationType op pending" +update progress of schema change job #1: "PostCommitPhase stage 4 of 6 with 1 MutationType op pending" commit transaction #5 begin transaction #6 -## PostCommitPhase stage 4 of 7 with 3 MutationType ops +## PostCommitPhase stage 4 of 6 with 3 MutationType ops upsert descriptor #104 ... - PUBLIC @@ -382,18 +382,18 @@ upsert descriptor #104 unexposedParentSchemaId: 101 - version: "4" + version: "5" -update progress of schema change job #1: "PostCommitPhase stage 5 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 5 of 6 with 1 BackfillType op pending" commit transaction #6 begin transaction #7 -## PostCommitPhase stage 5 of 7 with 1 BackfillType op +## PostCommitPhase stage 5 of 6 with 1 BackfillType op merge temporary indexes [3] into backfilled indexes [2] in table #104 commit transaction #7 begin transaction #8 -## PostCommitPhase stage 6 of 7 with 1 ValidationType op +## PostCommitPhase stage 6 of 6 with 1 ValidationType op validate forward indexes [2] in table #104 commit transaction #8 begin transaction #9 -## PostCommitPhase stage 7 of 7 with 4 MutationType ops +## PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops upsert descriptor #104 ... - PUBLIC @@ -401,10 +401,16 @@ upsert descriptor #104 - - MERGE_ONLY - - ABSENT - PUBLIC + - - WRITE_ONLY + - PUBLIC + - PUBLIC + + - TRANSIENT_DELETE_ONLY + - PUBLIC + - PUBLIC + - PUBLIC + - PUBLIC - - WRITE_ONLY - - PUBLIC + jobId: "1" + relevantStatements: ... BY LIST (id) (PARTITION p1 VALUES IN (1)) statementTag: CREATE INDEX @@ -448,7 +454,8 @@ upsert descriptor #104 + version: 4 + modificationTime: {} mutations: - - direction: ADD + - - direction: ADD + + - direction: DROP index: - constraintId: 2 - createdExplicitly: true @@ -485,35 +492,6 @@ upsert descriptor #104 - index: constraintId: 3 createdExplicitly: true - ... - time: {} - unexposedParentSchemaId: 101 - - version: "5" - + version: "6" -update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 1 of 2 with 1 MutationType op pending" -set schema change job #1 to non-cancellable -commit transaction #9 -begin transaction #10 -## PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops -upsert descriptor #104 - ... - - PUBLIC - - PUBLIC - - - WRITE_ONLY - + - TRANSIENT_DELETE_ONLY - - PUBLIC - - PUBLIC - ... - - money - version: 4 - - modificationTime: - - wallTime: "1640995200000000009" - + modificationTime: {} - mutations: - - - direction: ADD - + - direction: DROP - index: - constraintId: 3 ... version: 4 mutationId: 1 @@ -524,11 +502,12 @@ upsert descriptor #104 ... time: {} unexposedParentSchemaId: 101 - - version: "6" - + version: "7" + - version: "5" + + version: "6" update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 2 with 2 MutationType ops pending" -commit transaction #10 -begin transaction #11 +set schema change job #1 to non-cancellable +commit transaction #9 +begin transaction #10 ## PostCommitNonRevertiblePhase stage 2 of 2 with 4 MutationType ops upsert descriptor #104 ... @@ -694,7 +673,7 @@ upsert descriptor #104 - money version: 4 - modificationTime: - - wallTime: "1640995200000000010" + - wallTime: "1640995200000000009" - mutations: - - direction: DROP - index: @@ -737,12 +716,12 @@ upsert descriptor #104 ... time: {} unexposedParentSchemaId: 101 - - version: "7" - + version: "8" + - version: "6" + + version: "7" write *eventpb.FinishSchemaChange to event log for descriptor 104 create job #2 (non-cancelable: true): "GC for " descriptor IDs: [104] update progress of schema change job #1: "all stages completed" -commit transaction #11 +commit transaction #10 notified job registry to adopt jobs: [2] # end PostCommitPhase diff --git a/pkg/sql/logictest/testdata/logic_test/new_schema_changer b/pkg/sql/logictest/testdata/logic_test/new_schema_changer index 4397faf76b18..75256b622207 100644 --- a/pkg/sql/logictest/testdata/logic_test/new_schema_changer +++ b/pkg/sql/logictest/testdata/logic_test/new_schema_changer @@ -1995,8 +1995,6 @@ Schema change plan for DROP INDEX ‹test›.public.‹parent›@‹idx› CASCA ├── RemoveJobStateFromDescriptor {"DescriptorID":266} └── UpdateSchemaChangerJob {"IsNonCancelable":true,"RunningStatus":"all stages compl..."} - - statement ok SET use_declarative_schema_changer = 'on' diff --git a/pkg/sql/schemachanger/scplan/internal/opgen/target.go b/pkg/sql/schemachanger/scplan/internal/opgen/target.go index 20cffebde8c4..be20f91cf563 100644 --- a/pkg/sql/schemachanger/scplan/internal/opgen/target.go +++ b/pkg/sql/schemachanger/scplan/internal/opgen/target.go @@ -107,19 +107,15 @@ func makeTransitions(e scpb.Element, spec targetSpec) (ret []transition, err err } t.revertible = tbs.isRevertible t.minPhase = tbs.currentMinPhase - if t.opType != scop.MutationType { + if t.opType != scop.MutationType && t.opType != 0 { lastTransitionWhichCanFail = i } ret = append(ret, t) } // Mark the transitions which can fail or precede something which can fail. - for i := range ret { - if i <= lastTransitionWhichCanFail { - ret[i].canFail = true - } else { - break - } + for i := 0; i <= lastTransitionWhichCanFail; i++ { + ret[i].canFail = true } // Check that the final status has been reached. diff --git a/pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go b/pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go index e1af0bf1786f..e857c703b18b 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go +++ b/pkg/sql/schemachanger/scplan/internal/rules/dep_index_and_column.go @@ -743,6 +743,47 @@ func init() { currentStatus(to.node, scpb.Status_WRITE_ONLY), } }) + + // This is a pair of somewhat brute-force hack to ensure that we only create + // a single GC job for all the indexes of a table being dropped by a + // transaction. + registerDepRule("temp indexes reach absent at the same time as other indexes", + scgraph.SameStagePrecedence, + "index-a", "index-b", + func(from, to nodeVars) rel.Clauses { + return rel.Clauses{ + from.el.Type((*scpb.TemporaryIndex)(nil)), + to.el.Type((*scpb.PrimaryIndex)(nil), (*scpb.SecondaryIndex)(nil)), + joinOnDescID(from.el, to.el, "descID"), + targetStatus(from.target, scpb.Transient), + targetStatus(to.target, scpb.ToAbsent), + currentStatus(from.node, scpb.Status_TRANSIENT_ABSENT), + currentStatus(to.node, scpb.Status_ABSENT), + } + }) + registerDepRule("indexes reach absent at the same time as other indexes", + scgraph.SameStagePrecedence, + "index-a", "index-b", + func(from, to nodeVars) rel.Clauses { + return rel.Clauses{ + from.el.Type((*scpb.PrimaryIndex)(nil), (*scpb.SecondaryIndex)(nil)), + to.el.Type((*scpb.PrimaryIndex)(nil), (*scpb.SecondaryIndex)(nil)), + joinOnDescID(from.el, to.el, "descID"), + targetStatusEq(from.target, to.target, scpb.ToAbsent), + currentStatusEq(from.node, to.node, scpb.Status_ABSENT), + + // Use the index ID to provide an ordering for dropping the indexes + // to ensure that there is no cycle in the edges. + // + // TODO(ajwerner): It'd be nice to be able to express this in rel + // directly. + rel.Filter("indexes-id-less", "a", "b")(func(a, b scpb.Element) bool { + aID, _ := screl.GetIndexID(a) + bID, _ := screl.GetIndexID(b) + return aID < bID + }), + } + }) } // This rule ensures that columns depend on each other in increasing order. diff --git a/pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules b/pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules index c7938168fc2e..8d21421dd7dc 100644 --- a/pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules +++ b/pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules @@ -592,6 +592,35 @@ deprules - $index-node[CurrentStatus] = WRITE_ONLY - joinTargetNode($index-column, $index-column-target, $index-column-node) - joinTargetNode($index, $index-target, $index-node) +- name: temp indexes reach absent at the same time as other indexes + from: index-a-node + kind: SameStagePrecedence + to: index-b-node + query: + - $index-a[Type] = '*scpb.TemporaryIndex' + - $index-b[Type] IN ['*scpb.PrimaryIndex', '*scpb.SecondaryIndex'] + - joinOnDescID($index-a, $index-b, $descID) + - $index-a-target[TargetStatus] = TRANSIENT_ABSENT + - $index-b-target[TargetStatus] = ABSENT + - $index-a-node[CurrentStatus] = TRANSIENT_ABSENT + - $index-b-node[CurrentStatus] = ABSENT + - joinTargetNode($index-a, $index-a-target, $index-a-node) + - joinTargetNode($index-b, $index-b-target, $index-b-node) +- name: indexes reach absent at the same time as other indexes + from: index-a-node + kind: SameStagePrecedence + to: index-b-node + query: + - $index-a[Type] IN ['*scpb.PrimaryIndex', '*scpb.SecondaryIndex'] + - $index-b[Type] IN ['*scpb.PrimaryIndex', '*scpb.SecondaryIndex'] + - joinOnDescID($index-a, $index-b, $descID) + - $index-a-target[TargetStatus] = ABSENT + - $index-b-target[TargetStatus] = ABSENT + - $index-a-node[CurrentStatus] = ABSENT + - $index-b-node[CurrentStatus] = ABSENT + - indexes-id-less(scpb.Element, scpb.Element)($a, $b) + - joinTargetNode($index-a, $index-a-target, $index-a-node) + - joinTargetNode($index-b, $index-b-target, $index-b-node) - name: ensure columns are in increasing order from: later-column-node kind: SameStagePrecedence diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go b/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go index 85555c85eb80..132af20d015e 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go +++ b/pkg/sql/schemachanger/scplan/internal/scgraph/edge.go @@ -60,7 +60,7 @@ func (oe *OpEdge) Revertible() bool { return oe.revertible } // CanFail returns if the dependency edge corresponds to a target in a state // which has yet to undergo all of its validation and backfill operations // before reaching its targeted status. -func (oe *OpEdge) CanFail() bool { return oe.revertible } +func (oe *OpEdge) CanFail() bool { return oe.canFail } // Type returns the types of operations associated with this edge. func (oe *OpEdge) Type() scop.Type { diff --git a/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go b/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go index 253e79e10f1b..9380f6399f87 100644 --- a/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go +++ b/pkg/sql/schemachanger/scplan/internal/scgraph/graph.go @@ -208,7 +208,6 @@ func (g *Graph) AddOpEdges( minPhase scop.Phase, ops ...scop.Op, ) (err error) { - oe := &OpEdge{ op: ops, revertible: revertible, diff --git a/pkg/sql/schemachanger/scplan/internal/scstage/build.go b/pkg/sql/schemachanger/scplan/internal/scstage/build.go index 67942c109b9f..99dde6fc0e23 100644 --- a/pkg/sql/schemachanger/scplan/internal/scstage/build.go +++ b/pkg/sql/schemachanger/scplan/internal/scstage/build.go @@ -110,11 +110,22 @@ func buildStages(bc buildContext) (stages []Stage) { for !bc.isStateTerminal(bs.incumbent) { // Generate a stage builder which can make progress. sb := bc.makeStageBuilder(bs) - for !sb.canMakeProgress() { + // We allow mixing of revertible and non-revertible operations if there + // are no remaining operations which can fail. That being said, we want + // to not want to build such a stage as PostCommit but rather as + // PostCommitNonRevertible. This condition is used to determine whether + // the phase needs to be advanced due to the presence of non-revertible + // operations. + shouldBeInPostCommitNonRevertible := func() bool { + return !bc.isRevertibilityIgnored && + bs.phase == scop.PostCommitPhase && + sb.hasAnyNonRevertibleOps() + } + for !sb.canMakeProgress() || shouldBeInPostCommitNonRevertible() { // When no further progress is possible, move to the next phase and try // again, until progress is possible. We haven't reached the terminal // state yet, so this is guaranteed (barring any horrible bugs). - if bs.phase == scop.PreCommitPhase { + if !sb.canMakeProgress() && bs.phase == scop.PreCommitPhase { // This is a special case. // We need to move to the post-commit phase, but this will require // creating a schema changer job, which in turn will require this @@ -197,11 +208,27 @@ func (bc buildContext) makeStageBuilderForType(bs buildState, opType scop.Type) lut: make(map[*scpb.Target]*currentTargetState, numTargets), visited: make(map[*screl.Node]uint64, numTargets), } - for i, n := range bc.nodes(bs.incumbent) { - t := sb.makeCurrentTargetState(n) - sb.current[i] = t - sb.lut[t.n.Target] = &sb.current[i] + { + nodes := bc.nodes(bs.incumbent) + + // Determine whether there are any backfill or validation operations + // remaining which should prevent the scheduling of any non-revertible + // operations. This information will be used when building the current + // set of targets below. + for _, n := range nodes { + if oe, ok := bc.g.GetOpEdgeFrom(n); ok && oe.CanFail() { + sb.anyRemainingOpsCanFail = true + break + } + } + + for i, n := range nodes { + t := sb.makeCurrentTargetState(n) + sb.current[i] = t + sb.lut[t.n.Target] = &sb.current[i] + } } + // Greedily try to make progress by going down op edges when possible. for isDone := false; !isDone; { isDone = true @@ -236,6 +263,13 @@ type stageBuilder struct { fulfilling map[*screl.Node]struct{} opEdges []*scgraph.OpEdge + // anyRemainingOpsCanFail indicates whether there are any backfill or + // validation operations remaining in the schema change. If not, then + // revertible operations can be combined with non-revertible operations + // in order to move the schema change into the PostCommitNonRevertiblePhase + // earlier than it might otherwise. + anyRemainingOpsCanFail bool + // Helper data structures used to improve performance. lut map[*scpb.Target]*currentTargetState @@ -283,9 +317,20 @@ func (sb stageBuilder) isOutgoingOpEdgeAllowed(e *scgraph.OpEdge) bool { if !e.IsPhaseSatisfied(sb.bs.phase) { return false } + // We allow non-revertible ops to be included at stages preceding + // PostCommitNonRevertible if nothing left in the schema change at this + // point can fail. The caller is responsible for detecting whether any + // non-revertible operations are included in a phase before + // PostCommitNonRevertible and adjusting the phase accordingly. This is + // critical to allow op-edges which might otherwise be revertible to be + // grouped with non-revertible operations. if !sb.bc.isRevertibilityIgnored && sb.bs.phase < scop.PostCommitNonRevertiblePhase && - !e.Revertible() { + !e.Revertible() && + // We can't act on the knowledge that nothing remaining can fail while in + // StatementPhase because we don't know about what future targets may + // show up which could fail. + (sb.bs.phase < scop.PostCommitPhase || sb.anyRemainingOpsCanFail) { return false } return true @@ -456,6 +501,17 @@ func (sb stageBuilder) build() Stage { return s } +// hasAnyNonRevertibleOps returns true if there are any ops in the current +// stage which are not revertible. +func (sb stageBuilder) hasAnyNonRevertibleOps() bool { + for _, e := range sb.opEdges { + if !e.Revertible() { + return true + } + } + return false +} + // computeExtraJobOps generates job-related operations to decorate a stage with. // // TODO(ajwerner): Rather than adding this above the opgen layer, it may be @@ -552,10 +608,14 @@ func (bc buildContext) removeJobReferenceOp(descID descpb.ID) scop.Op { } func (bc buildContext) nodes(current []scpb.Status) []*screl.Node { - nodes := make([]*screl.Node, len(bc.targetState.Targets)) + return nodes(bc.g, bc.targetState.Targets, current) +} + +func nodes(g *scgraph.Graph, targets []scpb.Target, current []scpb.Status) []*screl.Node { + nodes := make([]*screl.Node, len(targets)) for i, status := range current { - t := &bc.targetState.Targets[i] - n, ok := bc.g.GetNode(t, status) + t := &targets[i] + n, ok := g.GetNode(t, status) if !ok { panic(errors.AssertionFailedf("could not find node for element %s, target status %s, current status %s", screl.ElementString(t.Element()), t.TargetStatus, status)) diff --git a/pkg/sql/schemachanger/scplan/testdata/alter_table b/pkg/sql/schemachanger/scplan/testdata/alter_table index 7e5a83105cee..2fab13926bfd 100644 --- a/pkg/sql/schemachanger/scplan/testdata/alter_table +++ b/pkg/sql/schemachanger/scplan/testdata/alter_table @@ -232,13 +232,13 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops DescriptorIDs: - 104 JobID: 1 - RunningStatus: PostCommitPhase stage 1 of 7 with 2 MutationType ops pending + RunningStatus: PostCommitPhase stage 1 of 6 with 2 MutationType ops pending Statements: - statement: ALTER TABLE defaultdb.foo ADD COLUMN j INT8 DEFAULT 123 redactedstatement: ALTER TABLE ‹defaultdb›.public.‹foo› ADD COLUMN ‹j› INT8 DEFAULT ‹123› statementtag: ALTER TABLE -PostCommitPhase stage 1 of 7 with 4 MutationType ops +PostCommitPhase stage 1 of 6 with 4 MutationType ops transitions: [[Column:{DescID: 104, ColumnID: 2}, PUBLIC], DELETE_ONLY] -> WRITE_ONLY [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], DELETE_ONLY] -> WRITE_ONLY @@ -253,7 +253,7 @@ PostCommitPhase stage 1 of 7 with 4 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 2 of 7 with 1 BackfillType op +PostCommitPhase stage 2 of 6 with 1 BackfillType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILL_ONLY] -> BACKFILLED ops: @@ -261,7 +261,7 @@ PostCommitPhase stage 2 of 7 with 1 BackfillType op IndexID: 2 SourceIndexID: 1 TableID: 104 -PostCommitPhase stage 3 of 7 with 3 MutationType ops +PostCommitPhase stage 3 of 6 with 3 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILLED] -> DELETE_ONLY ops: @@ -272,7 +272,7 @@ PostCommitPhase stage 3 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 4 of 7 with 3 MutationType ops +PostCommitPhase stage 4 of 6 with 3 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], DELETE_ONLY] -> MERGE_ONLY ops: @@ -283,7 +283,7 @@ PostCommitPhase stage 4 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 5 of 7 with 1 BackfillType op +PostCommitPhase stage 5 of 6 with 1 BackfillType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGE_ONLY] -> MERGED ops: @@ -291,20 +291,22 @@ PostCommitPhase stage 5 of 7 with 1 BackfillType op BackfilledIndexID: 2 TableID: 104 TemporaryIndexID: 3 -PostCommitPhase stage 6 of 7 with 1 ValidationType op +PostCommitPhase stage 6 of 6 with 1 ValidationType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGED] -> VALIDATED ops: *scop.ValidateUniqueIndex IndexID: 2 TableID: 104 -PostCommitPhase stage 7 of 7 with 8 MutationType ops +PostCommitNonRevertiblePhase stage 1 of 3 with 10 MutationType ops transitions: - [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC] -> VALIDATED + [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT + [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC] -> WRITE_ONLY [[IndexName:{DescID: 104, Name: foo_pkey, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT [[Column:{DescID: 104, ColumnID: 2}, PUBLIC], WRITE_ONLY] -> PUBLIC [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], VALIDATED] -> PUBLIC [[IndexName:{DescID: 104, Name: foo_pkey, IndexID: 2}, PUBLIC], ABSENT] -> PUBLIC + [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: *scop.MakeDroppedPrimaryIndexDeleteAndWriteOnly IndexID: 1 @@ -317,6 +319,13 @@ PostCommitPhase stage 7 of 7 with 8 MutationType ops IndexID: 2 Name: foo_pkey TableID: 104 + *scop.MakeDroppedIndexDeleteOnly + IndexID: 3 + TableID: 104 + *scop.RemoveColumnFromIndex + ColumnID: 1 + IndexID: 1 + TableID: 104 *scop.MakeAddedPrimaryIndexPublic EventBase: Authorization: @@ -346,19 +355,10 @@ PostCommitPhase stage 7 of 7 with 8 MutationType ops *scop.UpdateSchemaChangerJob IsNonCancelable: true JobID: 1 -PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops +PostCommitNonRevertiblePhase stage 2 of 3 with 3 MutationType ops transitions: - [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT - [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], VALIDATED] -> DELETE_ONLY - [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY + [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], WRITE_ONLY] -> DELETE_ONLY ops: - *scop.RemoveColumnFromIndex - ColumnID: 1 - IndexID: 1 - TableID: 104 - *scop.MakeDroppedIndexDeleteOnly - IndexID: 3 - TableID: 104 *scop.MakeDroppedIndexDeleteOnly IndexID: 1 TableID: 104 @@ -367,11 +367,17 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops *scop.UpdateSchemaChangerJob IsNonCancelable: true JobID: 1 -PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops +PostCommitNonRevertiblePhase stage 3 of 3 with 6 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], DELETE_ONLY] -> ABSENT [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], TRANSIENT_DELETE_ONLY] -> TRANSIENT_ABSENT ops: + *scop.CreateGcJobForIndex + IndexID: 3 + TableID: 104 + *scop.MakeIndexAbsent + IndexID: 3 + TableID: 104 *scop.CreateGcJobForIndex IndexID: 1 StatementForDropJob: @@ -388,12 +394,6 @@ PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops SubWorkID: 1 IndexID: 1 TableID: 104 - *scop.CreateGcJobForIndex - IndexID: 3 - TableID: 104 - *scop.MakeIndexAbsent - IndexID: 3 - TableID: 104 *scop.RemoveJobStateFromDescriptor DescriptorID: 104 JobID: 1 @@ -562,7 +562,7 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops DescriptorIDs: - 104 JobID: 1 - RunningStatus: PostCommitPhase stage 1 of 7 with 3 MutationType ops pending + RunningStatus: PostCommitPhase stage 1 of 6 with 3 MutationType ops pending Statements: - statement: ALTER TABLE defaultdb.foo ADD COLUMN j INT8 DEFAULT 123 redactedstatement: ALTER TABLE ‹defaultdb›.public.‹foo› ADD COLUMN ‹j› INT8 DEFAULT @@ -572,7 +572,7 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops redactedstatement: ALTER TABLE ‹defaultdb›.public.‹foo› ADD COLUMN ‹k› INT8 DEFAULT ‹456› statementtag: ALTER TABLE -PostCommitPhase stage 1 of 7 with 5 MutationType ops +PostCommitPhase stage 1 of 6 with 5 MutationType ops transitions: [[Column:{DescID: 104, ColumnID: 2}, PUBLIC], DELETE_ONLY] -> WRITE_ONLY [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], DELETE_ONLY] -> WRITE_ONLY @@ -591,7 +591,7 @@ PostCommitPhase stage 1 of 7 with 5 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 2 of 7 with 1 BackfillType op +PostCommitPhase stage 2 of 6 with 1 BackfillType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILL_ONLY] -> BACKFILLED ops: @@ -599,7 +599,7 @@ PostCommitPhase stage 2 of 7 with 1 BackfillType op IndexID: 2 SourceIndexID: 1 TableID: 104 -PostCommitPhase stage 3 of 7 with 3 MutationType ops +PostCommitPhase stage 3 of 6 with 3 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILLED] -> DELETE_ONLY ops: @@ -610,7 +610,7 @@ PostCommitPhase stage 3 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 4 of 7 with 3 MutationType ops +PostCommitPhase stage 4 of 6 with 3 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], DELETE_ONLY] -> MERGE_ONLY ops: @@ -621,7 +621,7 @@ PostCommitPhase stage 4 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 5 of 7 with 1 BackfillType op +PostCommitPhase stage 5 of 6 with 1 BackfillType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGE_ONLY] -> MERGED ops: @@ -629,20 +629,22 @@ PostCommitPhase stage 5 of 7 with 1 BackfillType op BackfilledIndexID: 2 TableID: 104 TemporaryIndexID: 3 -PostCommitPhase stage 6 of 7 with 1 ValidationType op +PostCommitPhase stage 6 of 6 with 1 ValidationType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGED] -> VALIDATED ops: *scop.ValidateUniqueIndex IndexID: 2 TableID: 104 -PostCommitPhase stage 7 of 7 with 10 MutationType ops +PostCommitNonRevertiblePhase stage 1 of 3 with 12 MutationType ops transitions: - [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC] -> VALIDATED + [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT + [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC] -> WRITE_ONLY [[IndexName:{DescID: 104, Name: foo_pkey, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT [[Column:{DescID: 104, ColumnID: 2}, PUBLIC], WRITE_ONLY] -> PUBLIC [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], VALIDATED] -> PUBLIC [[IndexName:{DescID: 104, Name: foo_pkey, IndexID: 2}, PUBLIC], ABSENT] -> PUBLIC + [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY [[Column:{DescID: 104, ColumnID: 3}, PUBLIC], WRITE_ONLY] -> PUBLIC ops: *scop.MakeDroppedPrimaryIndexDeleteAndWriteOnly @@ -656,6 +658,13 @@ PostCommitPhase stage 7 of 7 with 10 MutationType ops IndexID: 2 Name: foo_pkey TableID: 104 + *scop.MakeDroppedIndexDeleteOnly + IndexID: 3 + TableID: 104 + *scop.RemoveColumnFromIndex + ColumnID: 1 + IndexID: 1 + TableID: 104 *scop.MakeAddedPrimaryIndexPublic EventBase: Authorization: @@ -699,19 +708,10 @@ PostCommitPhase stage 7 of 7 with 10 MutationType ops *scop.UpdateSchemaChangerJob IsNonCancelable: true JobID: 1 -PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops +PostCommitNonRevertiblePhase stage 2 of 3 with 3 MutationType ops transitions: - [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT - [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], VALIDATED] -> DELETE_ONLY - [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY + [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], WRITE_ONLY] -> DELETE_ONLY ops: - *scop.RemoveColumnFromIndex - ColumnID: 1 - IndexID: 1 - TableID: 104 - *scop.MakeDroppedIndexDeleteOnly - IndexID: 3 - TableID: 104 *scop.MakeDroppedIndexDeleteOnly IndexID: 1 TableID: 104 @@ -720,11 +720,17 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops *scop.UpdateSchemaChangerJob IsNonCancelable: true JobID: 1 -PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops +PostCommitNonRevertiblePhase stage 3 of 3 with 6 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], DELETE_ONLY] -> ABSENT [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], TRANSIENT_DELETE_ONLY] -> TRANSIENT_ABSENT ops: + *scop.CreateGcJobForIndex + IndexID: 3 + TableID: 104 + *scop.MakeIndexAbsent + IndexID: 3 + TableID: 104 *scop.CreateGcJobForIndex IndexID: 1 StatementForDropJob: @@ -741,12 +747,6 @@ PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops SubWorkID: 1 IndexID: 1 TableID: 104 - *scop.CreateGcJobForIndex - IndexID: 3 - TableID: 104 - *scop.MakeIndexAbsent - IndexID: 3 - TableID: 104 *scop.RemoveJobStateFromDescriptor DescriptorID: 104 JobID: 1 @@ -851,13 +851,13 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops DescriptorIDs: - 104 JobID: 1 - RunningStatus: PostCommitPhase stage 1 of 7 with 2 MutationType ops pending + RunningStatus: PostCommitPhase stage 1 of 6 with 2 MutationType ops pending Statements: - statement: ALTER TABLE defaultdb.foo ADD COLUMN a INT8 AS (i + 1) STORED redactedstatement: ALTER TABLE ‹defaultdb›.public.‹foo› ADD COLUMN ‹a› INT8 AS (‹i› + ‹1›) STORED statementtag: ALTER TABLE -PostCommitPhase stage 1 of 7 with 4 MutationType ops +PostCommitPhase stage 1 of 6 with 4 MutationType ops transitions: [[Column:{DescID: 104, ColumnID: 2}, PUBLIC], DELETE_ONLY] -> WRITE_ONLY [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], DELETE_ONLY] -> WRITE_ONLY @@ -872,7 +872,7 @@ PostCommitPhase stage 1 of 7 with 4 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 2 of 7 with 1 BackfillType op +PostCommitPhase stage 2 of 6 with 1 BackfillType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILL_ONLY] -> BACKFILLED ops: @@ -880,7 +880,7 @@ PostCommitPhase stage 2 of 7 with 1 BackfillType op IndexID: 2 SourceIndexID: 1 TableID: 104 -PostCommitPhase stage 3 of 7 with 3 MutationType ops +PostCommitPhase stage 3 of 6 with 3 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILLED] -> DELETE_ONLY ops: @@ -891,7 +891,7 @@ PostCommitPhase stage 3 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 4 of 7 with 3 MutationType ops +PostCommitPhase stage 4 of 6 with 3 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], DELETE_ONLY] -> MERGE_ONLY ops: @@ -902,7 +902,7 @@ PostCommitPhase stage 4 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 5 of 7 with 1 BackfillType op +PostCommitPhase stage 5 of 6 with 1 BackfillType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGE_ONLY] -> MERGED ops: @@ -910,20 +910,22 @@ PostCommitPhase stage 5 of 7 with 1 BackfillType op BackfilledIndexID: 2 TableID: 104 TemporaryIndexID: 3 -PostCommitPhase stage 6 of 7 with 1 ValidationType op +PostCommitPhase stage 6 of 6 with 1 ValidationType op transitions: [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGED] -> VALIDATED ops: *scop.ValidateUniqueIndex IndexID: 2 TableID: 104 -PostCommitPhase stage 7 of 7 with 8 MutationType ops +PostCommitNonRevertiblePhase stage 1 of 3 with 10 MutationType ops transitions: - [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC] -> VALIDATED + [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT + [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], PUBLIC] -> WRITE_ONLY [[IndexName:{DescID: 104, Name: foo_pkey, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT [[Column:{DescID: 104, ColumnID: 2}, PUBLIC], WRITE_ONLY] -> PUBLIC [[PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 1, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], VALIDATED] -> PUBLIC [[IndexName:{DescID: 104, Name: foo_pkey, IndexID: 2}, PUBLIC], ABSENT] -> PUBLIC + [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: *scop.MakeDroppedPrimaryIndexDeleteAndWriteOnly IndexID: 1 @@ -936,6 +938,13 @@ PostCommitPhase stage 7 of 7 with 8 MutationType ops IndexID: 2 Name: foo_pkey TableID: 104 + *scop.MakeDroppedIndexDeleteOnly + IndexID: 3 + TableID: 104 + *scop.RemoveColumnFromIndex + ColumnID: 1 + IndexID: 1 + TableID: 104 *scop.MakeAddedPrimaryIndexPublic EventBase: Authorization: @@ -967,19 +976,10 @@ PostCommitPhase stage 7 of 7 with 8 MutationType ops *scop.UpdateSchemaChangerJob IsNonCancelable: true JobID: 1 -PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops +PostCommitNonRevertiblePhase stage 2 of 3 with 3 MutationType ops transitions: - [[IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 1}, ABSENT], PUBLIC] -> ABSENT - [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], VALIDATED] -> DELETE_ONLY - [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY + [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], WRITE_ONLY] -> DELETE_ONLY ops: - *scop.RemoveColumnFromIndex - ColumnID: 1 - IndexID: 1 - TableID: 104 - *scop.MakeDroppedIndexDeleteOnly - IndexID: 3 - TableID: 104 *scop.MakeDroppedIndexDeleteOnly IndexID: 1 TableID: 104 @@ -988,11 +988,17 @@ PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops *scop.UpdateSchemaChangerJob IsNonCancelable: true JobID: 1 -PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops +PostCommitNonRevertiblePhase stage 3 of 3 with 6 MutationType ops transitions: [[PrimaryIndex:{DescID: 104, IndexID: 1, ConstraintID: 1}, ABSENT], DELETE_ONLY] -> ABSENT [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], TRANSIENT_DELETE_ONLY] -> TRANSIENT_ABSENT ops: + *scop.CreateGcJobForIndex + IndexID: 3 + TableID: 104 + *scop.MakeIndexAbsent + IndexID: 3 + TableID: 104 *scop.CreateGcJobForIndex IndexID: 1 StatementForDropJob: @@ -1010,12 +1016,6 @@ PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops SubWorkID: 1 IndexID: 1 TableID: 104 - *scop.CreateGcJobForIndex - IndexID: 3 - TableID: 104 - *scop.MakeIndexAbsent - IndexID: 3 - TableID: 104 *scop.RemoveJobStateFromDescriptor DescriptorID: 104 JobID: 1 diff --git a/pkg/sql/schemachanger/scplan/testdata/create_index b/pkg/sql/schemachanger/scplan/testdata/create_index index 9356ebb107ce..f3b7e87c4fbd 100644 --- a/pkg/sql/schemachanger/scplan/testdata/create_index +++ b/pkg/sql/schemachanger/scplan/testdata/create_index @@ -69,13 +69,13 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops DescriptorIDs: - 104 JobID: 1 - RunningStatus: PostCommitPhase stage 1 of 7 with 1 MutationType op pending + RunningStatus: PostCommitPhase stage 1 of 6 with 1 MutationType op pending Statements: - statement: CREATE INDEX id1 ON defaultdb.t1 (id, name) STORING (money) redactedstatement: CREATE INDEX ‹id1› ON ‹defaultdb›.public.‹t1› (‹id›, ‹name›) STORING (‹money›) statementtag: CREATE INDEX -PostCommitPhase stage 1 of 7 with 3 MutationType ops +PostCommitPhase stage 1 of 6 with 3 MutationType ops transitions: [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], DELETE_ONLY] -> WRITE_ONLY ops: @@ -86,7 +86,7 @@ PostCommitPhase stage 1 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 2 of 7 with 1 BackfillType op +PostCommitPhase stage 2 of 6 with 1 BackfillType op transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILL_ONLY] -> BACKFILLED ops: @@ -94,7 +94,7 @@ PostCommitPhase stage 2 of 7 with 1 BackfillType op IndexID: 2 SourceIndexID: 1 TableID: 104 -PostCommitPhase stage 3 of 7 with 3 MutationType ops +PostCommitPhase stage 3 of 6 with 3 MutationType ops transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILLED] -> DELETE_ONLY ops: @@ -105,7 +105,7 @@ PostCommitPhase stage 3 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 4 of 7 with 3 MutationType ops +PostCommitPhase stage 4 of 6 with 3 MutationType ops transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], DELETE_ONLY] -> MERGE_ONLY ops: @@ -116,7 +116,7 @@ PostCommitPhase stage 4 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 5 of 7 with 1 BackfillType op +PostCommitPhase stage 5 of 6 with 1 BackfillType op transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGE_ONLY] -> MERGED ops: @@ -124,37 +124,29 @@ PostCommitPhase stage 5 of 7 with 1 BackfillType op BackfilledIndexID: 2 TableID: 104 TemporaryIndexID: 3 -PostCommitPhase stage 6 of 7 with 1 ValidationType op +PostCommitPhase stage 6 of 6 with 1 ValidationType op transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGED] -> VALIDATED ops: *scop.ValidateUniqueIndex IndexID: 2 TableID: 104 -PostCommitPhase stage 7 of 7 with 4 MutationType ops +PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], VALIDATED] -> PUBLIC [[IndexName:{DescID: 104, Name: id1, IndexID: 2}, PUBLIC], ABSENT] -> PUBLIC + [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: *scop.SetIndexName IndexID: 2 Name: id1 TableID: 104 - *scop.MakeAddedSecondaryIndexPublic - IndexID: 2 - TableID: 104 - *scop.SetJobStateOnDescriptor - DescriptorID: 104 - *scop.UpdateSchemaChangerJob - IsNonCancelable: true - JobID: 1 -PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops - transitions: - [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY - ops: *scop.MakeDroppedIndexDeleteOnly IndexID: 3 TableID: 104 + *scop.MakeAddedSecondaryIndexPublic + IndexID: 2 + TableID: 104 *scop.SetJobStateOnDescriptor DescriptorID: 104 *scop.UpdateSchemaChangerJob @@ -312,14 +304,14 @@ PreCommitPhase stage 1 of 1 with 2 MutationType ops DescriptorIDs: - 104 JobID: 1 - RunningStatus: PostCommitPhase stage 1 of 7 with 1 MutationType op pending + RunningStatus: PostCommitPhase stage 1 of 6 with 1 MutationType op pending Statements: - statement: CREATE INVERTED INDEX CONCURRENTLY id1 ON defaultdb.t1 (id, name) STORING (money) redactedstatement: CREATE INVERTED INDEX CONCURRENTLY ‹id1› ON ‹defaultdb›.public.‹t1› (‹id›, ‹name›) STORING (‹money›) statementtag: CREATE INDEX -PostCommitPhase stage 1 of 7 with 3 MutationType ops +PostCommitPhase stage 1 of 6 with 3 MutationType ops transitions: [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], DELETE_ONLY] -> WRITE_ONLY ops: @@ -330,7 +322,7 @@ PostCommitPhase stage 1 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 2 of 7 with 1 BackfillType op +PostCommitPhase stage 2 of 6 with 1 BackfillType op transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILL_ONLY] -> BACKFILLED ops: @@ -338,7 +330,7 @@ PostCommitPhase stage 2 of 7 with 1 BackfillType op IndexID: 2 SourceIndexID: 1 TableID: 104 -PostCommitPhase stage 3 of 7 with 3 MutationType ops +PostCommitPhase stage 3 of 6 with 3 MutationType ops transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], BACKFILLED] -> DELETE_ONLY ops: @@ -349,7 +341,7 @@ PostCommitPhase stage 3 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 4 of 7 with 3 MutationType ops +PostCommitPhase stage 4 of 6 with 3 MutationType ops transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], DELETE_ONLY] -> MERGE_ONLY ops: @@ -360,7 +352,7 @@ PostCommitPhase stage 4 of 7 with 3 MutationType ops DescriptorID: 104 *scop.UpdateSchemaChangerJob JobID: 1 -PostCommitPhase stage 5 of 7 with 1 BackfillType op +PostCommitPhase stage 5 of 6 with 1 BackfillType op transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGE_ONLY] -> MERGED ops: @@ -368,37 +360,29 @@ PostCommitPhase stage 5 of 7 with 1 BackfillType op BackfilledIndexID: 2 TableID: 104 TemporaryIndexID: 3 -PostCommitPhase stage 6 of 7 with 1 ValidationType op +PostCommitPhase stage 6 of 6 with 1 ValidationType op transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], MERGED] -> VALIDATED ops: *scop.ValidateUniqueIndex IndexID: 2 TableID: 104 -PostCommitPhase stage 7 of 7 with 4 MutationType ops +PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops transitions: [[SecondaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 0, TemporaryIndexID: 3, SourceIndexID: 1}, PUBLIC], VALIDATED] -> PUBLIC [[IndexName:{DescID: 104, Name: id1, IndexID: 2}, PUBLIC], ABSENT] -> PUBLIC + [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY ops: *scop.SetIndexName IndexID: 2 Name: id1 TableID: 104 - *scop.MakeAddedSecondaryIndexPublic - IndexID: 2 - TableID: 104 - *scop.SetJobStateOnDescriptor - DescriptorID: 104 - *scop.UpdateSchemaChangerJob - IsNonCancelable: true - JobID: 1 -PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops - transitions: - [[TemporaryIndex:{DescID: 104, IndexID: 3, SourceIndexID: 1}, TRANSIENT_ABSENT], WRITE_ONLY] -> TRANSIENT_DELETE_ONLY - ops: *scop.MakeDroppedIndexDeleteOnly IndexID: 3 TableID: 104 + *scop.MakeAddedSecondaryIndexPublic + IndexID: 2 + TableID: 104 *scop.SetJobStateOnDescriptor DescriptorID: 104 *scop.UpdateSchemaChangerJob diff --git a/pkg/sql/schemachanger/screl/scalars.go b/pkg/sql/schemachanger/screl/scalars.go index 4d7dc7c01298..673c1c46b5e7 100644 --- a/pkg/sql/schemachanger/screl/scalars.go +++ b/pkg/sql/schemachanger/screl/scalars.go @@ -31,6 +31,15 @@ func GetDescID(e scpb.Element) catid.DescID { return id.(catid.DescID) } +// GetIndexID retrieves the index ID from the element if it has one. +func GetIndexID(e scpb.Element) (catid.IndexID, bool) { + v, err := Schema.GetAttribute(IndexID, e) + if err != nil { + return 0, false + } + return v.(catid.IndexID), true +} + // AllTargetDescIDs returns all the descriptor IDs referenced in the // target state's elements. This is a superset of the IDs of the descriptors // affected by the schema change. diff --git a/pkg/sql/schemachanger/testdata/alter_table_add_column b/pkg/sql/schemachanger/testdata/alter_table_add_column index 74d03841ac14..fcb4f0b87659 100644 --- a/pkg/sql/schemachanger/testdata/alter_table_add_column +++ b/pkg/sql/schemachanger/testdata/alter_table_add_column @@ -324,7 +324,7 @@ notified job registry to adopt jobs: [1] begin transaction #2 commit transaction #2 begin transaction #3 -## PostCommitPhase stage 1 of 7 with 4 MutationType ops +## PostCommitPhase stage 1 of 6 with 4 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -367,14 +367,14 @@ upsert descriptor #106 unexposedParentSchemaId: 105 - version: "2" + version: "3" -update progress of schema change job #1: "PostCommitPhase stage 2 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 2 of 6 with 1 BackfillType op pending" commit transaction #3 begin transaction #4 -## PostCommitPhase stage 2 of 7 with 1 BackfillType op +## PostCommitPhase stage 2 of 6 with 1 BackfillType op backfill indexes [2] from index #1 in table #106 commit transaction #4 begin transaction #5 -## PostCommitPhase stage 3 of 7 with 3 MutationType ops +## PostCommitPhase stage 3 of 6 with 3 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -403,10 +403,10 @@ upsert descriptor #106 unexposedParentSchemaId: 105 - version: "3" + version: "4" -update progress of schema change job #1: "PostCommitPhase stage 4 of 7 with 1 MutationType op pending" +update progress of schema change job #1: "PostCommitPhase stage 4 of 6 with 1 MutationType op pending" commit transaction #5 begin transaction #6 -## PostCommitPhase stage 4 of 7 with 3 MutationType ops +## PostCommitPhase stage 4 of 6 with 3 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -435,18 +435,18 @@ upsert descriptor #106 unexposedParentSchemaId: 105 - version: "4" + version: "5" -update progress of schema change job #1: "PostCommitPhase stage 5 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 5 of 6 with 1 BackfillType op pending" commit transaction #6 begin transaction #7 -## PostCommitPhase stage 5 of 7 with 1 BackfillType op +## PostCommitPhase stage 5 of 6 with 1 BackfillType op merge temporary indexes [3] into backfilled indexes [2] in table #106 commit transaction #7 begin transaction #8 -## PostCommitPhase stage 6 of 7 with 1 ValidationType op +## PostCommitPhase stage 6 of 6 with 1 ValidationType op validate forward indexes [2] in table #106 commit transaction #8 begin transaction #9 -## PostCommitPhase stage 7 of 7 with 8 MutationType ops +## PostCommitNonRevertiblePhase stage 1 of 3 with 10 MutationType ops upsert descriptor #106 ... oid: 20 @@ -462,10 +462,12 @@ upsert descriptor #106 createAsOfTime: wallTime: "1640995200000000000" ... + userName: root currentStatuses: - - PUBLIC - + - VALIDATED + - ABSENT + + - WRITE_ONLY + + - ABSENT + - PUBLIC - PUBLIC - PUBLIC - - WRITE_ONLY @@ -476,8 +478,9 @@ upsert descriptor #106 - PUBLIC - - MERGE_ONLY - - ABSENT - + - PUBLIC - - WRITE_ONLY + - - WRITE_ONLY + + - TRANSIENT_DELETE_ONLY + - PUBLIC - PUBLIC ... statement: ALTER TABLE db.public.tbl ADD COLUMN j INT8 NOT NULL DEFAULT 42 @@ -504,7 +507,8 @@ upsert descriptor #106 - direction: ADD - mutationId: 1 - state: DELETE_AND_WRITE_ONLY - - direction: ADD + - - direction: ADD + + - direction: DROP index: - constraintId: 2 + constraintId: 3 @@ -529,8 +533,9 @@ upsert descriptor #106 + useDeletePreservingEncoding: true version: 4 mutationId: 1 - state: DELETE_AND_WRITE_ONLY + - state: DELETE_AND_WRITE_ONLY - - direction: ADD + + state: DELETE_ONLY + - direction: DROP index: - constraintId: 3 @@ -588,28 +593,18 @@ upsert descriptor #106 - version: "5" + version: "6" adding table for stats refresh: 106 -update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops pending" +update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 3 with 1 MutationType op pending" set schema change job #1 to non-cancellable commit transaction #9 begin transaction #10 -## PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops +## PostCommitNonRevertiblePhase stage 2 of 3 with 3 MutationType ops upsert descriptor #106 ... - userName: root currentStatuses: - - - PUBLIC - - - VALIDATED - ABSENT - + - DELETE_ONLY - + - ABSENT - - PUBLIC - - PUBLIC - ... - - PUBLIC - - PUBLIC - - WRITE_ONLY - + - TRANSIENT_DELETE_ONLY - - PUBLIC + + - DELETE_ONLY + - ABSENT - PUBLIC ... formatVersion: 3 @@ -618,17 +613,7 @@ upsert descriptor #106 - wallTime: "1640995200000000009" + modificationTime: {} mutations: - - - direction: ADD - + - direction: DROP - index: - constraintId: 3 - ... - version: 4 - mutationId: 1 - - state: DELETE_AND_WRITE_ONLY - + state: DELETE_ONLY - direction: DROP - index: ... version: 4 mutationId: 1 @@ -641,10 +626,10 @@ upsert descriptor #106 unexposedParentSchemaId: 105 - version: "6" + version: "7" -update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 2 with 4 MutationType ops pending" +update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 3 of 3 with 4 MutationType ops pending" commit transaction #10 begin transaction #11 -## PostCommitNonRevertiblePhase stage 2 of 2 with 6 MutationType ops +## PostCommitNonRevertiblePhase stage 3 of 3 with 6 MutationType ops upsert descriptor #106 ... createAsOfTime: diff --git a/pkg/sql/schemachanger/testdata/alter_table_add_column_default_seq b/pkg/sql/schemachanger/testdata/alter_table_add_column_default_seq index 186c9f75c281..17fcf73a0e56 100644 --- a/pkg/sql/schemachanger/testdata/alter_table_add_column_default_seq +++ b/pkg/sql/schemachanger/testdata/alter_table_add_column_default_seq @@ -371,7 +371,7 @@ notified job registry to adopt jobs: [1] begin transaction #2 commit transaction #2 begin transaction #3 -## PostCommitPhase stage 1 of 7 with 5 MutationType ops +## PostCommitPhase stage 1 of 6 with 5 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -428,14 +428,14 @@ upsert descriptor #107 unexposedParentSchemaId: 105 - version: "2" + version: "3" -update progress of schema change job #1: "PostCommitPhase stage 2 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 2 of 6 with 1 BackfillType op pending" commit transaction #3 begin transaction #4 -## PostCommitPhase stage 2 of 7 with 1 BackfillType op +## PostCommitPhase stage 2 of 6 with 1 BackfillType op backfill indexes [2] from index #1 in table #106 commit transaction #4 begin transaction #5 -## PostCommitPhase stage 3 of 7 with 4 MutationType ops +## PostCommitPhase stage 3 of 6 with 4 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -478,10 +478,10 @@ upsert descriptor #107 unexposedParentSchemaId: 105 - version: "3" + version: "4" -update progress of schema change job #1: "PostCommitPhase stage 4 of 7 with 1 MutationType op pending" +update progress of schema change job #1: "PostCommitPhase stage 4 of 6 with 1 MutationType op pending" commit transaction #5 begin transaction #6 -## PostCommitPhase stage 4 of 7 with 4 MutationType ops +## PostCommitPhase stage 4 of 6 with 4 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -524,18 +524,18 @@ upsert descriptor #107 unexposedParentSchemaId: 105 - version: "4" + version: "5" -update progress of schema change job #1: "PostCommitPhase stage 5 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 5 of 6 with 1 BackfillType op pending" commit transaction #6 begin transaction #7 -## PostCommitPhase stage 5 of 7 with 1 BackfillType op +## PostCommitPhase stage 5 of 6 with 1 BackfillType op merge temporary indexes [3] into backfilled indexes [2] in table #106 commit transaction #7 begin transaction #8 -## PostCommitPhase stage 6 of 7 with 1 ValidationType op +## PostCommitPhase stage 6 of 6 with 1 ValidationType op validate forward indexes [2] in table #106 commit transaction #8 begin transaction #9 -## PostCommitPhase stage 7 of 7 with 9 MutationType ops +## PostCommitNonRevertiblePhase stage 1 of 3 with 11 MutationType ops upsert descriptor #106 ... oid: 20 @@ -553,10 +553,12 @@ upsert descriptor #106 createAsOfTime: wallTime: "1640995200000000000" ... + userName: root currentStatuses: - - PUBLIC - + - VALIDATED + - ABSENT + + - WRITE_ONLY + + - ABSENT + - PUBLIC - PUBLIC - PUBLIC - - WRITE_ONLY @@ -567,8 +569,9 @@ upsert descriptor #106 - PUBLIC - - MERGE_ONLY - - ABSENT - + - PUBLIC - - WRITE_ONLY + - - WRITE_ONLY + + - TRANSIENT_DELETE_ONLY + - PUBLIC - PUBLIC ... statement: ALTER TABLE db.public.tbl ADD COLUMN l INT8 NOT NULL DEFAULT nextval('db.public.sq1') @@ -597,7 +600,8 @@ upsert descriptor #106 - direction: ADD - mutationId: 1 - state: DELETE_AND_WRITE_ONLY - - direction: ADD + - - direction: ADD + + - direction: DROP index: - constraintId: 2 + constraintId: 3 @@ -622,8 +626,9 @@ upsert descriptor #106 + useDeletePreservingEncoding: true version: 4 mutationId: 1 - state: DELETE_AND_WRITE_ONLY + - state: DELETE_AND_WRITE_ONLY - - direction: ADD + + state: DELETE_ONLY + - direction: DROP index: - constraintId: 3 @@ -701,28 +706,18 @@ upsert descriptor #107 - version: "5" + version: "6" adding table for stats refresh: 106 -update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops pending" +update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 3 with 1 MutationType op pending" set schema change job #1 to non-cancellable commit transaction #9 begin transaction #10 -## PostCommitNonRevertiblePhase stage 1 of 2 with 6 MutationType ops +## PostCommitNonRevertiblePhase stage 2 of 3 with 4 MutationType ops upsert descriptor #106 ... - userName: root currentStatuses: - - - PUBLIC - - - VALIDATED - ABSENT - + - DELETE_ONLY - + - ABSENT - - PUBLIC - - PUBLIC - ... - - PUBLIC - - PUBLIC - - WRITE_ONLY - + - TRANSIENT_DELETE_ONLY - - PUBLIC + + - DELETE_ONLY + - ABSENT - PUBLIC ... formatVersion: 3 @@ -731,17 +726,7 @@ upsert descriptor #106 - wallTime: "1640995200000000009" + modificationTime: {} mutations: - - - direction: ADD - + - direction: DROP - index: - constraintId: 3 - ... - version: 4 - mutationId: 1 - - state: DELETE_AND_WRITE_ONLY - + state: DELETE_ONLY - direction: DROP - index: ... version: 4 mutationId: 1 @@ -768,10 +753,10 @@ upsert descriptor #107 unexposedParentSchemaId: 105 - version: "6" + version: "7" -update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 2 with 4 MutationType ops pending" +update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 3 of 3 with 4 MutationType ops pending" commit transaction #10 begin transaction #11 -## PostCommitNonRevertiblePhase stage 2 of 2 with 7 MutationType ops +## PostCommitNonRevertiblePhase stage 3 of 3 with 7 MutationType ops upsert descriptor #106 ... createAsOfTime: diff --git a/pkg/sql/schemachanger/testdata/create_index b/pkg/sql/schemachanger/testdata/create_index index 844d74de2625..282369eb1f6b 100644 --- a/pkg/sql/schemachanger/testdata/create_index +++ b/pkg/sql/schemachanger/testdata/create_index @@ -205,7 +205,7 @@ notified job registry to adopt jobs: [1] begin transaction #2 commit transaction #2 begin transaction #3 -## PostCommitPhase stage 1 of 7 with 3 MutationType ops +## PostCommitPhase stage 1 of 6 with 3 MutationType ops upsert descriptor #106 ... - BACKFILL_ONLY @@ -234,14 +234,14 @@ upsert descriptor #106 unexposedParentSchemaId: 101 - version: "2" + version: "3" -update progress of schema change job #1: "PostCommitPhase stage 2 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 2 of 6 with 1 BackfillType op pending" commit transaction #3 begin transaction #4 -## PostCommitPhase stage 2 of 7 with 1 BackfillType op +## PostCommitPhase stage 2 of 6 with 1 BackfillType op backfill indexes [2] from index #1 in table #106 commit transaction #4 begin transaction #5 -## PostCommitPhase stage 3 of 7 with 3 MutationType ops +## PostCommitPhase stage 3 of 6 with 3 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -270,10 +270,10 @@ upsert descriptor #106 unexposedParentSchemaId: 101 - version: "3" + version: "4" -update progress of schema change job #1: "PostCommitPhase stage 4 of 7 with 1 MutationType op pending" +update progress of schema change job #1: "PostCommitPhase stage 4 of 6 with 1 MutationType op pending" commit transaction #5 begin transaction #6 -## PostCommitPhase stage 4 of 7 with 3 MutationType ops +## PostCommitPhase stage 4 of 6 with 3 MutationType ops upsert descriptor #106 ... - PUBLIC @@ -302,28 +302,32 @@ upsert descriptor #106 unexposedParentSchemaId: 101 - version: "4" + version: "5" -update progress of schema change job #1: "PostCommitPhase stage 5 of 7 with 1 BackfillType op pending" +update progress of schema change job #1: "PostCommitPhase stage 5 of 6 with 1 BackfillType op pending" commit transaction #6 begin transaction #7 -## PostCommitPhase stage 5 of 7 with 1 BackfillType op +## PostCommitPhase stage 5 of 6 with 1 BackfillType op merge temporary indexes [3] into backfilled indexes [2] in table #106 commit transaction #7 begin transaction #8 -## PostCommitPhase stage 6 of 7 with 1 ValidationType op +## PostCommitPhase stage 6 of 6 with 1 ValidationType op validate forward indexes [2] in table #106 commit transaction #8 begin transaction #9 -## PostCommitPhase stage 7 of 7 with 4 MutationType ops +## PostCommitNonRevertiblePhase stage 1 of 2 with 5 MutationType ops upsert descriptor #106 ... - PUBLIC - PUBLIC - - MERGE_ONLY - - ABSENT + - - WRITE_ONLY + - PUBLIC + - PUBLIC + + - TRANSIENT_DELETE_ONLY + - PUBLIC + - PUBLIC - - WRITE_ONLY - - PUBLIC + jobId: "1" + relevantStatements: ... statement: CREATE INDEX idx1 ON t (v) WHERE (v = 'a') statementTag: CREATE INDEX @@ -357,7 +361,8 @@ upsert descriptor #106 + version: 4 + modificationTime: {} mutations: - - direction: ADD + - - direction: ADD + + - direction: DROP index: - constraintId: 2 - createdExplicitly: true @@ -384,35 +389,6 @@ upsert descriptor #106 - index: constraintId: 3 createdExplicitly: true - ... - time: {} - unexposedParentSchemaId: 101 - - version: "5" - + version: "6" -update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 1 of 2 with 1 MutationType op pending" -set schema change job #1 to non-cancellable -commit transaction #9 -begin transaction #10 -## PostCommitNonRevertiblePhase stage 1 of 2 with 3 MutationType ops -upsert descriptor #106 - ... - - PUBLIC - - PUBLIC - - - WRITE_ONLY - + - TRANSIENT_DELETE_ONLY - - PUBLIC - - PUBLIC - ... - storeColumnNames: [] - version: 4 - - modificationTime: - - wallTime: "1640995200000000009" - + modificationTime: {} - mutations: - - - direction: ADD - + - direction: DROP - index: - constraintId: 3 ... version: 4 mutationId: 1 @@ -423,11 +399,12 @@ upsert descriptor #106 ... time: {} unexposedParentSchemaId: 101 - - version: "6" - + version: "7" + - version: "5" + + version: "6" update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 2 with 2 MutationType ops pending" -commit transaction #10 -begin transaction #11 +set schema change job #1 to non-cancellable +commit transaction #9 +begin transaction #10 ## PostCommitNonRevertiblePhase stage 2 of 2 with 4 MutationType ops upsert descriptor #106 ... @@ -533,7 +510,7 @@ upsert descriptor #106 storeColumnNames: [] version: 4 - modificationTime: - - wallTime: "1640995200000000010" + - wallTime: "1640995200000000009" - mutations: - - direction: DROP - index: @@ -566,12 +543,12 @@ upsert descriptor #106 ... time: {} unexposedParentSchemaId: 101 - - version: "7" - + version: "8" + - version: "6" + + version: "7" write *eventpb.FinishSchemaChange to event log for descriptor 106 create job #2 (non-cancelable: true): "GC for " descriptor IDs: [106] update progress of schema change job #1: "all stages completed" -commit transaction #11 +commit transaction #10 notified job registry to adopt jobs: [2] # end PostCommitPhase From e9f1c056cde6054f05e40206cfea0e16491616b5 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 14 Jul 2022 07:55:04 -0700 Subject: [PATCH 5/7] rowexec: increase the batch size for join reader ordering strategy This commit increases the default value of `sql.distsql.join_reader_ordering_strategy.batch_size` cluster setting (which determines the input row batch size for the lookup joins when ordering needs to be maintained) from 10KiB to 100KiB since the previous number is likely to have been too conservative. We have seen support cases (https://github.com/cockroachlabs/support/issues/1627) where bumping up this setting was needed to get reasonable performance, we also change this setting to 100KiB in our TPC-E setup (https://github.com/cockroachlabs/tpc-e/blob/d47d3ea5ce71ecb1be5e0e466a8aa7c94af63d17/tier-a/src/schema.rs#L404). I did some historical digging, and I believe that the number 10KiB was chosen somewhat arbitrarily with no real justification in #48058. That PR changed how we measure the input row batches from the number of rows to the memory footprint of the input rows. Prior to that change we had 100 rows limit, so my guess that 10KiB was somewhat dependent on that number. The reason we don't want this batch size to be too large is that we buffer all looked up rows in a disk-backed container, so if too many responses come back (because we included too many input rows in the batch), that container has to spill to disk. To make sure we don't regress in such scenarios this commit teaches the join reader to lower the batch size bytes limit if the container does spill to disk, until 10 KiB which is treated as the minimum. Release note: None --- .../testdata/logic_test/information_schema | 2 +- .../logictest/testdata/logic_test/pg_catalog | 4 ++-- .../logictest/testdata/logic_test/show_source | 2 +- pkg/sql/rowexec/joinreader.go | 19 +++++++++++++++++++ pkg/sql/rowexec/joinreader_strategies.go | 6 ++---- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 234310929302..a326ecb4f0eb 100644 --- a/pkg/sql/logictest/testdata/logic_test/information_schema +++ b/pkg/sql/logictest/testdata/logic_test/information_schema @@ -4657,7 +4657,7 @@ integer_datetimes on intervalstyle postgres intervalstyle_enabled on is_superuser on -join_reader_ordering_strategy_batch_size 10 KiB +join_reader_ordering_strategy_batch_size 100 KiB large_full_scan_rows 1000 lc_collate C.UTF-8 lc_ctype C.UTF-8 diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog index a8cd5c37fd6d..cb9bcd9f2580 100644 --- a/pkg/sql/logictest/testdata/logic_test/pg_catalog +++ b/pkg/sql/logictest/testdata/logic_test/pg_catalog @@ -4186,7 +4186,7 @@ inject_retry_errors_enabled off NULL integer_datetimes on NULL NULL NULL string intervalstyle postgres NULL NULL NULL string is_superuser on NULL NULL NULL string -join_reader_ordering_strategy_batch_size 10 KiB NULL NULL NULL string +join_reader_ordering_strategy_batch_size 100 KiB NULL NULL NULL string large_full_scan_rows 1000 NULL NULL NULL string lc_collate C.UTF-8 NULL NULL NULL string lc_ctype C.UTF-8 NULL NULL NULL string @@ -4310,7 +4310,7 @@ inject_retry_errors_enabled off NULL integer_datetimes on NULL user NULL on on intervalstyle postgres NULL user NULL postgres postgres is_superuser on NULL user NULL on on -join_reader_ordering_strategy_batch_size 10 KiB NULL user NULL 10 KiB 10 KiB +join_reader_ordering_strategy_batch_size 100 KiB NULL user NULL 100 KiB 100 KiB large_full_scan_rows 1000 NULL user NULL 1000 1000 lc_collate C.UTF-8 NULL user NULL C.UTF-8 C.UTF-8 lc_ctype C.UTF-8 NULL user NULL C.UTF-8 C.UTF-8 diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source index 9fd11d1270e9..e044c5b6f14b 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_source +++ b/pkg/sql/logictest/testdata/logic_test/show_source @@ -78,7 +78,7 @@ inject_retry_errors_enabled off integer_datetimes on intervalstyle postgres is_superuser on -join_reader_ordering_strategy_batch_size 10 KiB +join_reader_ordering_strategy_batch_size 100 KiB large_full_scan_rows 1000 lc_collate C.UTF-8 lc_ctype C.UTF-8 diff --git a/pkg/sql/rowexec/joinreader.go b/pkg/sql/rowexec/joinreader.go index b7f21e06c2e8..c95434ae76d9 100644 --- a/pkg/sql/rowexec/joinreader.go +++ b/pkg/sql/rowexec/joinreader.go @@ -1047,9 +1047,28 @@ func (jr *joinReader) performLookup() (joinReaderState, *execinfrapb.ProducerMet log.VEvent(jr.Ctx, 1, "done joining rows") jr.strategy.prepareToEmit(jr.Ctx) + // Check if the strategy spilled to disk and reduce the batch size if it + // did. + // TODO(yuzefovich): we should probably also grow the batch size bytes limit + // dynamically if we haven't spilled and are not close to spilling (say not + // exceeding half of the memory limit of the disk-backed container), up to + // some limit. (This would only apply to the joinReaderOrderingStrategy + // since other strategies cannot spill in the first place.) Probably it'd be + // good to look at not just the current batch of input rows, but to keep + // some statistics over the last several batches to make a more informed + // decision. + if jr.strategy.spilled() && jr.batchSizeBytes > joinReaderMinBatchSize { + jr.batchSizeBytes = jr.batchSizeBytes / 2 + if jr.batchSizeBytes < joinReaderMinBatchSize { + jr.batchSizeBytes = joinReaderMinBatchSize + } + } + return jrEmittingRows, nil } +const joinReaderMinBatchSize = 10 << 10 /* 10 KiB */ + // emitRow returns the next row from jr.toEmit, if present. Otherwise it // prepares for another input batch. func (jr *joinReader) emitRow() ( diff --git a/pkg/sql/rowexec/joinreader_strategies.go b/pkg/sql/rowexec/joinreader_strategies.go index e497fcbe73d6..36fde25c1455 100644 --- a/pkg/sql/rowexec/joinreader_strategies.go +++ b/pkg/sql/rowexec/joinreader_strategies.go @@ -451,7 +451,7 @@ var partialJoinSentinel = []int{-1} // // Say the joinReader looks up rows in order: (red, x), then (blue, y). Once // (red, x) is fetched, it is handed to -// joinReaderOderingStrategy.processLookedUpRow(), which will match it against +// joinReaderOrderingStrategy.processLookedUpRow(), which will match it against // all the corresponding input rows, producing (1, x), (4, x). These two rows // are not emitted because that would violate the input ordering (well, (1, x) // could be emitted, but we're not smart enough). So, they are buffered until @@ -535,7 +535,7 @@ type joinReaderOrderingStrategy struct { testingInfoSpilled bool } -const joinReaderOrderingStrategyBatchSizeDefault = 10 << 10 /* 10 KiB */ +const joinReaderOrderingStrategyBatchSizeDefault = 100 << 10 /* 100 KiB */ // JoinReaderOrderingStrategyBatchSize determines the size of input batches used // to construct a single lookup KV batch by joinReaderOrderingStrategy. @@ -548,8 +548,6 @@ var JoinReaderOrderingStrategyBatchSize = settings.RegisterByteSizeSetting( ) func (s *joinReaderOrderingStrategy) getLookupRowsBatchSizeHint(sd *sessiondata.SessionData) int64 { - // TODO(asubiotto): Eventually we might want to adjust this batch size - // dynamically based on whether the result row container spilled or not. if sd.JoinReaderOrderingStrategyBatchSize == 0 { // In some tests the session data might not be set - use the default // value then. From 784f20a264a3b494e803982d8e4265268068d830 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Tue, 12 Jul 2022 16:59:31 -0700 Subject: [PATCH 6/7] cmd: add smith command Add a new top-level command `smith` which dumps randomly-generated sqlsmith queries. This is useful for testing modifications to sqlsmith. Assists: #83024 Release note: None --- .github/CODEOWNERS | 1 + pkg/BUILD.bazel | 3 + pkg/cmd/dev/build.go | 3 + pkg/cmd/smith/BUILD.bazel | 22 +++++ pkg/cmd/smith/main.go | 149 ++++++++++++++++++++++++++++++ pkg/internal/sqlsmith/schema.go | 1 + pkg/internal/sqlsmith/sqlsmith.go | 22 ++++- 7 files changed, 198 insertions(+), 3 deletions(-) create mode 100644 pkg/cmd/smith/BUILD.bazel create mode 100644 pkg/cmd/smith/main.go diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 28a3afcb72c2..9bec248b4c88 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -271,6 +271,7 @@ /pkg/cmd/skip-test/ @cockroachdb/test-eng /pkg/cmd/skiperrs/ @cockroachdb/sql-experience /pkg/cmd/skipped-tests/ @cockroachdb/test-eng +/pkg/cmd/smith/ @cockroachdb/sql-queries /pkg/cmd/smithcmp/ @cockroachdb/sql-queries /pkg/cmd/smithtest/ @cockroachdb/sql-queries /pkg/cmd/teamcity-trigger/ @cockroachdb/dev-inf diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel index 16b4e142bd91..d0ab6eb93422 100644 --- a/pkg/BUILD.bazel +++ b/pkg/BUILD.bazel @@ -858,6 +858,8 @@ GO_TARGETS = [ "//pkg/cmd/skiperrs:skiperrs_lib", "//pkg/cmd/skipped-tests:skipped-tests", "//pkg/cmd/skipped-tests:skipped-tests_lib", + "//pkg/cmd/smith:smith", + "//pkg/cmd/smith:smith_lib", "//pkg/cmd/smithcmp:smithcmp", "//pkg/cmd/smithcmp:smithcmp_lib", "//pkg/cmd/smithtest:smithtest", @@ -2104,6 +2106,7 @@ GET_X_DATA_TARGETS = [ "//pkg/cmd/skip-test:get_x_data", "//pkg/cmd/skiperrs:get_x_data", "//pkg/cmd/skipped-tests:get_x_data", + "//pkg/cmd/smith:get_x_data", "//pkg/cmd/smithcmp:get_x_data", "//pkg/cmd/smithtest:get_x_data", "//pkg/cmd/teamcity-trigger:get_x_data", diff --git a/pkg/cmd/dev/build.go b/pkg/cmd/dev/build.go index 490905f61e79..b2b9c1e1cd77 100644 --- a/pkg/cmd/dev/build.go +++ b/pkg/cmd/dev/build.go @@ -99,6 +99,9 @@ var buildTargetMapping = map[string]string{ "roachprod-stress": "//pkg/cmd/roachprod-stress:roachprod-stress", "roachtest": "//pkg/cmd/roachtest:roachtest", "short": "//pkg/cmd/cockroach-short:cockroach-short", + "smith": "//pkg/cmd/smith:smith", + "smithcmp": "//pkg/cmd/smithcmp:smithcmp", + "smithtest": "//pkg/cmd/smithtest:smithtest", "staticcheck": "@co_honnef_go_tools//cmd/staticcheck:staticcheck", "stress": stressTarget, "swagger": "@com_github_go_swagger_go_swagger//cmd/swagger:swagger", diff --git a/pkg/cmd/smith/BUILD.bazel b/pkg/cmd/smith/BUILD.bazel new file mode 100644 index 000000000000..9654da21e193 --- /dev/null +++ b/pkg/cmd/smith/BUILD.bazel @@ -0,0 +1,22 @@ +load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data") +load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library") + +go_library( + name = "smith_lib", + srcs = ["main.go"], + importpath = "github.com/cockroachdb/cockroach/pkg/cmd/smith", + visibility = ["//visibility:private"], + deps = [ + "//pkg/internal/sqlsmith", + "//pkg/util/randutil", + "@com_github_cockroachdb_errors//:errors", + ], +) + +go_binary( + name = "smith", + embed = [":smith_lib"], + visibility = ["//visibility:public"], +) + +get_x_data(name = "get_x_data") diff --git a/pkg/cmd/smith/main.go b/pkg/cmd/smith/main.go new file mode 100644 index 000000000000..2b5ef31563a8 --- /dev/null +++ b/pkg/cmd/smith/main.go @@ -0,0 +1,149 @@ +// Copyright 2022 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 main + +import ( + gosql "database/sql" + "flag" + "fmt" + "os" + "sort" + + "github.com/cockroachdb/cockroach/pkg/internal/sqlsmith" + "github.com/cockroachdb/cockroach/pkg/util/randutil" + "github.com/cockroachdb/errors" +) + +// smith is a command-line wrapper around sqlsmith-go, useful for examining +// sqlsmith output when making changes to the random query generator. + +const description = ` +%[1]s prints random SQL statements to stdout, using a random SQL statement +generator inspired by SQLsmith. See https://github.com/anse1/sqlsmith for more +about the SQLsmith project. + +Usage: + [COCKROACH_RANDOM_SEED=1234] %[1]s [options] [sqlsmith-go options] + +Options: +` + +var ( + flags = flag.NewFlagSet(os.Args[0], flag.ContinueOnError) + expr = flags.Bool("expr", false, "generate expressions instead of statements") + num = flags.Int("num", 1, "number of statements / expressions to generate") + url = flags.String("url", "", "database to fetch schema from") + smitherOptMap = map[string]sqlsmith.SmitherOption{ + "DisableMutations": sqlsmith.DisableMutations(), + "DisableDDLs": sqlsmith.DisableDDLs(), + "OnlyNoDropDDLs": sqlsmith.OnlyNoDropDDLs(), + "MultiRegionDDLs": sqlsmith.MultiRegionDDLs(), + "DisableWith": sqlsmith.DisableWith(), + "DisableImpureFns": sqlsmith.DisableImpureFns(), + "DisableCRDBFns": sqlsmith.DisableCRDBFns(), + "SimpleDatums": sqlsmith.SimpleDatums(), + "MutationsOnly": sqlsmith.MutationsOnly(), + "InsUpdOnly": sqlsmith.InsUpdOnly(), + "DisableLimits": sqlsmith.DisableLimits(), + "AvoidConsts": sqlsmith.AvoidConsts(), + "DisableWindowFuncs": sqlsmith.DisableWindowFuncs(), + "OutputSort": sqlsmith.OutputSort(), + "UnlikelyConstantPredicate": sqlsmith.UnlikelyConstantPredicate(), + "FavorCommonData": sqlsmith.FavorCommonData(), + "UnlikelyRandomNulls": sqlsmith.UnlikelyRandomNulls(), + "DisableCrossJoins": sqlsmith.DisableCrossJoins(), + "DisableIndexHints": sqlsmith.DisableIndexHints(), + "LowProbabilityWhereClauseWithJoinTables": sqlsmith.LowProbabilityWhereClauseWithJoinTables(), + "DisableInsertSelect": sqlsmith.DisableInsertSelect(), + "CompareMode": sqlsmith.CompareMode(), + "PostgresMode": sqlsmith.PostgresMode(), + "MutatingMode": sqlsmith.MutatingMode(), + } + smitherOpts []string +) + +func usage() { + fmt.Fprintf(flags.Output(), description, os.Args[0]) + flags.PrintDefaults() + fmt.Fprint(flags.Output(), "\nSqlsmith-go options:\n") + for _, opt := range smitherOpts { + fmt.Fprintln(flags.Output(), " ", opt) + } +} + +func init() { + smitherOpts = make([]string, 0, len(smitherOptMap)) + for opt := range smitherOptMap { + smitherOpts = append(smitherOpts, opt) + } + sort.Strings(smitherOpts) + flags.Usage = usage +} + +func main() { + if err := flags.Parse(os.Args[1:]); err != nil { + if errors.Is(err, flag.ErrHelp) { + os.Exit(0) + } else { + os.Exit(1) + } + } + + rng, seed := randutil.NewPseudoRand() + fmt.Print("-- COCKROACH_RANDOM_SEED=", seed, "\n") + + var smitherOpts []sqlsmith.SmitherOption + for _, arg := range flags.Args() { + if opt, ok := smitherOptMap[arg]; ok { + fmt.Print("-- ", arg, ": ", opt, "\n") + smitherOpts = append(smitherOpts, opt) + } else { + fmt.Fprintf(flags.Output(), "unrecognized sqlsmith-go option: %v\n", arg) + usage() + os.Exit(2) + } + } + + var db *gosql.DB + if *url != "" { + var err error + db, err = gosql.Open("postgres", *url) + if err != nil { + fmt.Fprintf(flags.Output(), "could not connect to database\n") + os.Exit(3) + } + defer db.Close() + if err := db.Ping(); err != nil { + fmt.Fprintf(flags.Output(), "could not ping database\n") + os.Exit(4) + } + fmt.Println("-- connected to", *url) + } + + smither, err := sqlsmith.NewSmither(db, rng, smitherOpts...) + if err != nil { + fmt.Println(err) + os.Exit(1) + } + defer smither.Close() + + fmt.Println("-- num", *num) + if *expr { + fmt.Println("-- expr") + for i := 0; i < *num; i++ { + fmt.Print("\n", smither.GenerateExpr(), "\n") + } + } else { + for i := 0; i < *num; i++ { + fmt.Print("\n", smither.Generate(), ";\n") + } + } +} diff --git a/pkg/internal/sqlsmith/schema.go b/pkg/internal/sqlsmith/schema.go index e3f8471e84ea..a090e95d5e92 100644 --- a/pkg/internal/sqlsmith/schema.go +++ b/pkg/internal/sqlsmith/schema.go @@ -54,6 +54,7 @@ type tableRefs []*tableRef // ReloadSchemas loads tables from the database. func (s *Smither) ReloadSchemas() error { if s.db == nil { + s.schemas = []*schemaRef{{SchemaName: "public"}} return nil } s.lock.Lock() diff --git a/pkg/internal/sqlsmith/sqlsmith.go b/pkg/internal/sqlsmith/sqlsmith.go index f01c7f265f18..8757b94c0b35 100644 --- a/pkg/internal/sqlsmith/sqlsmith.go +++ b/pkg/internal/sqlsmith/sqlsmith.go @@ -138,9 +138,11 @@ func NewSmither(db *gosql.DB, rnd *rand.Rand, opts ...SmitherOption) (*Smither, s.scalarExprSampler = newWeightedScalarExprSampler(s.scalarExprWeights, rnd.Int63()) s.boolExprSampler = newWeightedScalarExprSampler(s.boolExprWeights, rnd.Int63()) s.enableBulkIO() - row := s.db.QueryRow("SELECT current_database()") - if err := row.Scan(&s.dbName); err != nil { - return nil, err + if s.db != nil { + row := s.db.QueryRow("SELECT current_database()") + if err := row.Scan(&s.dbName); err != nil { + return nil, err + } } return s, s.ReloadSchemas() } @@ -475,3 +477,17 @@ var PostgresMode = multiOption( IgnoreFNs("^postgis_.*version"), IgnoreFNs("^postgis_.*scripts"), ) + +// MutatingMode causes the Smither to generate mutation statements in the same +// way as the query-comparison roachtests (costfuzz and +// unoptimized-query-oracle). +var MutatingMode = multiOption( + "mutating mode", + MutationsOnly(), + FavorCommonData(), + UnlikelyRandomNulls(), + DisableInsertSelect(), + DisableCrossJoins(), + SetComplexity(.05), + SetScalarComplexity(.01), +) From f118f5d547fbd2695bcf5d285ae45d502a170098 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Tue, 12 Jul 2022 17:03:32 -0700 Subject: [PATCH 7/7] sqlsmith: make order-dependent aggregation functions deterministic Some aggregation functions (e.g. string_agg) have results that depend on the order of input rows. To make sqlsmith more deterministic, add ORDER BY clauses to these aggregation functions whenever their argument is a column reference. (When their argument is a constant, ordering doesn't matter.) Fixes: #83024 Release note: None --- .../roachtest/tests/query_comparison_util.go | 2 +- pkg/cmd/roachtest/tests/tlp.go | 2 +- pkg/cmd/smith/main.go | 38 +++++++++--------- pkg/internal/sqlsmith/scalar.go | 40 ++++++++++++++++--- pkg/internal/sqlsmith/sqlsmith.go | 10 ++--- 5 files changed, 61 insertions(+), 31 deletions(-) diff --git a/pkg/cmd/roachtest/tests/query_comparison_util.go b/pkg/cmd/roachtest/tests/query_comparison_util.go index 3de1198e8819..eb0e386b4d50 100644 --- a/pkg/cmd/roachtest/tests/query_comparison_util.go +++ b/pkg/cmd/roachtest/tests/query_comparison_util.go @@ -177,7 +177,7 @@ func runOneRoundQueryComparison( // Initialize a smither that generates only deterministic SELECT statements. smither, err := sqlsmith.NewSmither(conn, rnd, - sqlsmith.DisableMutations(), sqlsmith.DisableImpureFns(), sqlsmith.DisableLimits(), + sqlsmith.DisableMutations(), sqlsmith.DisableNondeterministicFns(), sqlsmith.DisableLimits(), sqlsmith.UnlikelyConstantPredicate(), sqlsmith.FavorCommonData(), sqlsmith.UnlikelyRandomNulls(), sqlsmith.DisableCrossJoins(), sqlsmith.DisableIndexHints(), sqlsmith.DisableWith(), diff --git a/pkg/cmd/roachtest/tests/tlp.go b/pkg/cmd/roachtest/tests/tlp.go index 78966bcc02e0..530f8e82e61f 100644 --- a/pkg/cmd/roachtest/tests/tlp.go +++ b/pkg/cmd/roachtest/tests/tlp.go @@ -147,7 +147,7 @@ func runOneTLP( // Initialize a smither that will never generate mutations. tlpSmither, err := sqlsmith.NewSmither(conn, rnd, - sqlsmith.DisableMutations(), sqlsmith.DisableImpureFns()) + sqlsmith.DisableMutations(), sqlsmith.DisableNondeterministicFns()) if err != nil { t.Fatal(err) } diff --git a/pkg/cmd/smith/main.go b/pkg/cmd/smith/main.go index 2b5ef31563a8..da9bde36628d 100644 --- a/pkg/cmd/smith/main.go +++ b/pkg/cmd/smith/main.go @@ -42,25 +42,25 @@ var ( num = flags.Int("num", 1, "number of statements / expressions to generate") url = flags.String("url", "", "database to fetch schema from") smitherOptMap = map[string]sqlsmith.SmitherOption{ - "DisableMutations": sqlsmith.DisableMutations(), - "DisableDDLs": sqlsmith.DisableDDLs(), - "OnlyNoDropDDLs": sqlsmith.OnlyNoDropDDLs(), - "MultiRegionDDLs": sqlsmith.MultiRegionDDLs(), - "DisableWith": sqlsmith.DisableWith(), - "DisableImpureFns": sqlsmith.DisableImpureFns(), - "DisableCRDBFns": sqlsmith.DisableCRDBFns(), - "SimpleDatums": sqlsmith.SimpleDatums(), - "MutationsOnly": sqlsmith.MutationsOnly(), - "InsUpdOnly": sqlsmith.InsUpdOnly(), - "DisableLimits": sqlsmith.DisableLimits(), - "AvoidConsts": sqlsmith.AvoidConsts(), - "DisableWindowFuncs": sqlsmith.DisableWindowFuncs(), - "OutputSort": sqlsmith.OutputSort(), - "UnlikelyConstantPredicate": sqlsmith.UnlikelyConstantPredicate(), - "FavorCommonData": sqlsmith.FavorCommonData(), - "UnlikelyRandomNulls": sqlsmith.UnlikelyRandomNulls(), - "DisableCrossJoins": sqlsmith.DisableCrossJoins(), - "DisableIndexHints": sqlsmith.DisableIndexHints(), + "DisableMutations": sqlsmith.DisableMutations(), + "DisableDDLs": sqlsmith.DisableDDLs(), + "OnlyNoDropDDLs": sqlsmith.OnlyNoDropDDLs(), + "MultiRegionDDLs": sqlsmith.MultiRegionDDLs(), + "DisableWith": sqlsmith.DisableWith(), + "DisableNondeterministicFns": sqlsmith.DisableNondeterministicFns(), + "DisableCRDBFns": sqlsmith.DisableCRDBFns(), + "SimpleDatums": sqlsmith.SimpleDatums(), + "MutationsOnly": sqlsmith.MutationsOnly(), + "InsUpdOnly": sqlsmith.InsUpdOnly(), + "DisableLimits": sqlsmith.DisableLimits(), + "AvoidConsts": sqlsmith.AvoidConsts(), + "DisableWindowFuncs": sqlsmith.DisableWindowFuncs(), + "OutputSort": sqlsmith.OutputSort(), + "UnlikelyConstantPredicate": sqlsmith.UnlikelyConstantPredicate(), + "FavorCommonData": sqlsmith.FavorCommonData(), + "UnlikelyRandomNulls": sqlsmith.UnlikelyRandomNulls(), + "DisableCrossJoins": sqlsmith.DisableCrossJoins(), + "DisableIndexHints": sqlsmith.DisableIndexHints(), "LowProbabilityWhereClauseWithJoinTables": sqlsmith.LowProbabilityWhereClauseWithJoinTables(), "DisableInsertSelect": sqlsmith.DisableInsertSelect(), "CompareMode": sqlsmith.CompareMode(), diff --git a/pkg/internal/sqlsmith/scalar.go b/pkg/internal/sqlsmith/scalar.go index 13e0ada6b03e..08fc5c77c604 100644 --- a/pkg/internal/sqlsmith/scalar.go +++ b/pkg/internal/sqlsmith/scalar.go @@ -401,7 +401,7 @@ func makeFunc(s *Smither, ctx Context, typ *types.T, refs colRefs) (tree.TypedEx return nil, false } fn := fns[s.rnd.Intn(len(fns))] - if s.disableImpureFns && fn.overload.Volatility > volatility.Immutable { + if s.disableNondeterministicFns && fn.overload.Volatility > volatility.Immutable { return nil, false } for _, ignore := range s.ignoreFNs { @@ -410,6 +410,9 @@ func makeFunc(s *Smither, ctx Context, typ *types.T, refs colRefs) (tree.TypedEx } } + // Some aggregation functions benefit from an order by clause. + var orderExpr tree.Expr + args := make(tree.TypedExprs, 0) for _, argTyp := range fn.overload.Types.Types() { // Postgres is picky about having Int4 arguments instead of Int8. @@ -421,6 +424,9 @@ func makeFunc(s *Smither, ctx Context, typ *types.T, refs colRefs) (tree.TypedEx if class == tree.AggregateClass || class == tree.WindowClass { var ok bool arg, ok = makeColRef(s, argTyp, refs) + if ok && len(args) == 0 { + orderExpr = arg + } if !ok { // If we can't find a col ref for our aggregate function, just use a // constant. @@ -469,9 +475,7 @@ func makeFunc(s *Smither, ctx Context, typ *types.T, refs colRefs) (tree.TypedEx } } - // Cast the return and arguments to prevent ambiguity during function - // implementation choosing. - return castType(tree.NewTypedFuncExpr( + funcExpr := tree.NewTypedFuncExpr( tree.ResolvableFunctionReference{FunctionReference: fn.def}, 0, /* aggQualifier */ args, @@ -480,7 +484,33 @@ func makeFunc(s *Smither, ctx Context, typ *types.T, refs colRefs) (tree.TypedEx typ, &fn.def.FunctionProperties, fn.overload, - ), typ), true + ) + + // Some aggregation functions need an order by clause to be deterministic. + if s.disableNondeterministicFns { + switch fn.def.Name { + case "array_agg", + "concat_agg", + "json_agg", + "json_object_agg", + "jsonb_agg", + "jsonb_object_agg", + "st_makeline", + "string_agg", + "xmlagg": + if orderExpr != nil { + funcExpr.AggType = tree.GeneralAgg + funcExpr.OrderBy = tree.OrderBy{{ + Expr: orderExpr, + Direction: s.randDirection(), + }} + } + } + } + + // Cast the return and arguments to prevent ambiguity during function + // implementation choosing. + return castType(funcExpr, typ), true } var windowFrameModes = []treewindow.WindowFrameMode{ diff --git a/pkg/internal/sqlsmith/sqlsmith.go b/pkg/internal/sqlsmith/sqlsmith.go index 8757b94c0b35..fa8eceed510f 100644 --- a/pkg/internal/sqlsmith/sqlsmith.go +++ b/pkg/internal/sqlsmith/sqlsmith.go @@ -79,7 +79,7 @@ type Smither struct { scalarExprSampler, boolExprSampler *scalarExprSampler disableWith bool - disableImpureFns bool + disableNondeterministicFns bool disableLimits bool disableWindowFuncs bool simpleDatums bool @@ -318,9 +318,9 @@ var DisableWith = simpleOption("disable WITH", func(s *Smither) { s.disableWith = true }) -// DisableImpureFns causes the Smither to disable impure functions. -var DisableImpureFns = simpleOption("disable impure funcs", func(s *Smither) { - s.disableImpureFns = true +// DisableNondeterministicFns causes the Smither to disable nondeterministic functions. +var DisableNondeterministicFns = simpleOption("disable nondeterministic funcs", func(s *Smither) { + s.disableNondeterministicFns = true }) // DisableCRDBFns causes the Smither to disable crdb_internal functions. @@ -435,7 +435,7 @@ var DisableInsertSelect = simpleOption("disable insert select", func(s *Smither) var CompareMode = multiOption( "compare mode", DisableMutations(), - DisableImpureFns(), + DisableNondeterministicFns(), DisableCRDBFns(), IgnoreFNs("^version"), DisableLimits(),