From 04d93908e546e2b8d822b098341594d1d905f71b Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 14 Apr 2021 12:46:32 -0400 Subject: [PATCH 1/5] gcjob: share ClearRange logic for tables and indexes No obvious reason to keep these separate. Release note: None --- pkg/sql/gcjob/index_garbage_collection.go | 22 +++++++++++----------- pkg/sql/gcjob/table_garbage_collection.go | 18 ++++++++++++------ 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/pkg/sql/gcjob/index_garbage_collection.go b/pkg/sql/gcjob/index_garbage_collection.go index 4e4895d1444b..03dc3eb94054 100644 --- a/pkg/sql/gcjob/index_garbage_collection.go +++ b/pkg/sql/gcjob/index_garbage_collection.go @@ -14,6 +14,7 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/sql" @@ -116,17 +117,16 @@ func clearIndex( } sp := tableDesc.IndexSpan(execCfg.Codec, index.ID) - - // ClearRange cannot be run in a transaction, so create a - // non-transactional batch to send the request. - b := &kv.Batch{} - b.AddRawRequest(&roachpb.ClearRangeRequest{ - RequestHeader: roachpb.RequestHeader{ - Key: sp.Key, - EndKey: sp.EndKey, - }, - }) - return execCfg.DB.Run(ctx, b) + start, err := keys.Addr(sp.Key) + if err != nil { + return errors.Errorf("failed to addr index start: %v", err) + } + end, err := keys.Addr(sp.EndKey) + if err != nil { + return errors.Errorf("failed to addr index end: %v", err) + } + rSpan := roachpb.RSpan{Key: start, EndKey: end} + return clearSpanData(ctx, execCfg.DB, execCfg.DistSender, rSpan) } // completeDroppedIndexes updates the mutations of the table descriptor to diff --git a/pkg/sql/gcjob/table_garbage_collection.go b/pkg/sql/gcjob/table_garbage_collection.go index 7d8277cc25ab..40d30baf3aee 100644 --- a/pkg/sql/gcjob/table_garbage_collection.go +++ b/pkg/sql/gcjob/table_garbage_collection.go @@ -101,6 +101,12 @@ func ClearTableData( tableKey := roachpb.RKey(codec.TablePrefix(uint32(table.GetID()))) tableSpan := roachpb.RSpan{Key: tableKey, EndKey: tableKey.PrefixEnd()} + return clearSpanData(ctx, db, distSender, tableSpan) +} + +func clearSpanData( + ctx context.Context, db *kv.DB, distSender *kvcoord.DistSender, span roachpb.RSpan, +) error { // ClearRange requests lays down RocksDB range deletion tombstones that have // serious performance implications (#24029). The logic below attempts to @@ -126,20 +132,20 @@ func ClearTableData( const waitTime = 500 * time.Millisecond var n int - lastKey := tableSpan.Key + lastKey := span.Key ri := kvcoord.NewRangeIterator(distSender) timer := timeutil.NewTimer() defer timer.Stop() - for ri.Seek(ctx, tableSpan.Key, kvcoord.Ascending); ; ri.Next(ctx) { + for ri.Seek(ctx, span.Key, kvcoord.Ascending); ; ri.Next(ctx) { if !ri.Valid() { return ri.Error() } - if n++; n >= batchSize || !ri.NeedAnother(tableSpan) { + if n++; n >= batchSize || !ri.NeedAnother(span) { endKey := ri.Desc().EndKey - if tableSpan.EndKey.Less(endKey) { - endKey = tableSpan.EndKey + if span.EndKey.Less(endKey) { + endKey = span.EndKey } var b kv.Batch b.AddRawRequest(&roachpb.ClearRangeRequest{ @@ -163,7 +169,7 @@ func ClearTableData( } } - if !ri.NeedAnother(tableSpan) { + if !ri.NeedAnother(span) { break } } From b66e11e1d77f7ee8cc387d6a336f1aa648160534 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Wed, 14 Apr 2021 14:35:15 -0400 Subject: [PATCH 2/5] kv/kvserver: skip TestClosedTimestampWorksWhenRequestsAreSentToNonLeaseHolders Refs: #60682 Reason: flaky test Generated by bin/skip-test. Release justification: non-production code changes Release note: None --- pkg/kv/kvserver/client_closed_timestamp_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/kv/kvserver/client_closed_timestamp_test.go b/pkg/kv/kvserver/client_closed_timestamp_test.go index cfd72e273439..2ff9e711cf0b 100644 --- a/pkg/kv/kvserver/client_closed_timestamp_test.go +++ b/pkg/kv/kvserver/client_closed_timestamp_test.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -35,6 +36,7 @@ import ( // becomes the leaseholder. See #48553 for more details. func TestClosedTimestampWorksWhenRequestsAreSentToNonLeaseHolders(t *testing.T) { defer leaktest.AfterTest(t)() + skip.WithIssue(t, 60682, "flaky test") defer log.Scope(t).Close(t) ctx := context.Background() From 1ba3b585b7dfebac27fe26d750af00bb8049d6f2 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 9 Apr 2021 13:55:35 -0700 Subject: [PATCH 3/5] opt: make check constraint and partial index columns anonymous This commit updates optbuilder so that synthesized columns used for maintaining check constraints and partial indexes are anonymized in the scope resulting from their projection. This prevents these columns from being found in the scope when resolving columns while building expressions. This simplifies some code and eliminates a class of bugs that can cause "ambiguous column" errors when a user's table has column names that match the synthesized column names. Now that these synthesized columns are anonymous, there cannot be ambiguity between them and table columns. Release note: None --- pkg/sql/opt/optbuilder/insert.go | 16 +- pkg/sql/opt/optbuilder/mutation_builder.go | 13 +- .../opt/optbuilder/testdata/partial-indexes | 246 ++++++++++-------- pkg/sql/opt/optbuilder/update.go | 8 +- 4 files changed, 156 insertions(+), 127 deletions(-) diff --git a/pkg/sql/opt/optbuilder/insert.go b/pkg/sql/opt/optbuilder/insert.go index b72af626b89b..a47c5065785b 100644 --- a/pkg/sql/opt/optbuilder/insert.go +++ b/pkg/sql/opt/optbuilder/insert.go @@ -652,16 +652,11 @@ func (mb *mutationBuilder) buildInsert(returning tree.ReturningExprs) { // check constraint, refer to the correct columns. mb.disambiguateColumns() - // Keep a reference to the scope before the check constraint columns are - // projected. We use this scope when projecting the partial index put - // columns because the check columns are not in-scope for those expressions. - preCheckScope := mb.outScope - // Add any check constraint boolean columns to the input. mb.addCheckConstraintCols() // Project partial index PUT boolean columns. - mb.projectPartialIndexPutCols(preCheckScope) + mb.projectPartialIndexPutCols(mb.outScope) mb.buildUniqueChecksForInsert() @@ -869,11 +864,6 @@ func (mb *mutationBuilder) buildUpsert(returning tree.ReturningExprs) { // check constraint, refer to the correct columns. mb.disambiguateColumns() - // Keep a reference to the scope before the check constraint columns are - // projected. We use this scope when projecting the partial index put - // columns because the check columns are not in-scope for those expressions. - preCheckScope := mb.outScope - // Add any check constraint boolean columns to the input. mb.addCheckConstraintCols() @@ -890,9 +880,9 @@ func (mb *mutationBuilder) buildUpsert(returning tree.ReturningExprs) { // mb.fetchScope will be nil. Therefore, we only project partial index // PUT columns. if mb.needExistingRows() { - mb.projectPartialIndexPutAndDelCols(preCheckScope, mb.fetchScope) + mb.projectPartialIndexPutAndDelCols(mb.outScope, mb.fetchScope) } else { - mb.projectPartialIndexPutCols(preCheckScope) + mb.projectPartialIndexPutCols(mb.outScope) } mb.buildUniqueChecksForUpsert() diff --git a/pkg/sql/opt/optbuilder/mutation_builder.go b/pkg/sql/opt/optbuilder/mutation_builder.go index 140cf293dbd0..6e9fa95dabc9 100644 --- a/pkg/sql/opt/optbuilder/mutation_builder.go +++ b/pkg/sql/opt/optbuilder/mutation_builder.go @@ -837,8 +837,8 @@ func findRoundingFunction( } // addCheckConstraintCols synthesizes a boolean output column for each check -// constraint defined on the target table. The mutation operator will report -// a constraint violation error if the value of the column is false. +// constraint defined on the target table. The mutation operator will report a +// constraint violation error if the value of the column is false. func (mb *mutationBuilder) addCheckConstraintCols() { if mb.tab.CheckCount() != 0 { projectionsScope := mb.outScope.replace() @@ -860,6 +860,9 @@ func (mb *mutationBuilder) addCheckConstraintCols() { referencedCols := &opt.ColSet{} mb.b.buildScalar(texpr, mb.outScope, projectionsScope, scopeCol, referencedCols) + // Clear the column name so that it cannot be referenced. + scopeCol.clearName() + // Synthesized check columns are only necessary if the columns // referenced in the check expression are being mutated. If they are // not being mutated, we do not add the newly built column to @@ -967,6 +970,9 @@ func (mb *mutationBuilder) projectPartialIndexColsImpl(putScope, delScope *scope mb.b.buildScalar(texpr, putScope, projectionScope, scopeCol, nil) mb.partialIndexPutColIDs[ord] = scopeCol.id + + // Clear the column name so that it cannot be referenced. + scopeCol.clearName() } // Build synthesized DEL columns. @@ -977,6 +983,9 @@ func (mb *mutationBuilder) projectPartialIndexColsImpl(putScope, delScope *scope mb.b.buildScalar(texpr, delScope, projectionScope, scopeCol, nil) mb.partialIndexDelColIDs[ord] = scopeCol.id + + // Clear the column name so that it cannot be referenced. + scopeCol.clearName() } ord++ diff --git a/pkg/sql/opt/optbuilder/testdata/partial-indexes b/pkg/sql/opt/optbuilder/testdata/partial-indexes index 1518757530f8..d37ad9323310 100644 --- a/pkg/sql/opt/optbuilder/testdata/partial-indexes +++ b/pkg/sql/opt/optbuilder/testdata/partial-indexes @@ -37,8 +37,11 @@ exec-ddl CREATE TABLE ambig ( partial_index_put1 INT, partial_index_del1 INT, + check1 INT, + CHECK (partial_index_put1 > 10), UNIQUE INDEX (partial_index_put1) WHERE partial_index_put1 > 0, - UNIQUE INDEX (partial_index_del1) WHERE partial_index_del1 > 0 + UNIQUE INDEX (partial_index_del1) WHERE partial_index_del1 > 0, + INDEX (partial_index_put1) WHERE check1 > 0 ) ---- @@ -112,28 +115,34 @@ insert partial_indexes # Do not error with "column reference is ambiguous" when table column names # match synthesized column names. build -INSERT INTO ambig (partial_index_put1) VALUES (1) +INSERT INTO ambig (partial_index_put1, partial_index_del1, check1) VALUES (1, 2, 3) ---- insert ambig ├── columns: ├── insert-mapping: - │ ├── column1:5 => ambig.partial_index_put1:1 - │ ├── column6:6 => partial_index_del1:2 - │ └── column7:7 => rowid:3 - ├── partial index put columns: partial_index_put1:8 partial_index_put2:9 + │ ├── column1:6 => ambig.partial_index_put1:1 + │ ├── column2:7 => partial_index_del1:2 + │ ├── column3:8 => ambig.check1:3 + │ └── column9:9 => rowid:4 + ├── check columns: check1:10 + ├── partial index put columns: partial_index_put1:11 partial_index_put2:12 partial_index_put3:13 └── project - ├── columns: partial_index_put1:8!null partial_index_put2:9 column1:5!null column6:6 column7:7 + ├── columns: partial_index_put1:11!null partial_index_put2:12!null partial_index_put3:13!null column1:6!null column2:7!null column3:8!null column9:9 check1:10!null ├── project - │ ├── columns: column6:6 column7:7 column1:5!null - │ ├── values - │ │ ├── columns: column1:5!null - │ │ └── (1,) + │ ├── columns: check1:10!null column1:6!null column2:7!null column3:8!null column9:9 + │ ├── project + │ │ ├── columns: column9:9 column1:6!null column2:7!null column3:8!null + │ │ ├── values + │ │ │ ├── columns: column1:6!null column2:7!null column3:8!null + │ │ │ └── (1, 2, 3) + │ │ └── projections + │ │ └── unique_rowid() [as=column9:9] │ └── projections - │ ├── NULL::INT8 [as=column6:6] - │ └── unique_rowid() [as=column7:7] + │ └── column1:6 > 10 [as=check1:10] └── projections - ├── column1:5 > 0 [as=partial_index_put1:8] - └── column6:6 > 0 [as=partial_index_put2:9] + ├── column1:6 > 0 [as=partial_index_put1:11] + ├── column2:7 > 0 [as=partial_index_put2:12] + └── column3:8 > 0 [as=partial_index_put3:13] build INSERT INTO comp (a, b, c) VALUES (2, 1, 'Foo') @@ -221,24 +230,27 @@ DELETE FROM ambig WHERE partial_index_del1 = 5 ---- delete ambig ├── columns: - ├── fetch columns: partial_index_put1:5 ambig.partial_index_del1:6 rowid:7 - ├── partial index del columns: partial_index_del1:9 partial_index_del2:10 + ├── fetch columns: partial_index_put1:6 ambig.partial_index_del1:7 check1:8 rowid:9 + ├── partial index del columns: partial_index_del1:11 partial_index_del2:12 partial_index_del3:13 └── project - ├── columns: partial_index_del1:9 partial_index_del2:10!null partial_index_put1:5 ambig.partial_index_del1:6!null rowid:7!null crdb_internal_mvcc_timestamp:8 + ├── columns: partial_index_del1:11 partial_index_del2:12!null partial_index_del3:13 partial_index_put1:6 ambig.partial_index_del1:7!null check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 ├── select - │ ├── columns: partial_index_put1:5 ambig.partial_index_del1:6!null rowid:7!null crdb_internal_mvcc_timestamp:8 + │ ├── columns: partial_index_put1:6 ambig.partial_index_del1:7!null check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 │ ├── scan ambig - │ │ ├── columns: partial_index_put1:5 ambig.partial_index_del1:6 rowid:7!null crdb_internal_mvcc_timestamp:8 + │ │ ├── columns: partial_index_put1:6 ambig.partial_index_del1:7 check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 │ │ └── partial index predicates │ │ ├── secondary: filters - │ │ │ └── partial_index_put1:5 > 0 + │ │ │ └── partial_index_put1:6 > 0 + │ │ ├── secondary: filters + │ │ │ └── ambig.partial_index_del1:7 > 0 │ │ └── secondary: filters - │ │ └── ambig.partial_index_del1:6 > 0 + │ │ └── check1:8 > 0 │ └── filters - │ └── ambig.partial_index_del1:6 = 5 + │ └── ambig.partial_index_del1:7 = 5 └── projections - ├── partial_index_put1:5 > 0 [as=partial_index_del1:9] - └── ambig.partial_index_del1:6 > 0 [as=partial_index_del2:10] + ├── partial_index_put1:6 > 0 [as=partial_index_del1:11] + ├── ambig.partial_index_del1:7 > 0 [as=partial_index_del2:12] + └── check1:8 > 0 [as=partial_index_del3:13] build DELETE FROM comp @@ -399,35 +411,43 @@ UPDATE ambig SET partial_index_put1 = 1, partial_index_del1 = 2 WHERE partial_in ---- update ambig ├── columns: - ├── fetch columns: ambig.partial_index_put1:5 ambig.partial_index_del1:6 rowid:7 + ├── fetch columns: ambig.partial_index_put1:6 ambig.partial_index_del1:7 ambig.check1:8 rowid:9 ├── update-mapping: - │ ├── partial_index_put1_new:9 => ambig.partial_index_put1:1 - │ └── partial_index_del1_new:10 => ambig.partial_index_del1:2 - ├── partial index put columns: partial_index_put1:11 partial_index_put2:13 - ├── partial index del columns: partial_index_del1:12 partial_index_del2:14 + │ ├── partial_index_put1_new:11 => ambig.partial_index_put1:1 + │ └── partial_index_del1_new:12 => ambig.partial_index_del1:2 + ├── check columns: check1:13 + ├── partial index put columns: partial_index_put1:14 partial_index_put2:16 partial_index_put3:18 + ├── partial index del columns: partial_index_del1:15 partial_index_del2:17 partial_index_put3:18 └── project - ├── columns: partial_index_put1:11!null partial_index_del1:12!null partial_index_put2:13!null partial_index_del2:14!null ambig.partial_index_put1:5!null ambig.partial_index_del1:6!null rowid:7!null crdb_internal_mvcc_timestamp:8 partial_index_put1_new:9!null partial_index_del1_new:10!null + ├── columns: partial_index_put1:14!null partial_index_del1:15!null partial_index_put2:16!null partial_index_del2:17!null partial_index_put3:18 ambig.partial_index_put1:6!null ambig.partial_index_del1:7!null ambig.check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 partial_index_put1_new:11!null partial_index_del1_new:12!null check1:13!null ├── project - │ ├── columns: partial_index_put1_new:9!null partial_index_del1_new:10!null ambig.partial_index_put1:5!null ambig.partial_index_del1:6!null rowid:7!null crdb_internal_mvcc_timestamp:8 - │ ├── select - │ │ ├── columns: ambig.partial_index_put1:5!null ambig.partial_index_del1:6!null rowid:7!null crdb_internal_mvcc_timestamp:8 - │ │ ├── scan ambig - │ │ │ ├── columns: ambig.partial_index_put1:5 ambig.partial_index_del1:6 rowid:7!null crdb_internal_mvcc_timestamp:8 - │ │ │ └── partial index predicates - │ │ │ ├── secondary: filters - │ │ │ │ └── ambig.partial_index_put1:5 > 0 - │ │ │ └── secondary: filters - │ │ │ └── ambig.partial_index_del1:6 > 0 - │ │ └── filters - │ │ └── (ambig.partial_index_put1:5 = 10) AND (ambig.partial_index_del1:6 = 20) + │ ├── columns: check1:13!null ambig.partial_index_put1:6!null ambig.partial_index_del1:7!null ambig.check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 partial_index_put1_new:11!null partial_index_del1_new:12!null + │ ├── project + │ │ ├── columns: partial_index_put1_new:11!null partial_index_del1_new:12!null ambig.partial_index_put1:6!null ambig.partial_index_del1:7!null ambig.check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 + │ │ ├── select + │ │ │ ├── columns: ambig.partial_index_put1:6!null ambig.partial_index_del1:7!null ambig.check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 + │ │ │ ├── scan ambig + │ │ │ │ ├── columns: ambig.partial_index_put1:6 ambig.partial_index_del1:7 ambig.check1:8 rowid:9!null crdb_internal_mvcc_timestamp:10 + │ │ │ │ └── partial index predicates + │ │ │ │ ├── secondary: filters + │ │ │ │ │ └── ambig.partial_index_put1:6 > 0 + │ │ │ │ ├── secondary: filters + │ │ │ │ │ └── ambig.partial_index_del1:7 > 0 + │ │ │ │ └── secondary: filters + │ │ │ │ └── ambig.check1:8 > 0 + │ │ │ └── filters + │ │ │ └── (ambig.partial_index_put1:6 = 10) AND (ambig.partial_index_del1:7 = 20) + │ │ └── projections + │ │ ├── 1 [as=partial_index_put1_new:11] + │ │ └── 2 [as=partial_index_del1_new:12] │ └── projections - │ ├── 1 [as=partial_index_put1_new:9] - │ └── 2 [as=partial_index_del1_new:10] + │ └── partial_index_put1_new:11 > 10 [as=check1:13] └── projections - ├── partial_index_put1_new:9 > 0 [as=partial_index_put1:11] - ├── ambig.partial_index_put1:5 > 0 [as=partial_index_del1:12] - ├── partial_index_del1_new:10 > 0 [as=partial_index_put2:13] - └── ambig.partial_index_del1:6 > 0 [as=partial_index_del2:14] + ├── partial_index_put1_new:11 > 0 [as=partial_index_put1:14] + ├── ambig.partial_index_put1:6 > 0 [as=partial_index_del1:15] + ├── partial_index_del1_new:12 > 0 [as=partial_index_put2:16] + ├── ambig.partial_index_del1:7 > 0 [as=partial_index_del2:17] + └── ambig.check1:8 > 0 [as=partial_index_put3:18] build UPDATE comp SET b = 1 @@ -1320,72 +1340,86 @@ DO UPDATE SET partial_index_put1 = 10, partial_index_del1 = 20 upsert ambig ├── columns: ├── arbiter indexes: secondary - ├── canary column: rowid:11 - ├── fetch columns: ambig.partial_index_put1:9 ambig.partial_index_del1:10 rowid:11 + ├── canary column: rowid:14 + ├── fetch columns: ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14 ├── insert-mapping: - │ ├── column1:5 => ambig.partial_index_put1:1 - │ ├── column2:6 => ambig.partial_index_del1:2 - │ └── column7:7 => rowid:3 + │ ├── column1:6 => ambig.partial_index_put1:1 + │ ├── column2:7 => ambig.partial_index_del1:2 + │ ├── column8:8 => ambig.check1:3 + │ └── column9:9 => rowid:4 ├── update-mapping: - │ ├── upsert_partial_index_put1:15 => ambig.partial_index_put1:1 - │ └── upsert_partial_index_del1:16 => ambig.partial_index_del1:2 - ├── partial index put columns: partial_index_put1:18 partial_index_put2:20 - ├── partial index del columns: partial_index_del1:19 partial_index_del2:21 + │ ├── upsert_partial_index_put1:18 => ambig.partial_index_put1:1 + │ └── upsert_partial_index_del1:19 => ambig.partial_index_del1:2 + ├── check columns: check1:22 + ├── partial index put columns: partial_index_put1:23 partial_index_put2:25 partial_index_put3:27 + ├── partial index del columns: partial_index_del1:24 partial_index_del2:26 partial_index_del3:28 └── project - ├── columns: partial_index_put1:18!null partial_index_del1:19 partial_index_put2:20!null partial_index_del2:21 column1:5!null column2:6!null column7:7 ambig.partial_index_put1:9 ambig.partial_index_del1:10 rowid:11 crdb_internal_mvcc_timestamp:12 partial_index_put1_new:13!null partial_index_del1_new:14!null upsert_partial_index_put1:15!null upsert_partial_index_del1:16!null upsert_rowid:17 + ├── columns: partial_index_put1:23!null partial_index_del1:24 partial_index_put2:25!null partial_index_del2:26 partial_index_put3:27 partial_index_del3:28 column1:6!null column2:7!null column8:8 column9:9 ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14 crdb_internal_mvcc_timestamp:15 partial_index_put1_new:16!null partial_index_del1_new:17!null upsert_partial_index_put1:18!null upsert_partial_index_del1:19!null upsert_check1:20 upsert_rowid:21 check1:22!null ├── project - │ ├── columns: upsert_partial_index_put1:15!null upsert_partial_index_del1:16!null upsert_rowid:17 column1:5!null column2:6!null column7:7 ambig.partial_index_put1:9 ambig.partial_index_del1:10 rowid:11 crdb_internal_mvcc_timestamp:12 partial_index_put1_new:13!null partial_index_del1_new:14!null + │ ├── columns: check1:22!null column1:6!null column2:7!null column8:8 column9:9 ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14 crdb_internal_mvcc_timestamp:15 partial_index_put1_new:16!null partial_index_del1_new:17!null upsert_partial_index_put1:18!null upsert_partial_index_del1:19!null upsert_check1:20 upsert_rowid:21 │ ├── project - │ │ ├── columns: partial_index_put1_new:13!null partial_index_del1_new:14!null column1:5!null column2:6!null column7:7 ambig.partial_index_put1:9 ambig.partial_index_del1:10 rowid:11 crdb_internal_mvcc_timestamp:12 - │ │ ├── left-join (hash) - │ │ │ ├── columns: column1:5!null column2:6!null column7:7 ambig.partial_index_put1:9 ambig.partial_index_del1:10 rowid:11 crdb_internal_mvcc_timestamp:12 - │ │ │ ├── project - │ │ │ │ ├── columns: column1:5!null column2:6!null column7:7 - │ │ │ │ └── ensure-upsert-distinct-on - │ │ │ │ ├── columns: column1:5!null column2:6!null column7:7 arbiter_secondary_distinct:8 - │ │ │ │ ├── grouping columns: column1:5!null arbiter_secondary_distinct:8 - │ │ │ │ ├── project - │ │ │ │ │ ├── columns: arbiter_secondary_distinct:8 column1:5!null column2:6!null column7:7 - │ │ │ │ │ ├── project - │ │ │ │ │ │ ├── columns: column7:7 column1:5!null column2:6!null - │ │ │ │ │ │ ├── values - │ │ │ │ │ │ │ ├── columns: column1:5!null column2:6!null - │ │ │ │ │ │ │ └── (1, 2) - │ │ │ │ │ │ └── projections - │ │ │ │ │ │ └── unique_rowid() [as=column7:7] - │ │ │ │ │ └── projections - │ │ │ │ │ └── (column1:5 > 0) OR NULL::BOOL [as=arbiter_secondary_distinct:8] - │ │ │ │ └── aggregations - │ │ │ │ ├── first-agg [as=column2:6] - │ │ │ │ │ └── column2:6 - │ │ │ │ └── first-agg [as=column7:7] - │ │ │ │ └── column7:7 - │ │ │ ├── select - │ │ │ │ ├── columns: ambig.partial_index_put1:9!null ambig.partial_index_del1:10 rowid:11!null crdb_internal_mvcc_timestamp:12 - │ │ │ │ ├── scan ambig - │ │ │ │ │ ├── columns: ambig.partial_index_put1:9 ambig.partial_index_del1:10 rowid:11!null crdb_internal_mvcc_timestamp:12 - │ │ │ │ │ └── partial index predicates - │ │ │ │ │ ├── secondary: filters - │ │ │ │ │ │ └── ambig.partial_index_put1:9 > 0 - │ │ │ │ │ └── secondary: filters - │ │ │ │ │ └── ambig.partial_index_del1:10 > 0 + │ │ ├── columns: upsert_partial_index_put1:18!null upsert_partial_index_del1:19!null upsert_check1:20 upsert_rowid:21 column1:6!null column2:7!null column8:8 column9:9 ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14 crdb_internal_mvcc_timestamp:15 partial_index_put1_new:16!null partial_index_del1_new:17!null + │ │ ├── project + │ │ │ ├── columns: partial_index_put1_new:16!null partial_index_del1_new:17!null column1:6!null column2:7!null column8:8 column9:9 ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14 crdb_internal_mvcc_timestamp:15 + │ │ │ ├── left-join (hash) + │ │ │ │ ├── columns: column1:6!null column2:7!null column8:8 column9:9 ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14 crdb_internal_mvcc_timestamp:15 + │ │ │ │ ├── project + │ │ │ │ │ ├── columns: column1:6!null column2:7!null column8:8 column9:9 + │ │ │ │ │ └── ensure-upsert-distinct-on + │ │ │ │ │ ├── columns: column1:6!null column2:7!null column8:8 column9:9 arbiter_secondary_distinct:10 + │ │ │ │ │ ├── grouping columns: column1:6!null arbiter_secondary_distinct:10 + │ │ │ │ │ ├── project + │ │ │ │ │ │ ├── columns: arbiter_secondary_distinct:10 column1:6!null column2:7!null column8:8 column9:9 + │ │ │ │ │ │ ├── project + │ │ │ │ │ │ │ ├── columns: column8:8 column9:9 column1:6!null column2:7!null + │ │ │ │ │ │ │ ├── values + │ │ │ │ │ │ │ │ ├── columns: column1:6!null column2:7!null + │ │ │ │ │ │ │ │ └── (1, 2) + │ │ │ │ │ │ │ └── projections + │ │ │ │ │ │ │ ├── NULL::INT8 [as=column8:8] + │ │ │ │ │ │ │ └── unique_rowid() [as=column9:9] + │ │ │ │ │ │ └── projections + │ │ │ │ │ │ └── (column1:6 > 0) OR NULL::BOOL [as=arbiter_secondary_distinct:10] + │ │ │ │ │ └── aggregations + │ │ │ │ │ ├── first-agg [as=column2:7] + │ │ │ │ │ │ └── column2:7 + │ │ │ │ │ ├── first-agg [as=column8:8] + │ │ │ │ │ │ └── column8:8 + │ │ │ │ │ └── first-agg [as=column9:9] + │ │ │ │ │ └── column9:9 + │ │ │ │ ├── select + │ │ │ │ │ ├── columns: ambig.partial_index_put1:11!null ambig.partial_index_del1:12 ambig.check1:13 rowid:14!null crdb_internal_mvcc_timestamp:15 + │ │ │ │ │ ├── scan ambig + │ │ │ │ │ │ ├── columns: ambig.partial_index_put1:11 ambig.partial_index_del1:12 ambig.check1:13 rowid:14!null crdb_internal_mvcc_timestamp:15 + │ │ │ │ │ │ └── partial index predicates + │ │ │ │ │ │ ├── secondary: filters + │ │ │ │ │ │ │ └── ambig.partial_index_put1:11 > 0 + │ │ │ │ │ │ ├── secondary: filters + │ │ │ │ │ │ │ └── ambig.partial_index_del1:12 > 0 + │ │ │ │ │ │ └── secondary: filters + │ │ │ │ │ │ └── ambig.check1:13 > 0 + │ │ │ │ │ └── filters + │ │ │ │ │ └── ambig.partial_index_put1:11 > 0 │ │ │ │ └── filters - │ │ │ │ └── ambig.partial_index_put1:9 > 0 - │ │ │ └── filters - │ │ │ ├── column1:5 = ambig.partial_index_put1:9 - │ │ │ └── column1:5 > 0 + │ │ │ │ ├── column1:6 = ambig.partial_index_put1:11 + │ │ │ │ └── column1:6 > 0 + │ │ │ └── projections + │ │ │ ├── 10 [as=partial_index_put1_new:16] + │ │ │ └── 20 [as=partial_index_del1_new:17] │ │ └── projections - │ │ ├── 10 [as=partial_index_put1_new:13] - │ │ └── 20 [as=partial_index_del1_new:14] + │ │ ├── CASE WHEN rowid:14 IS NULL THEN column1:6 ELSE partial_index_put1_new:16 END [as=upsert_partial_index_put1:18] + │ │ ├── CASE WHEN rowid:14 IS NULL THEN column2:7 ELSE partial_index_del1_new:17 END [as=upsert_partial_index_del1:19] + │ │ ├── CASE WHEN rowid:14 IS NULL THEN column8:8 ELSE ambig.check1:13 END [as=upsert_check1:20] + │ │ └── CASE WHEN rowid:14 IS NULL THEN column9:9 ELSE rowid:14 END [as=upsert_rowid:21] │ └── projections - │ ├── CASE WHEN rowid:11 IS NULL THEN column1:5 ELSE partial_index_put1_new:13 END [as=upsert_partial_index_put1:15] - │ ├── CASE WHEN rowid:11 IS NULL THEN column2:6 ELSE partial_index_del1_new:14 END [as=upsert_partial_index_del1:16] - │ └── CASE WHEN rowid:11 IS NULL THEN column7:7 ELSE rowid:11 END [as=upsert_rowid:17] + │ └── upsert_partial_index_put1:18 > 10 [as=check1:22] └── projections - ├── upsert_partial_index_put1:15 > 0 [as=partial_index_put1:18] - ├── ambig.partial_index_put1:9 > 0 [as=partial_index_del1:19] - ├── upsert_partial_index_del1:16 > 0 [as=partial_index_put2:20] - └── ambig.partial_index_del1:10 > 0 [as=partial_index_del2:21] + ├── upsert_partial_index_put1:18 > 0 [as=partial_index_put1:23] + ├── ambig.partial_index_put1:11 > 0 [as=partial_index_del1:24] + ├── upsert_partial_index_del1:19 > 0 [as=partial_index_put2:25] + ├── ambig.partial_index_del1:12 > 0 [as=partial_index_del2:26] + ├── upsert_check1:20 > 0 [as=partial_index_put3:27] + └── ambig.check1:13 > 0 [as=partial_index_del3:28] exec-ddl CREATE UNIQUE INDEX u1 ON comp (b) WHERE d = 'foo' diff --git a/pkg/sql/opt/optbuilder/update.go b/pkg/sql/opt/optbuilder/update.go index ea5d0c33da15..7266e03264b4 100644 --- a/pkg/sql/opt/optbuilder/update.go +++ b/pkg/sql/opt/optbuilder/update.go @@ -329,11 +329,7 @@ func (mb *mutationBuilder) buildUpdate(returning tree.ReturningExprs) { // check constraint, refer to the correct columns. mb.disambiguateColumns() - // Keep a reference to the scope before the check constraint columns are - // projected. We use this scope when projecting the partial index put - // columns because the check columns are not in-scope for those expressions. - preCheckScope := mb.outScope - + // Add any check constraint boolean columns to the input. mb.addCheckConstraintCols() // Add the partial index predicate expressions to the table metadata. @@ -342,7 +338,7 @@ func (mb *mutationBuilder) buildUpdate(returning tree.ReturningExprs) { mb.b.addPartialIndexPredicatesForTable(mb.md.TableMeta(mb.tabID), nil /* scan */) // Project partial index PUT and DEL boolean columns. - mb.projectPartialIndexPutAndDelCols(preCheckScope, mb.fetchScope) + mb.projectPartialIndexPutAndDelCols(mb.outScope, mb.fetchScope) mb.buildUniqueChecksForUpdate() From 1c26999e2e79d68ec6b2dcabe5f1bd6389cb7835 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 9 Apr 2021 14:09:37 -0700 Subject: [PATCH 4/5] opt: simplify partial index PUT/DEL column building Previously, the scopes to build synthesized partial index PUT and DEL columns were passed as input the building functions. They were always called with `mb.outScope` or `mb.fetchScope`, so there was no need to provide them as arguments. The building functions no longer accept any arguments. Release note: None --- pkg/sql/opt/optbuilder/delete.go | 2 +- pkg/sql/opt/optbuilder/insert.go | 6 ++-- pkg/sql/opt/optbuilder/mutation_builder.go | 40 +++++----------------- pkg/sql/opt/optbuilder/update.go | 2 +- 4 files changed, 13 insertions(+), 37 deletions(-) diff --git a/pkg/sql/opt/optbuilder/delete.go b/pkg/sql/opt/optbuilder/delete.go index 1db1736ab8d4..5b6fd2b0373b 100644 --- a/pkg/sql/opt/optbuilder/delete.go +++ b/pkg/sql/opt/optbuilder/delete.go @@ -77,7 +77,7 @@ func (mb *mutationBuilder) buildDelete(returning tree.ReturningExprs) { mb.buildFKChecksAndCascadesForDelete() // Project partial index DEL boolean columns. - mb.projectPartialIndexDelCols(mb.fetchScope) + mb.projectPartialIndexDelCols() private := mb.makeMutationPrivate(returning != nil) mb.outScope.expr = mb.b.factory.ConstructDelete( diff --git a/pkg/sql/opt/optbuilder/insert.go b/pkg/sql/opt/optbuilder/insert.go index a47c5065785b..27362a0494dd 100644 --- a/pkg/sql/opt/optbuilder/insert.go +++ b/pkg/sql/opt/optbuilder/insert.go @@ -656,7 +656,7 @@ func (mb *mutationBuilder) buildInsert(returning tree.ReturningExprs) { mb.addCheckConstraintCols() // Project partial index PUT boolean columns. - mb.projectPartialIndexPutCols(mb.outScope) + mb.projectPartialIndexPutCols() mb.buildUniqueChecksForInsert() @@ -880,9 +880,9 @@ func (mb *mutationBuilder) buildUpsert(returning tree.ReturningExprs) { // mb.fetchScope will be nil. Therefore, we only project partial index // PUT columns. if mb.needExistingRows() { - mb.projectPartialIndexPutAndDelCols(mb.outScope, mb.fetchScope) + mb.projectPartialIndexPutAndDelCols() } else { - mb.projectPartialIndexPutCols(mb.outScope) + mb.projectPartialIndexPutCols() } mb.buildUniqueChecksForUpsert() diff --git a/pkg/sql/opt/optbuilder/mutation_builder.go b/pkg/sql/opt/optbuilder/mutation_builder.go index 6e9fa95dabc9..1578a2a06dbe 100644 --- a/pkg/sql/opt/optbuilder/mutation_builder.go +++ b/pkg/sql/opt/optbuilder/mutation_builder.go @@ -896,47 +896,23 @@ func (mb *mutationBuilder) mutationColumnIDs() opt.ColSet { // projectPartialIndexPutCols builds a Project that synthesizes boolean PUT // columns for each partial index defined on the target table. See // partialIndexPutColIDs for more info on these columns. -// -// putScope must contain the columns representing the values of each mutated row -// AFTER the mutation is applied. -func (mb *mutationBuilder) projectPartialIndexPutCols(putScope *scope) { - if putScope == nil { - panic(errors.AssertionFailedf("cannot project partial index PUT columns with nil scope")) - } - mb.projectPartialIndexColsImpl(putScope, nil /* delScope */) +func (mb *mutationBuilder) projectPartialIndexPutCols() { + mb.projectPartialIndexColsImpl(mb.outScope, nil /* delScope */) } // projectPartialIndexDelCols builds a Project that synthesizes boolean PUT // columns for each partial index defined on the target table. See // partialIndexDelColIDs for more info on these columns. -// -// delScope must contain the columns representing the values of each mutated row -// BEFORE the mutation is applied. -func (mb *mutationBuilder) projectPartialIndexDelCols(delScope *scope) { - if delScope == nil { - panic(errors.AssertionFailedf("cannot project partial index DEL columns with nil scope")) - } - mb.projectPartialIndexColsImpl(nil /* putScope */, delScope) +func (mb *mutationBuilder) projectPartialIndexDelCols() { + mb.projectPartialIndexColsImpl(nil /* putScope */, mb.fetchScope) } -// projectPartialIndexPutAndDelCols builds a Project that synthesizes boolean PUT and -// DEL columns for each partial index defined on the target table. See +// projectPartialIndexPutAndDelCols builds a Project that synthesizes boolean +// PUT and DEL columns for each partial index defined on the target table. See // partialIndexPutColIDs and partialIndexDelColIDs for more info on these // columns. -// -// putScope must contain the columns representing the values of each mutated row -// AFTER the mutation is applied. -// -// delScope must contain the columns representing the values of each mutated row -// BEFORE the mutation is applied. -func (mb *mutationBuilder) projectPartialIndexPutAndDelCols(putScope, delScope *scope) { - if putScope == nil { - panic(errors.AssertionFailedf("cannot project partial index PUT columns with nil scope")) - } - if delScope == nil { - panic(errors.AssertionFailedf("cannot project partial index DEL columns with nil scope")) - } - mb.projectPartialIndexColsImpl(putScope, delScope) +func (mb *mutationBuilder) projectPartialIndexPutAndDelCols() { + mb.projectPartialIndexColsImpl(mb.outScope, mb.fetchScope) } // projectPartialIndexColsImpl builds a Project that synthesizes boolean PUT and diff --git a/pkg/sql/opt/optbuilder/update.go b/pkg/sql/opt/optbuilder/update.go index 7266e03264b4..552f4dc2c541 100644 --- a/pkg/sql/opt/optbuilder/update.go +++ b/pkg/sql/opt/optbuilder/update.go @@ -338,7 +338,7 @@ func (mb *mutationBuilder) buildUpdate(returning tree.ReturningExprs) { mb.b.addPartialIndexPredicatesForTable(mb.md.TableMeta(mb.tabID), nil /* scan */) // Project partial index PUT and DEL boolean columns. - mb.projectPartialIndexPutAndDelCols(mb.outScope, mb.fetchScope) + mb.projectPartialIndexPutAndDelCols() mb.buildUniqueChecksForUpdate() From 5885e94d29ac29390e4ae69d7c9ea224e97fcc41 Mon Sep 17 00:00:00 2001 From: Tommy Reilly Date: Wed, 14 Apr 2021 13:36:19 -0400 Subject: [PATCH 5/5] scripts: allow GCEWORKER_USER env to override ssh user Release note: None --- scripts/gceworker.sh | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/scripts/gceworker.sh b/scripts/gceworker.sh index d0b36477be8a..d9739302dda9 100755 --- a/scripts/gceworker.sh +++ b/scripts/gceworker.sh @@ -7,7 +7,8 @@ source build/shlib.sh export CLOUDSDK_CORE_PROJECT=${CLOUDSDK_CORE_PROJECT-${GCEWORKER_PROJECT-cockroach-workers}} export CLOUDSDK_COMPUTE_ZONE=${GCEWORKER_ZONE-${CLOUDSDK_COMPUTE_ZONE-us-east1-b}} -NAME=${GCEWORKER_NAME-gceworker-$(id -un)} +INSTANCE=${GCEWORKER_NAME-gceworker-$(id -un)} +SSH_USER=${GCEWORKER_USER:-""} cmd=${1-} if [[ "${cmd}" ]]; then @@ -27,7 +28,7 @@ case "${cmd}" in fi gcloud compute instances \ - create "${NAME}" \ + create "${INSTANCE}" \ --machine-type "custom-24-32768" \ --network "default" \ --maintenance-policy "MIGRATE" \ @@ -35,32 +36,32 @@ case "${cmd}" in --image-family "ubuntu-1804-lts" \ --boot-disk-size "100" \ --boot-disk-type "pd-ssd" \ - --boot-disk-device-name "${NAME}" \ + --boot-disk-device-name "${INSTANCE}" \ --scopes "cloud-platform" - gcloud compute firewall-rules create "${NAME}-mosh" --allow udp:60000-61000 + gcloud compute firewall-rules create "${INSTANCE}-mosh" --allow udp:60000-61000 # Retry while vm and sshd start up. - retry gcloud compute ssh "${NAME}" --command=true + retry gcloud compute ssh "${SSH_USER}@${INSTANCE}" --command=true - gcloud compute scp --recurse "build/bootstrap" "${NAME}:bootstrap" - gcloud compute ssh "${NAME}" --ssh-flag="-A" --command="./bootstrap/bootstrap-debian.sh" + gcloud compute scp --recurse "build/bootstrap" "${INSTANCE}:bootstrap" + gcloud compute ssh "${SSH_USER}@${INSTANCE}" --ssh-flag="-A" --command="./bootstrap/bootstrap-debian.sh" if [[ "$COCKROACH_DEV_LICENSE" ]]; then - gcloud compute ssh "${NAME}" --command="echo COCKROACH_DEV_LICENSE=$COCKROACH_DEV_LICENSE >> ~/.bashrc_bootstrap" + gcloud compute ssh "${SSH_USER}@${INSTANCE}" --command="echo COCKROACH_DEV_LICENSE=$COCKROACH_DEV_LICENSE >> ~/.bashrc_bootstrap" fi # Install automatic shutdown after ten minutes of operation without a # logged in user. To disable this, `sudo touch /.active`. - gcloud compute ssh "${NAME}" --command="sudo cp bootstrap/autoshutdown.cron.sh /root/; echo '* * * * * /root/autoshutdown.cron.sh 10' | sudo crontab -i -" + gcloud compute ssh "${SSH_USER}@${INSTANCE}" --command="sudo cp bootstrap/autoshutdown.cron.sh /root/; echo '* * * * * /root/autoshutdown.cron.sh 10' | sudo crontab -i -" ;; start) - gcloud compute instances start "${NAME}" + gcloud compute instances start "${INSTANCE}" echo "waiting for node to finish starting..." # Wait for vm and sshd to start up. - retry gcloud compute ssh "${NAME}" --command=true || true + retry gcloud compute ssh "${SSH_USER}@${INSTANCE}" --command=true || true # SSH into the node, since that's probably why we started it. - gcloud compute ssh "${NAME}" --ssh-flag="-A" "$@" + gcloud compute ssh "${SSH_USER}@${INSTANCE}" --ssh-flag="-A" "$@" ;; stop) read -r -p "This will stop the VM. Are you sure? [yes] " response @@ -70,7 +71,7 @@ case "${cmd}" in echo Aborting exit 1 fi - gcloud compute instances stop "${NAME}" + gcloud compute instances stop "${INSTANCE}" ;; delete|destroy) read -r -p "This will delete the VM! Are you sure? [yes] " response @@ -81,18 +82,18 @@ case "${cmd}" in exit 1 fi status=0 - gcloud compute firewall-rules delete "${NAME}-mosh" --quiet || status=$((status+1)) - gcloud compute instances delete "${NAME}" --quiet || status=$((status+1)) + gcloud compute firewall-rules delete "${INSTANCE}-mosh" --quiet || status=$((status+1)) + gcloud compute instances delete "${INSTANCE}" --quiet || status=$((status+1)) exit ${status} ;; ssh) - gcloud compute ssh "${NAME}" --ssh-flag="-A" "$@" + gcloud compute ssh "${SSH_USER}@${INSTANCE}" --ssh-flag="-A" "$@" ;; mosh) # An alternative solution would be to run gcloud compute config-ssh after # starting or creating the vm, which adds stanzas to ~/.ssh/config that # make `ssh $HOST` (and by extension, hopefully, mosh). - read -r -a arr <<< "$(gcloud compute ssh "${NAME}" --dry-run)" + read -r -a arr <<< "$(gcloud compute ssh "${SSH_USER}@${INSTANCE}" --dry-run)" host="${arr[-1]}" unset 'arr[${#arr[@]}-1]' mosh --ssh=$(printf '%q' "${arr}") $host @@ -102,7 +103,7 @@ case "${cmd}" in retry gcloud compute scp "$@" ;; ip) - gcloud compute instances describe --format="value(networkInterfaces[0].accessConfigs[0].natIP)" "${NAME}" + gcloud compute instances describe --format="value(networkInterfaces[0].accessConfigs[0].natIP)" "${INSTANCE}" ;; sync) if ! hash unison 2>/dev/null; then @@ -130,7 +131,7 @@ case "${cmd}" in tmpfile=$(mktemp) trap 'rm -f ${tmpfile}' EXIT gcloud compute config-ssh --ssh-config-file "$tmpfile" > /dev/null - unison "$host" "ssh://${NAME}.${CLOUDSDK_COMPUTE_ZONE}.${CLOUDSDK_CORE_PROJECT}/$worker" \ + unison "$host" "ssh://${SSH_USER}@${INSTANCE}.${CLOUDSDK_COMPUTE_ZONE}.${CLOUDSDK_CORE_PROJECT}/$worker" \ -sshargs "-F ${tmpfile}" -auto -prefer "$host" -repeat watch \ -ignore 'Path .git' \ -ignore 'Path bin*' \