From 6760c29cbbd2b9f6e1634d3b281f8fb4bc7fb665 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 31 Mar 2023 11:44:59 -0400 Subject: [PATCH 1/3] opt: prohibit hash-sharded index syntactic sugar in test catalog The test catalog now panics when trying to build a hash-sharded index instead of silently ignoring the `USING HASH` clause. This prevents writing tests that incorrectly assume that `USING HASH` works as expected in the test catalog. One test with `USING HASH` has been updated. Fixes #99129 Release note: None --- pkg/sql/opt/testutils/testcat/create_table.go | 5 + pkg/sql/opt/xform/testdata/rules/join | 131 +++++++++++------- 2 files changed, 85 insertions(+), 51 deletions(-) diff --git a/pkg/sql/opt/testutils/testcat/create_table.go b/pkg/sql/opt/testutils/testcat/create_table.go index dfc4824961c3..1156b299ac27 100644 --- a/pkg/sql/opt/testutils/testcat/create_table.go +++ b/pkg/sql/opt/testutils/testcat/create_table.go @@ -852,6 +852,11 @@ func (tt *Table) addIndexWithVersion( tt.addUniqueConstraint(def.Name, def.Columns, def.Predicate, false /* withoutIndex */) } + // The test catalog does not support the hash-sharded index syntactic sugar. + if def.Sharded != nil { + panic("hash-sharded indexes are not supported by the test catalog") + } + idx := &Index{ IdxName: tt.makeIndexName(def.Name, def.Columns, typ), Unique: typ != nonUniqueIndex, diff --git a/pkg/sql/opt/xform/testdata/rules/join b/pkg/sql/opt/xform/testdata/rules/join index 38ff5169ce6d..6596d646ec94 100644 --- a/pkg/sql/opt/xform/testdata/rules/join +++ b/pkg/sql/opt/xform/testdata/rules/join @@ -12876,7 +12876,16 @@ CREATE TABLE t85353 (a INT, b INT) ---- exec-ddl -CREATE TABLE u85353 (a INT, b INT, INDEX (a,b) USING HASH, INDEX (b) USING HASH) +CREATE TABLE u85353 ( + a INT, + b INT, + a_b_hash INT NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(a, b)), 8)) VIRTUAL, + b_hash INT NOT VISIBLE NOT NULL AS (mod(fnv32(crdb_internal.datums_to_bytes(b)), 8)) VIRTUAL, + INDEX a_b_idx (a_b_hash, a, b), + INDEX b_idx (b_hash, b), + CHECK (a_b_hash IN (0, 1, 2, 3, 4, 5, 6, 7)), + CHECK (b_hash IN (0, 1, 2, 3, 4, 5, 6, 7)) +) ---- exec-ddl @@ -12946,71 +12955,91 @@ ALTER TABLE u85353 INJECT STATISTICS # should not reduce join selectivity and cause the following to # choose lookup join. opt -EXPLAIN (OPT) SELECT * FROM t85353 INNER JOIN u85353@u85353_b_idx USING (b) WHERE u85353.a < 10; +EXPLAIN (OPT) SELECT * FROM t85353 INNER JOIN u85353@b_idx USING (b) WHERE u85353.a < 10; ---- explain - ├── columns: info:11 + ├── columns: info:13 ├── mode: opt + ├── immutable └── project - ├── columns: b:2!null a:1 a:6!null - └── inner-join (merge) - ├── columns: t85353.a:1 t85353.b:2!null u85353.a:6!null u85353.b:7!null - ├── left ordering: +7 - ├── right ordering: +2 - ├── fd: (2)==(7), (7)==(2) - ├── select - │ ├── columns: u85353.a:6!null u85353.b:7 - │ ├── ordering: +7 - │ ├── index-join u85353 - │ │ ├── columns: u85353.a:6 u85353.b:7 - │ │ ├── ordering: +7 - │ │ └── scan u85353@u85353_b_idx - │ │ ├── columns: u85353.b:7 u85353.rowid:8!null - │ │ ├── flags: force-index=u85353_b_idx - │ │ ├── key: (8) - │ │ ├── fd: (8)-->(7) - │ │ └── ordering: +7 - │ └── filters - │ └── u85353.a:6 < 10 [outer=(6), constraints=(/6: (/NULL - /9]; tight)] - ├── sort - │ ├── columns: t85353.a:1 t85353.b:2 - │ ├── ordering: +2 - │ └── scan t85353 - │ └── columns: t85353.a:1 t85353.b:2 - └── filters (true) + ├── columns: b:2!null a:1 a:6!null a_b_hash:8 b_hash:9 + ├── immutable + ├── fd: (2,6)-->(8), (2)-->(9) + └── inner-join (hash) + ├── columns: t85353.a:1 t85353.b:2!null u85353.a:6!null u85353.b:7!null a_b_hash:8 b_hash:9 + ├── immutable + ├── fd: (6,7)-->(8), (7)-->(9), (2)==(7), (7)==(2) + ├── project + │ ├── columns: a_b_hash:8 b_hash:9 u85353.a:6!null u85353.b:7 + │ ├── immutable + │ ├── fd: (6,7)-->(8), (7)-->(9) + │ ├── select + │ │ ├── columns: u85353.a:6!null u85353.b:7 + │ │ ├── index-join u85353 + │ │ │ ├── columns: u85353.a:6 u85353.b:7 + │ │ │ └── scan u85353@b_idx + │ │ │ ├── columns: u85353.b:7 u85353.rowid:10!null + │ │ │ ├── constraint: /9/7/10: [/0 - /7] + │ │ │ ├── flags: force-index=b_idx + │ │ │ ├── key: (10) + │ │ │ └── fd: (10)-->(7) + │ │ └── filters + │ │ └── u85353.a:6 < 10 [outer=(6), constraints=(/6: (/NULL - /9]; tight)] + │ └── projections + │ ├── mod(fnv32(crdb_internal.datums_to_bytes(u85353.a:6, u85353.b:7)), 8) [as=a_b_hash:8, outer=(6,7), immutable] + │ └── mod(fnv32(crdb_internal.datums_to_bytes(u85353.b:7)), 8) [as=b_hash:9, outer=(7), immutable] + ├── scan t85353 + │ └── columns: t85353.a:1 t85353.b:2 + └── filters + └── t85353.b:2 = u85353.b:7 [outer=(2,7), constraints=(/2: (/NULL - ]; /7: (/NULL - ]), fd=(2)==(7), (7)==(2)] # The derived equijoin condition between the hash bucket column in # u85353@u85353_a_b_idx and a similar hash bucket function expression on t85353.b # should not reduce join selectivity and cause the following to # choose lookup join. opt -EXPLAIN (OPT) SELECT * FROM t85353 INNER JOIN u85353@u85353_a_b_idx USING (a,b) WHERE u85353.a < 10; +EXPLAIN (OPT) SELECT * FROM t85353 INNER JOIN u85353@a_b_idx USING (a,b) WHERE u85353.a < 10; ---- explain - ├── columns: info:11 + ├── columns: info:13 ├── mode: opt + ├── immutable └── project - ├── columns: a:1!null b:2!null - └── inner-join (merge) - ├── columns: t85353.a:1!null t85353.b:2!null u85353.a:6!null u85353.b:7!null - ├── left ordering: +6,+7 - ├── right ordering: +1,+2 - ├── fd: (1)==(6), (6)==(1), (2)==(7), (7)==(2) - ├── scan u85353@u85353_a_b_idx - │ ├── columns: u85353.a:6!null u85353.b:7 - │ ├── constraint: /6/7/8: (/NULL - /9] - │ ├── flags: force-index=u85353_a_b_idx - │ └── ordering: +6,+7 - ├── sort + ├── columns: a:1!null b:2!null a_b_hash:8 b_hash:9 + ├── immutable + ├── fd: (1,2)-->(8), (2)-->(9) + └── inner-join (hash) + ├── columns: t85353.a:1!null t85353.b:2!null u85353.a:6!null u85353.b:7!null a_b_hash:8 b_hash:9 + ├── immutable + ├── fd: (6,7)-->(8), (7)-->(9), (1)==(6), (6)==(1), (2)==(7), (7)==(2) + ├── project + │ ├── columns: a_b_hash:8 b_hash:9 u85353.a:6!null u85353.b:7 + │ ├── immutable + │ ├── fd: (6,7)-->(8), (7)-->(9) + │ ├── scan u85353@a_b_idx + │ │ ├── columns: u85353.a:6!null u85353.b:7 + │ │ ├── constraint: /8/6/7/10 + │ │ │ ├── (/0/NULL - /0/9] + │ │ │ ├── (/1/NULL - /1/9] + │ │ │ ├── (/2/NULL - /2/9] + │ │ │ ├── (/3/NULL - /3/9] + │ │ │ ├── (/4/NULL - /4/9] + │ │ │ ├── (/5/NULL - /5/9] + │ │ │ ├── (/6/NULL - /6/9] + │ │ │ └── (/7/NULL - /7/9] + │ │ └── flags: force-index=a_b_idx + │ └── projections + │ ├── mod(fnv32(crdb_internal.datums_to_bytes(u85353.a:6, u85353.b:7)), 8) [as=a_b_hash:8, outer=(6,7), immutable] + │ └── mod(fnv32(crdb_internal.datums_to_bytes(u85353.b:7)), 8) [as=b_hash:9, outer=(7), immutable] + ├── select │ ├── columns: t85353.a:1!null t85353.b:2 - │ ├── ordering: +1,+2 - │ └── select - │ ├── columns: t85353.a:1!null t85353.b:2 - │ ├── scan t85353 - │ │ └── columns: t85353.a:1 t85353.b:2 - │ └── filters - │ └── t85353.a:1 < 10 [outer=(1), constraints=(/1: (/NULL - /9]; tight)] - └── filters (true) + │ ├── scan t85353 + │ │ └── columns: t85353.a:1 t85353.b:2 + │ └── filters + │ └── t85353.a:1 < 10 [outer=(1), constraints=(/1: (/NULL - /9]; tight)] + └── filters + ├── t85353.a:1 = u85353.a:6 [outer=(1,6), constraints=(/1: (/NULL - ]; /6: (/NULL - ]), fd=(1)==(6), (6)==(1)] + └── t85353.b:2 = u85353.b:7 [outer=(2,7), constraints=(/2: (/NULL - ]; /7: (/NULL - ]), fd=(2)==(7), (7)==(2)] ### BEGIN Regression tests for issue #69617 From fb4c174cbbb7265b4b1ea477a882afc9e8b0396e Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Thu, 30 Mar 2023 16:41:07 -0700 Subject: [PATCH 2/3] sql: fix formatting of CREATE TABLE AS with storage parameters We were placing the storage parameters in the wrong place when formatting `CREATE TABLE AS` statements. Fixes: #100243 Epic: None Release note: None --- pkg/sql/parser/testdata/create_table | 8 ++++++++ pkg/sql/sem/tree/create.go | 5 +++++ pkg/sql/sem/tree/pretty.go | 6 +++++- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg/sql/parser/testdata/create_table b/pkg/sql/parser/testdata/create_table index 122a05b9b6b7..a498f86c4391 100644 --- a/pkg/sql/parser/testdata/create_table +++ b/pkg/sql/parser/testdata/create_table @@ -2062,6 +2062,14 @@ CREATE TABLE IF NOT EXISTS a (x, y FAMILY f1) AS SELECT (*) FROM b -- fully pare CREATE TABLE IF NOT EXISTS a (x, y FAMILY f1) AS SELECT * FROM b -- literals removed CREATE TABLE IF NOT EXISTS _ (_, _ FAMILY _) AS SELECT * FROM _ -- identifiers removed +parse +CREATE TABLE a WITH (fillfactor=100) AS SELECT * FROM b +---- +CREATE TABLE a WITH (fillfactor = 100) AS SELECT * FROM b -- normalized! +CREATE TABLE a WITH (fillfactor = (100)) AS SELECT (*) FROM b -- fully parenthesized +CREATE TABLE a WITH (fillfactor = _) AS SELECT * FROM b -- literals removed +CREATE TABLE _ WITH (_ = 100) AS SELECT * FROM _ -- identifiers removed + error CREATE TABLE test ( foo INT8 FAMILY a FAMILY b diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go index 2516806fc2a0..96b47bae7018 100644 --- a/pkg/sql/sem/tree/create.go +++ b/pkg/sql/sem/tree/create.go @@ -1524,6 +1524,11 @@ func (node *CreateTable) FormatBody(ctx *FmtCtx) { ctx.FormatNode(&node.Defs) ctx.WriteByte(')') } + if node.StorageParams != nil { + ctx.WriteString(` WITH (`) + ctx.FormatNode(&node.StorageParams) + ctx.WriteByte(')') + } ctx.WriteString(" AS ") ctx.FormatNode(node.AsSource) } else { diff --git a/pkg/sql/sem/tree/pretty.go b/pkg/sql/sem/tree/pretty.go index a7f5f16675d3..28531e0724bf 100644 --- a/pkg/sql/sem/tree/pretty.go +++ b/pkg/sql/sem/tree/pretty.go @@ -1267,6 +1267,10 @@ func (node *CreateTable) doc(p *PrettyCfg) pretty.Doc { title = pretty.ConcatSpace(title, p.bracket("(", p.Doc(&node.Defs), ")")) } + if node.StorageParams != nil { + title = pretty.ConcatSpace(title, pretty.Keyword("WITH")) + title = pretty.ConcatSpace(title, p.bracket(`(`, p.Doc(&node.StorageParams), `)`)) + } title = pretty.ConcatSpace(title, pretty.Keyword("AS")) } else { title = pretty.ConcatSpace(title, @@ -1281,7 +1285,7 @@ func (node *CreateTable) doc(p *PrettyCfg) pretty.Doc { if node.PartitionByTable != nil { clauses = append(clauses, p.Doc(node.PartitionByTable)) } - if node.StorageParams != nil { + if node.StorageParams != nil && !node.As() { clauses = append( clauses, pretty.ConcatSpace( From 5bfb0d97d311904b57ad4bf40f6abf3304d20a64 Mon Sep 17 00:00:00 2001 From: Michael Erickson Date: Thu, 30 Mar 2023 16:42:48 -0700 Subject: [PATCH 3/3] sql: deflake distsql_automatic_stats logic test Now that automatic stats are disabled for system tables in logic tests, `distsql_automatic_stats` was opening the floodgates with its first `SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true`. With this enabled, the automatic stats refresher was executing `CREATE STATISTICS` for every system table, one at a time, often taking longer than the 45s logic test retry limit to get to the user-defined table we really care about. Instead of using the cluster setting, change `distsql_automatic_stats` to use the per-table setting. Fixes: #99751 Epic: None Release note: None --- .../tests/3node-tenant/generated_test.go | 7 ++++ .../logic_test/distsql_automatic_stats | 32 ++++++------------- .../tests/fakedist-disk/generated_test.go | 7 ++++ .../tests/fakedist-vec-off/generated_test.go | 7 ++++ .../tests/fakedist/generated_test.go | 7 ++++ .../generated_test.go | 7 ++++ .../tests/local-vec-off/generated_test.go | 7 ++++ .../logictest/tests/local/generated_test.go | 7 ++++ 8 files changed, 58 insertions(+), 23 deletions(-) diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index e4fa95ebd12a..a6b4400ccd1d 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -605,6 +605,13 @@ func TestTenantLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestTenantLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestTenantLogic_distsql_event_log( t *testing.T, ) { diff --git a/pkg/sql/logictest/testdata/logic_test/distsql_automatic_stats b/pkg/sql/logictest/testdata/logic_test/distsql_automatic_stats index b2ea065543b1..9495e0668099 100644 --- a/pkg/sql/logictest/testdata/logic_test/distsql_automatic_stats +++ b/pkg/sql/logictest/testdata/logic_test/distsql_automatic_stats @@ -1,19 +1,8 @@ -# LogicTest: local !local !metamorphic-batch-sizes +# LogicTest: !metamorphic-batch-sizes -# TODO(yuzefovich): at the moment the tests below that assert a particular set -# of statistics are flaky, so we disable this file by using a contradictory -# config set. Once #99751 is addressed, "local !local" part should be removed. - -# Disable automatic stats +# Enable automatic stats for this table. statement ok -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false - -statement ok -CREATE TABLE data (a INT, b INT, c FLOAT, d DECIMAL, PRIMARY KEY (a, b, c), INDEX d_idx (d)) - -# Enable automatic stats -statement ok -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true +CREATE TABLE data (a INT, b INT, c FLOAT, d DECIMAL, PRIMARY KEY (a, b, c), INDEX d_idx (d)) WITH (sql_stats_automatic_collection_enabled = true) # Generate all combinations of values 1 to 10. statement ok @@ -40,7 +29,7 @@ __auto__ {d} 1000 1 1000 # Disable automatic stats statement ok -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false +ALTER TABLE data SET (sql_stats_automatic_collection_enabled = false) # Update more than 20% of the table. statement ok @@ -61,7 +50,7 @@ __auto__ {d} 1000 1 1000 # Enable automatic stats statement ok -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true +ALTER TABLE data SET (sql_stats_automatic_collection_enabled = true) # Update more than 20% of the table. statement ok @@ -129,7 +118,7 @@ __auto__ {d} 550 1 0 # Test CREATE TABLE ... AS statement ok -CREATE TABLE copy AS SELECT * FROM data +CREATE TABLE copy WITH (sql_stats_automatic_collection_enabled = true) AS SELECT * FROM data # Distinct count for rowid can be flaky, so don't show it. The estimate is # almost always 550, but occasionally it is 549... @@ -145,7 +134,7 @@ __auto__ {d} 550 0 __auto__ {rowid} 550 0 statement ok -CREATE TABLE test_create (x INT PRIMARY KEY, y CHAR) +CREATE TABLE test_create (x INT PRIMARY KEY, y CHAR) WITH (sql_stats_automatic_collection_enabled = true) query TTIII colnames,retry SELECT DISTINCT ON (column_names) statistics_name, column_names, row_count, distinct_count, null_count @@ -174,16 +163,13 @@ __auto__ {rowid} 0 0 # Test user-defined schemas. # Disable automatic stats -statement ok -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false - statement ok CREATE SCHEMA my_schema; -CREATE TABLE my_schema.my_table (k INT PRIMARY KEY, v STRING); +CREATE TABLE my_schema.my_table (k INT PRIMARY KEY, v STRING) WITH (sql_stats_automatic_collection_enabled = false) # Enable automatic stats statement ok -SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true +ALTER TABLE my_schema.my_table SET (sql_stats_automatic_collection_enabled = true) # Insert 10 rows. statement ok diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index 2f0eced5bfdd..67b20ec7f1c2 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -583,6 +583,13 @@ func TestLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestLogic_distsql_event_log( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index 23a891ed970c..d897cdf6cc52 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -583,6 +583,13 @@ func TestLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestLogic_distsql_event_log( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 240bb47fb74c..b5ab2ecb7404 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -583,6 +583,13 @@ func TestLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestLogic_distsql_event_log( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go index 29d1dba543b7..20a4224814e8 100644 --- a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go +++ b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go @@ -576,6 +576,13 @@ func TestLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestLogic_distsql_event_log( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index 553e10bb6538..860b4ca1cf5f 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -583,6 +583,13 @@ func TestLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestLogic_distsql_event_log( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index ac1a0c1aaa5d..d9220d202593 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -604,6 +604,13 @@ func TestLogic_distinct_on( runLogicTest(t, "distinct_on") } +func TestLogic_distsql_automatic_stats( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "distsql_automatic_stats") +} + func TestLogic_distsql_event_log( t *testing.T, ) {