From 5cd729c3820eed77d5eb32078bcd6f33a6843f77 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Mon, 8 Feb 2021 17:43:33 -0800 Subject: [PATCH 1/3] ccl: add inverted indexes to regional by row tests Release note: None --- .../testdata/logic_test/regional_by_row | 97 ++++++++++++++----- 1 file changed, 71 insertions(+), 26 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index e01cdb69d116..e4e337fbc1ad 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -53,6 +53,15 @@ CREATE TABLE regional_by_row_table ( ) LOCALITY REGIONAL BY ROW +statement error REGIONAL BY ROW on a table with an INDEX containing PARTITION BY is not supported +CREATE TABLE regional_by_row_table ( + pk int, + a int, + j JSON, + INVERTED INDEX (a, j) PARTITION BY LIST (a) (PARTITION one VALUES IN ((1))) +) +LOCALITY REGIONAL BY ROW + statement error REGIONAL BY ROW on a table with an UNIQUE constraint containing PARTITION BY is not supported CREATE TABLE regional_by_row_table ( pk int, @@ -76,8 +85,10 @@ CREATE TABLE regional_by_row_table ( pk2 int NOT NULL, a int NOT NULL, b int NOT NULL, + j JSON, INDEX (a), UNIQUE (b), + INVERTED INDEX (j), FAMILY (pk, pk2, a, b) ) LOCALITY REGIONAL BY ROW @@ -90,11 +101,13 @@ CREATE TABLE public.regional_by_row_table ( pk2 INT8 NOT NULL, a INT8 NOT NULL, b INT8 NOT NULL, + j JSONB NULL, crdb_region public.crdb_internal_region NOT VISIBLE NOT NULL DEFAULT gateway_region()::public.crdb_internal_region, CONSTRAINT "primary" PRIMARY KEY (pk ASC), INDEX regional_by_row_table_a_idx (a ASC), UNIQUE INDEX regional_by_row_table_b_key (b ASC), - FAMILY fam_0_pk_pk2_a_b_crdb_region (pk, pk2, a, b, crdb_region) + INVERTED INDEX regional_by_row_table_j_idx (j), + FAMILY fam_0_pk_pk2_a_b_j_crdb_region (pk, pk2, a, b, j, crdb_region) ) LOCALITY REGIONAL BY ROW; ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_by_row_table@primary CONFIGURE ZONE USING num_voters = 5, @@ -108,6 +121,10 @@ ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_b num_voters = 5, voter_constraints = '{+region=ap-southeast-2: 2}', lease_preferences = '[[+region=ap-southeast-2]]'; +ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_j_idx CONFIGURE ZONE USING + num_voters = 5, + voter_constraints = '{+region=ap-southeast-2: 2}', + lease_preferences = '[[+region=ap-southeast-2]]'; ALTER PARTITION "ca-central-1" OF INDEX multi_region_test_db.public.regional_by_row_table@primary CONFIGURE ZONE USING num_voters = 5, voter_constraints = '{+region=ca-central-1: 2}', @@ -120,6 +137,10 @@ ALTER PARTITION "ca-central-1" OF INDEX multi_region_test_db.public.regional_by_ num_voters = 5, voter_constraints = '{+region=ca-central-1: 2}', lease_preferences = '[[+region=ca-central-1]]'; +ALTER PARTITION "ca-central-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_j_idx CONFIGURE ZONE USING + num_voters = 5, + voter_constraints = '{+region=ca-central-1: 2}', + lease_preferences = '[[+region=ca-central-1]]'; ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@primary CONFIGURE ZONE USING num_voters = 5, voter_constraints = '{+region=us-east-1: 2}', @@ -129,6 +150,10 @@ ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row voter_constraints = '{+region=us-east-1: 2}', lease_preferences = '[[+region=us-east-1]]'; ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_b_key CONFIGURE ZONE USING + num_voters = 5, + voter_constraints = '{+region=us-east-1: 2}', + lease_preferences = '[[+region=us-east-1]]'; +ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_j_idx CONFIGURE ZONE USING num_voters = 5, voter_constraints = '{+region=us-east-1: 2}', lease_preferences = '[[+region=us-east-1]]' @@ -145,6 +170,8 @@ regional_by_row_table_a_idx a false regional_by_row_table_a_idx crdb_region true regional_by_row_table_b_key b false regional_by_row_table_b_key crdb_region true +regional_by_row_table_j_idx crdb_region true +regional_by_row_table_j_idx j false query TTTTIT colnames SHOW TABLES @@ -153,8 +180,8 @@ schema_name table_name type owner estimated_row_count locality public regional_by_row_table table root 0 REGIONAL BY ROW query TI -INSERT INTO regional_by_row_table (pk, pk2, a, b) VALUES -(1, 1, 2, 3), (4, 4, 5, 6) +INSERT INTO regional_by_row_table (pk, pk2, a, b, j) VALUES +(1, 1, 2, 3, '{"a": "b"}'), (4, 4, 5, 6, '{"c": "d"}') RETURNING crdb_region, pk ---- ap-southeast-2 1 @@ -199,16 +226,16 @@ ca-central-1 10 10 11 12 us-east-1 20 20 21 22 ap-southeast-2 23 23 24 25 -query IIII colnames +query IIIIT colnames SELECT * FROM regional_by_row_table ORDER BY pk ---- -pk pk2 a b -1 1 2 3 -4 4 5 6 -7 7 8 9 -10 10 11 12 -20 20 21 22 -23 23 24 25 +pk pk2 a b j +1 1 2 3 {"a": "b"} +4 4 5 6 {"c": "d"} +7 7 8 9 NULL +10 10 11 12 NULL +20 20 21 22 NULL +23 23 24 25 NULL # Tests creating a index and a unique constraint on a REGIONAL BY ROW table. statement ok @@ -228,13 +255,13 @@ vectorized: true • root │ ├── • insert -│ │ into: regional_by_row_table(pk, pk2, a, b, crdb_region) +│ │ into: regional_by_row_table(pk, pk2, a, b, j, crdb_region) │ │ │ └── • buffer │ │ label: buffer 1 │ │ │ └── • values -│ size: 6 columns, 1 row +│ size: 7 columns, 1 row │ ├── • constraint-check │ │ @@ -242,9 +269,9 @@ vectorized: true │ │ │ └── • lookup join (semi) │ │ table: regional_by_row_table@primary -│ │ equality: (lookup_join_const_col_@20, column1) = (crdb_region,pk) +│ │ equality: (lookup_join_const_col_@24, column1) = (crdb_region,pk) │ │ equality cols are key -│ │ pred: column12 != crdb_region +│ │ pred: column15 != crdb_region │ │ │ └── • cross join │ │ @@ -260,9 +287,9 @@ vectorized: true │ │ │ └── • lookup join (semi) │ │ table: regional_by_row_table@regional_by_row_table_b_key -│ │ equality: (lookup_join_const_col_@30, column4) = (crdb_region,b) +│ │ equality: (lookup_join_const_col_@36, column4) = (crdb_region,b) │ │ equality cols are key -│ │ pred: (column1 != pk) OR (column12 != crdb_region) +│ │ pred: (column1 != pk) OR (column15 != crdb_region) │ │ │ └── • cross join │ │ @@ -278,9 +305,9 @@ vectorized: true │ └── • lookup join (semi) │ table: regional_by_row_table@new_idx - │ equality: (lookup_join_const_col_@41, column3, column4) = (crdb_region,a,b) + │ equality: (lookup_join_const_col_@49, column3, column4) = (crdb_region,a,b) │ equality cols are key - │ pred: (column1 != pk) OR (column12 != crdb_region) + │ pred: (column1 != pk) OR (column15 != crdb_region) │ └── • cross join │ @@ -304,7 +331,7 @@ vectorized: true • root │ ├── • upsert -│ │ into: regional_by_row_table(pk, pk2, a, b, crdb_region) +│ │ into: regional_by_row_table(pk, pk2, a, b, j, crdb_region) │ │ arbiter constraints: primary │ │ │ └── • buffer @@ -320,7 +347,7 @@ vectorized: true │ │ spans: [/'ap-southeast-2'/2 - /'ap-southeast-2'/2] [/'ca-central-1'/2 - /'ca-central-1'/2] [/'us-east-1'/2 - /'us-east-1'/2] │ │ │ └── • values -│ size: 5 columns, 1 row +│ size: 6 columns, 1 row │ ├── • constraint-check │ │ @@ -328,7 +355,7 @@ vectorized: true │ │ │ └── • lookup join (semi) │ │ table: regional_by_row_table@primary -│ │ equality: (lookup_join_const_col_@28, upsert_pk) = (crdb_region,pk) +│ │ equality: (lookup_join_const_col_@35, upsert_pk) = (crdb_region,pk) │ │ equality cols are key │ │ pred: column1 != crdb_region │ │ @@ -346,7 +373,7 @@ vectorized: true │ │ │ └── • lookup join (semi) │ │ table: regional_by_row_table@regional_by_row_table_b_key -│ │ equality: (lookup_join_const_col_@38, column5) = (crdb_region,b) +│ │ equality: (lookup_join_const_col_@47, column5) = (crdb_region,b) │ │ equality cols are key │ │ pred: (upsert_pk != pk) OR (column1 != crdb_region) │ │ @@ -364,7 +391,7 @@ vectorized: true │ └── • lookup join (semi) │ table: regional_by_row_table@new_idx - │ equality: (lookup_join_const_col_@49, column4, column5) = (crdb_region,a,b) + │ equality: (lookup_join_const_col_@60, column4, column5) = (crdb_region,a,b) │ equality cols are key │ pred: (upsert_pk != pk) OR (column1 != crdb_region) │ @@ -401,13 +428,15 @@ CREATE TABLE public.regional_by_row_table ( pk2 INT8 NOT NULL, a INT8 NOT NULL, b INT8 NOT NULL, + j JSONB NULL, crdb_region public.crdb_internal_region NOT VISIBLE NOT NULL DEFAULT gateway_region()::public.crdb_internal_region, CONSTRAINT "primary" PRIMARY KEY (pk ASC), INDEX regional_by_row_table_a_idx (a ASC), UNIQUE INDEX regional_by_row_table_b_key (b ASC), + INVERTED INDEX regional_by_row_table_j_idx (j), INDEX new_idx (a ASC, b ASC), UNIQUE INDEX unique_b_a (b ASC, a ASC), - FAMILY fam_0_pk_pk2_a_b_crdb_region (pk, pk2, a, b, crdb_region) + FAMILY fam_0_pk_pk2_a_b_j_crdb_region (pk, pk2, a, b, j, crdb_region) ) LOCALITY REGIONAL BY ROW; ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_by_row_table@new_idx CONFIGURE ZONE USING num_voters = 5, @@ -425,6 +454,10 @@ ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_b num_voters = 5, voter_constraints = '{+region=ap-southeast-2: 2}', lease_preferences = '[[+region=ap-southeast-2]]'; +ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_j_idx CONFIGURE ZONE USING + num_voters = 5, + voter_constraints = '{+region=ap-southeast-2: 2}', + lease_preferences = '[[+region=ap-southeast-2]]'; ALTER PARTITION "ap-southeast-2" OF INDEX multi_region_test_db.public.regional_by_row_table@unique_b_a CONFIGURE ZONE USING num_voters = 5, voter_constraints = '{+region=ap-southeast-2: 2}', @@ -445,6 +478,10 @@ ALTER PARTITION "ca-central-1" OF INDEX multi_region_test_db.public.regional_by_ num_voters = 5, voter_constraints = '{+region=ca-central-1: 2}', lease_preferences = '[[+region=ca-central-1]]'; +ALTER PARTITION "ca-central-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_j_idx CONFIGURE ZONE USING + num_voters = 5, + voter_constraints = '{+region=ca-central-1: 2}', + lease_preferences = '[[+region=ca-central-1]]'; ALTER PARTITION "ca-central-1" OF INDEX multi_region_test_db.public.regional_by_row_table@unique_b_a CONFIGURE ZONE USING num_voters = 5, voter_constraints = '{+region=ca-central-1: 2}', @@ -465,6 +502,10 @@ ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row num_voters = 5, voter_constraints = '{+region=us-east-1: 2}', lease_preferences = '[[+region=us-east-1]]'; +ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@regional_by_row_table_j_idx CONFIGURE ZONE USING + num_voters = 5, + voter_constraints = '{+region=us-east-1: 2}', + lease_preferences = '[[+region=us-east-1]]'; ALTER PARTITION "us-east-1" OF INDEX multi_region_test_db.public.regional_by_row_table@unique_b_a CONFIGURE ZONE USING num_voters = 5, voter_constraints = '{+region=us-east-1: 2}', @@ -485,6 +526,8 @@ regional_by_row_table_a_idx a false regional_by_row_table_a_idx crdb_region true regional_by_row_table_b_key b false regional_by_row_table_b_key crdb_region true +regional_by_row_table_j_idx crdb_region true +regional_by_row_table_j_idx j false unique_b_a a false unique_b_a b false unique_b_a crdb_region true @@ -516,14 +559,16 @@ CREATE TABLE public.regional_by_row_table ( pk2 INT8 NOT NULL, a INT8 NOT NULL, b INT8 NOT NULL, + j JSONB NULL, crdb_region public.crdb_internal_region NOT VISIBLE NOT NULL DEFAULT gateway_region()::public.crdb_internal_region, CONSTRAINT "primary" PRIMARY KEY (pk2 ASC), UNIQUE INDEX regional_by_row_table_pk_key (pk ASC), INDEX regional_by_row_table_a_idx (a ASC), UNIQUE INDEX regional_by_row_table_b_key (b ASC), + INVERTED INDEX regional_by_row_table_j_idx (j), INDEX new_idx (a ASC, b ASC), UNIQUE INDEX unique_b_a (b ASC, a ASC), - FAMILY fam_0_pk_pk2_a_b_crdb_region (pk, pk2, a, b, crdb_region) + FAMILY fam_0_pk_pk2_a_b_j_crdb_region (pk, pk2, a, b, j, crdb_region) ) LOCALITY REGIONAL BY ROW query TT From 9352005ca6e954c3e46e162bd3a6f7b7ac5ccbb8 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Thu, 4 Feb 2021 21:41:16 -0800 Subject: [PATCH 2/3] colrpc: add a comment about runnerCtx check in sendBatches Release note: None --- pkg/sql/colflow/colrpc/outbox.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/sql/colflow/colrpc/outbox.go b/pkg/sql/colflow/colrpc/outbox.go index 546c785e7be6..0b3adfb22aac 100644 --- a/pkg/sql/colflow/colrpc/outbox.go +++ b/pkg/sql/colflow/colrpc/outbox.go @@ -224,6 +224,9 @@ func (o *Outbox) sendBatches( ctx context.Context, stream flowStreamClient, cancelFn context.CancelFunc, ) (terminatedGracefully bool, errToSend error) { if o.runnerCtx == nil { + // In the non-testing path, runnerCtx has been set in Run() method; + // however, the tests might use runWithStream() directly in which case + // runnerCtx will remain unset, so we have this check. o.runnerCtx = ctx } errToSend = colexecerror.CatchVectorizedRuntimeError(func() { From 53b7d3fd72ff3fb2fff60ee7f382e1a18b271d66 Mon Sep 17 00:00:00 2001 From: Eric Wang Date: Tue, 9 Feb 2021 11:35:55 -0500 Subject: [PATCH 3/3] sql: explicitly verify descriptors are sequences Fixes https://github.com/cockroachdb/cockroach/issues/59991. Previously, we relied on ObjectLookupFlags to verify descriptor types. This was inadequate because ID based resolution ignores the flag and doesn't actually make the check. This patch adds an explicit check to verify the returned descriptor is a sequence. Release note (bug fix): explicitly verify descriptors are sequences --- .../logictest/testdata/logic_test/sequences | 89 +++++++++++++++++++ pkg/sql/sequence.go | 14 +++ 2 files changed, 103 insertions(+) diff --git a/pkg/sql/logictest/testdata/logic_test/sequences b/pkg/sql/logictest/testdata/logic_test/sequences index 2020764086e6..b2ecd7c3f562 100644 --- a/pkg/sql/logictest/testdata/logic_test/sequences +++ b/pkg/sql/logictest/testdata/logic_test/sequences @@ -1394,3 +1394,92 @@ CREATE SEQUENCE db2.seq2 OWNED BY db1.t.a statement error invalid OWNED BY option ALTER SEQUENCE db2.seq2 OWNED BY doesntexist + + +# Test that passing a descriptor other than a sequence +# returns an appropriate error. +subtest invalid_ids + +statement ok +CREATE TABLE t (i SERIAL PRIMARY KEY) + +let $t_id +SELECT 't'::regclass::int + +statement error pgcode 42809 "test.public.t" is not a sequence +SELECT nextval($t_id::regclass) + +statement error pgcode 42809 "test.public.t" is not a sequence +SELECT setval($t_id::regclass, 30, false) + +statement error pgcode 42809 "test.public.t" is not a sequence +SELECT currval($t_id::regclass) + +statement ok +CREATE VIEW v AS (SELECT i FROM t) + +let $v_id +SELECT 'v'::regclass::int + +statement error pgcode 42809 "test.public.v" is not a sequence +SELECT nextval($v_id::regclass) + +statement error pgcode 42809 "test.public.v" is not a sequence +SELECT setval($v_id::regclass, 30, false) + +statement error pgcode 42809 "test.public.v" is not a sequence +SELECT currval($v_id::regclass) + +statement ok +CREATE SCHEMA sc + +let $sc_id +SELECT id FROM system.namespace WHERE name='sc' + +statement error does not exist +SELECT nextval($sc_id::regclass) + +statement error does not exist +SELECT setval($sc_id::regclass, 30, false) + +statement error does not exist +SELECT currval($sc_id::regclass) + +statement ok +CREATE DATABASE db + +let $db_id +SELECT id FROM system.namespace WHERE name='db' + +statement error does not exist +SELECT nextval($db_id::regclass) + +statement error does not exist +SELECT setval($db_id::regclass, 30, false) + +statement error does not exist +SELECT currval($db_id::regclass) + +statement ok +CREATE TYPE e AS ENUM ('foo', 'bar') + +let $e_id +SELECT id FROM system.namespace WHERE name='e' + +statement error does not exist +SELECT nextval($e_id::regclass) + +statement error does not exist +SELECT setval($e_id::regclass, 30, false) + +statement error does not exist +SELECT currval($e_id::regclass) + +statement error does not exist +SELECT nextval(12345::regclass) # Bogus ID + +statement error does not exist +SELECT setval(12345::regclass, 30, false) # Bogus ID + +statement error does not exist +SELECT currval(12345::regclass) # Bogus ID diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index bbb3f1bfa1b0..f562bfff6234 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -29,6 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/sequence" @@ -95,6 +96,13 @@ func (p *planner) IncrementSequenceByID(ctx context.Context, seqID int64) (int64 if err != nil { return 0, err } + if !descriptor.IsSequence() { + seqName, err := p.getQualifiedTableName(ctx, descriptor) + if err != nil { + return 0, err + } + return 0, sqlerrors.NewWrongObjectTypeError(seqName, "sequence") + } return incrementSequenceHelper(ctx, p, descriptor) } @@ -178,6 +186,9 @@ func (p *planner) GetLatestValueInSessionForSequenceByID( if err != nil { return 0, err } + if !descriptor.IsSequence() { + return 0, sqlerrors.NewWrongObjectTypeError(seqName, "sequence") + } return getLatestValueInSessionForSequenceHelper(p, descriptor, seqName) } @@ -230,6 +241,9 @@ func (p *planner) SetSequenceValueByID( if err != nil { return err } + if !descriptor.IsSequence() { + return sqlerrors.NewWrongObjectTypeError(seqName, "sequence") + } return setSequenceValueHelper(ctx, p, descriptor, newVal, isCalled, seqName) }