Skip to content

Commit

Permalink
Merge #58888 #58919 #58982
Browse files Browse the repository at this point in the history 
58888: sql, jobs: stop queuing schema change jobs for in-txn schema changes r=postamar a=postamar

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.

58919: kvnemesis: fix and unskip TestKVNemesisMultiNode r=aayushshah15 a=aayushshah15

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

58982: delegate: remove is_active_region from SHOW REGIONS FROM DATABASE r=ajstorm a=otan

Due to unpopular demand.

Release note: None

Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Aayush Shah <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
  • Loading branch information
@aayushshah15 @otan
4 people committed Jan 14, 2021
4 parents 690c122 + 2360c4b + cdc0b55 + 0a980a8 commit 481cb43
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 35 deletions.
1 change: 0 additions & 1 deletion pkg/kv/kvnemesis/kvnemesis_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion pkg/kv/kvnemesis/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`) {
Expand Down
1 change: 0 additions & 1 deletion pkg/sql/delegate/show_regions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/alter_table
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
60 changes: 30 additions & 30 deletions pkg/sql/logictest/testdata/logic_test/multiregion
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -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}
6 changes: 4 additions & 2 deletions pkg/sql/table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down

0 comments on commit 481cb43

Please sign in to comment.