From cdc0b555326b29365b27244bb585be0963428605 Mon Sep 17 00:00:00 2001 From: Aayush Shah Date: Wed, 13 Jan 2021 02:44:46 -0500 Subject: [PATCH 1/3] kvnemesis: fix and unskip TestKVNemesisMultiNode Before this patch, #56197 broke this test because it changed a range merge error string that let the KV nemesis validator ignore the error. This patch updates the regexp the validator uses to match the error and unskips the test. Fixes #58624. Release note: None --- pkg/kv/kvnemesis/kvnemesis_test.go | 1 - pkg/kv/kvnemesis/validator.go | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kv/kvnemesis/kvnemesis_test.go b/pkg/kv/kvnemesis/kvnemesis_test.go index 3188cdfa9e3a..5c8fd5c9dcc0 100644 --- a/pkg/kv/kvnemesis/kvnemesis_test.go +++ b/pkg/kv/kvnemesis/kvnemesis_test.go @@ -55,7 +55,6 @@ func TestKVNemesisSingleNode(t *testing.T) { func TestKVNemesisMultiNode(t *testing.T) { defer leaktest.AfterTest(t)() - skip.WithIssue(t, 58624, "flaky test") skip.UnderRace(t) defer log.Scope(t).Close(t) diff --git a/pkg/kv/kvnemesis/validator.go b/pkg/kv/kvnemesis/validator.go index 0407c7b9971b..0c824e75dc5a 100644 --- a/pkg/kv/kvnemesis/validator.go +++ b/pkg/kv/kvnemesis/validator.go @@ -310,7 +310,8 @@ func (v *validator) processOp(txnID *string, op Operation) { // However, I think the right thing to do is sniff this inside the // AdminMerge code and retry so the client never sees it. In the meantime, // no-op. #44377 - } else if resultIsError(t.Result, `merge failed: cannot merge range with non-voter replicas`) { + } else if resultIsError(t.Result, + `merge failed: cannot merge ranges when (rhs)|(lhs) is in a joint state or has learners`) { // This operation executed concurrently with one that was changing // replicas. } else if resultIsError(t.Result, `merge failed: ranges not collocated`) { From 0a980a85fb7dd8b94822b49744e0a0ccd74b6301 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Thu, 14 Jan 2021 11:09:36 +1100 Subject: [PATCH 2/3] delegate: remove is_active_region from SHOW REGIONS FROM DATABASE Due to unpopular demand. Release note: None --- pkg/sql/delegate/show_regions.go | 1 - .../logictest/testdata/logic_test/multiregion | 60 +++++++++---------- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/pkg/sql/delegate/show_regions.go b/pkg/sql/delegate/show_regions.go index 9f8db7a52733..708f954e22de 100644 --- a/pkg/sql/delegate/show_regions.go +++ b/pkg/sql/delegate/show_regions.go @@ -74,7 +74,6 @@ SELECT r.name AS "database", r.region as "region", r.region = r.primary_region AS "primary", - zones_table.region IS NOT NULL AS is_region_active, COALESCE(zones_table.zones, '{}'::string[]) AS zones diff --git a/pkg/sql/logictest/testdata/logic_test/multiregion b/pkg/sql/logictest/testdata/logic_test/multiregion index 7aac9be846e9..d9e3000faeb3 100644 --- a/pkg/sql/logictest/testdata/logic_test/multiregion +++ b/pkg/sql/logictest/testdata/logic_test/multiregion @@ -120,13 +120,13 @@ test {} NULL statement ok USE multi_region_test_db -query TTBBT colnames +query TTBT colnames SHOW REGIONS FROM DATABASE ---- -database region primary is_region_active zones -multi_region_test_db ap-southeast-2 false true {ap-az1,ap-az2,ap-az3} -multi_region_test_db ca-central-1 true true {ca-az1,ca-az2,ca-az3} -multi_region_test_db us-east-1 false true {us-az1,us-az2,us-az3} +database region primary zones +multi_region_test_db ap-southeast-2 false {ap-az1,ap-az2,ap-az3} +multi_region_test_db ca-central-1 true {ca-az1,ca-az2,ca-az3} +multi_region_test_db us-east-1 false {us-az1,us-az2,us-az3} query TTTT colnames SHOW REGIONS @@ -141,11 +141,11 @@ SHOW SURVIVAL GOAL FROM DATABASE ---- multi_region_test_db region -query TTBBT colnames +query TTBT colnames SHOW REGIONS FROM DATABASE region_test_db ---- -database region primary is_region_active zones -region_test_db ap-southeast-2 true true {ap-az1,ap-az2,ap-az3} +database region primary zones +region_test_db ap-southeast-2 true {ap-az1,ap-az2,ap-az3} query TT SHOW SURVIVAL GOAL FROM DATABASE region_test_db @@ -371,12 +371,12 @@ public crdb_internal_region {ca-central-1} root statement ok ALTER DATABASE alter_test_db ADD REGION "ap-southeast-2" -query TTBBT colnames +query TTBT colnames show regions from database alter_test_db ---- -database region primary is_region_active zones -alter_test_db ap-southeast-2 false true {ap-az1,ap-az2,ap-az3} -alter_test_db ca-central-1 true true {ca-az1,ca-az2,ca-az3} +database region primary zones +alter_test_db ap-southeast-2 false {ap-az1,ap-az2,ap-az3} +alter_test_db ca-central-1 true {ca-az1,ca-az2,ca-az3} query TTTT colnames SHOW ENUMS FROM alter_test_db.public @@ -399,13 +399,13 @@ DATABASE alter_test_db ALTER DATABASE alter_test_db CONFIGURE ZONE USING statement ok ALTER DATABASE alter_test_db ADD REGION "us-east-1" -query TTBBT colnames +query TTBT colnames show regions from database alter_test_db ---- -database region primary is_region_active zones -alter_test_db ap-southeast-2 false true {ap-az1,ap-az2,ap-az3} -alter_test_db ca-central-1 true true {ca-az1,ca-az2,ca-az3} -alter_test_db us-east-1 false true {us-az1,us-az2,us-az3} +database region primary zones +alter_test_db ap-southeast-2 false {ap-az1,ap-az2,ap-az3} +alter_test_db ca-central-1 true {ca-az1,ca-az2,ca-az3} +alter_test_db us-east-1 false {us-az1,us-az2,us-az3} query TTTT colnames SHOW ENUMS FROM alter_test_db.public @@ -454,10 +454,10 @@ RANGE default ALTER RANGE default CONFIGURE ZONE USING constraints = '[]', lease_preferences = '[]' -query TTBBT colnames +query TTBT colnames show regions from database primary_region_db ---- -database region primary is_region_active zones +database region primary zones query TTTT colnames SHOW ENUMS FROM primary_region_db.public @@ -481,11 +481,11 @@ DATABASE primary_region_db ALTER DATABASE primary_region_db CONFIGURE ZONE USIN constraints = '{+region=ca-central-1: 1}', lease_preferences = '[[+region=ca-central-1]]' -query TTBBT colnames +query TTBT colnames show regions from database primary_region_db ---- -database region primary is_region_active zones -primary_region_db ca-central-1 true true {ca-az1,ca-az2,ca-az3} +database region primary zones +primary_region_db ca-central-1 true {ca-az1,ca-az2,ca-az3} query TTTT colnames SHOW ENUMS FROM primary_region_db.public @@ -516,12 +516,12 @@ SHOW ENUMS FROM primary_region_db.public schema name values owner public crdb_internal_region {ap-southeast-2,ca-central-1} root -query TTBBT colnames +query TTBT colnames show regions from database primary_region_db ---- -database region primary is_region_active zones -primary_region_db ap-southeast-2 false true {ap-az1,ap-az2,ap-az3} -primary_region_db ca-central-1 true true {ca-az1,ca-az2,ca-az3} +database region primary zones +primary_region_db ap-southeast-2 false {ap-az1,ap-az2,ap-az3} +primary_region_db ca-central-1 true {ca-az1,ca-az2,ca-az3} statement ok ALTER DATABASE primary_region_db PRIMARY REGION "ap-southeast-2" @@ -543,9 +543,9 @@ SHOW ENUMS FROM primary_region_db.public schema name values owner public crdb_internal_region {ap-southeast-2,ca-central-1} root -query TTBBT colnames +query TTBT colnames show regions from database primary_region_db ---- -database region primary is_region_active zones -primary_region_db ap-southeast-2 true true {ap-az1,ap-az2,ap-az3} -primary_region_db ca-central-1 false true {ca-az1,ca-az2,ca-az3} +database region primary zones +primary_region_db ap-southeast-2 true {ap-az1,ap-az2,ap-az3} +primary_region_db ca-central-1 false {ca-az1,ca-az2,ca-az3} From 2360c4bb7ab57676ebc81d560cd93d1b3c24a0da Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 12 Jan 2021 16:17:51 -0500 Subject: [PATCH 3/3] sql, jobs: stop queuing schema change jobs for in-txn schema changes Creating a table and changing its schema within a transaction would cause a schema change job to be queued. Such jobs are not necessary and don't do anything. This patch prevents them from being queued in the first place. Fixes #45985. Release note (sql change): Creating a table and changing its schema within a transaction no longer schedules a schema change job. --- .../logictest/testdata/logic_test/alter_table | 20 +++++++++++++++++++ pkg/sql/table.go | 6 ++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 9fff041d9739..3d7d59228a54 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -1535,3 +1535,23 @@ CREATE TABLE t54629 (c INT NOT NULL, UNIQUE INDEX (c)); ALTER TABLE t54629 ADD CONSTRAINT pk PRIMARY KEY (c); INSERT INTO t54629 VALUES (1); DELETE FROM t54629 WHERE c = 1; + +subtest regression_45985 + +statement ok +BEGIN; +CREATE TABLE t45985 (a INT); +ALTER TABLE t45985 ADD COLUMN b INT; +COMMIT; + +query I +SELECT count(descriptor_id) + FROM ( + SELECT json_array_elements_text( + crdb_internal.pb_to_json('cockroach.sql.jobs.jobspb.Payload', payload)->'descriptorIds' + )::INT8 AS descriptor_id + FROM system.jobs + ) + WHERE descriptor_id = ('test.public.t45985'::REGCLASS)::INT8; +---- +0 diff --git a/pkg/sql/table.go b/pkg/sql/table.go index 4e5d95191cfc..06e82fbd240a 100644 --- a/pkg/sql/table.go +++ b/pkg/sql/table.go @@ -223,8 +223,10 @@ func (p *planner) writeSchemaChange( return errors.Errorf("no schema changes allowed on table %q as it is being dropped", tableDesc.Name) } - if err := p.createOrUpdateSchemaChangeJob(ctx, tableDesc, jobDesc, mutationID); err != nil { - return err + if !tableDesc.IsNew() { + if err := p.createOrUpdateSchemaChangeJob(ctx, tableDesc, jobDesc, mutationID); err != nil { + return err + } } return p.writeTableDesc(ctx, tableDesc) }