From 2c6bcd90012d59f57b9b5f843d2c6d06baf2ce20 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Thu, 23 Jul 2020 06:30:51 -0500 Subject: [PATCH 1/4] stats: fix flake in TestCacheWait This commit fixes a flake in TestCacheWait, which was introduced by #51616. That PR changed the call to InvalidateTableStats in TestCacheWait to RefreshTableStats, but that change should not have been made. The purpose of the test is to test that the cache invalidation and waiting mechanisms work correctly, and therefore it must call InvalidateTableStats, not RefreshTableStats. Fixes #51712 Release note: None --- pkg/sql/stats/stats_cache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sql/stats/stats_cache_test.go b/pkg/sql/stats/stats_cache_test.go index 1e0b08c9f8ae..d4e4f1958366 100644 --- a/pkg/sql/stats/stats_cache_test.go +++ b/pkg/sql/stats/stats_cache_test.go @@ -377,7 +377,7 @@ func TestCacheWait(t *testing.T) { before := sc.mu.numInternalQueries id := tableIDs[rand.Intn(len(tableIDs))] - sc.RefreshTableStats(ctx, id) + sc.InvalidateTableStats(ctx, id) // Run GetTableStats multiple times in parallel. var wg sync.WaitGroup for n := 0; n < 10; n++ { From 1bb2a5586b698f1aeb9f628d4259a40b7a1a9665 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Thu, 23 Jul 2020 08:52:22 -0500 Subject: [PATCH 2/4] sql: fix flake in TestQueryCache/group/statschange This commit fixes a flake in TestQueryCache/group/statschange, which was introduced by #51616. That PR made updates to the stats cache asynchronous, so we can no longer expect the query cache to be invalidated immediately after a stats update. This commit fixes the problem by introducing a retry mechanism into the test. Fixes #51693 Release note: None --- pkg/sql/plan_opt_test.go | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/pkg/sql/plan_opt_test.go b/pkg/sql/plan_opt_test.go index 31767f272d2f..21145a290a19 100644 --- a/pkg/sql/plan_opt_test.go +++ b/pkg/sql/plan_opt_test.go @@ -90,6 +90,20 @@ func (h *queryCacheTestHelper) AssertStats(tb *testing.T, expHits, expMisses int assert.Equal(tb, expMisses, misses, "misses") } +// CheckStats is similar to AssertStats, but returns an error instead of +// failing the test if the actual stats don't match the expected stats. +func (h *queryCacheTestHelper) CheckStats(tb *testing.T, expHits, expMisses int) error { + tb.Helper() + hits, misses := h.GetStats() + if expHits != hits { + return errors.Errorf("expected %d hits but found %d", expHits, hits) + } + if expMisses != misses { + return errors.Errorf("expected %d misses but found %d", expMisses, misses) + } + return nil +} + func TestQueryCache(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) @@ -347,8 +361,17 @@ SELECT cte.x, cte.y FROM cte LEFT JOIN cte as cte2 on cte.y = cte2.x`, j) h.AssertStats(t, 1 /* hits */, 1 /* misses */) r0.Exec(t, "CREATE STATISTICS s FROM t") h.AssertStats(t, 1 /* hits */, 1 /* misses */) - r1.CheckQueryResults(t, "SELECT * FROM t", [][]string{{"1", "1"}}) - h.AssertStats(t, 1 /* hits */, 2 /* misses */) + hits := 1 + testutils.SucceedsSoon(t, func() error { + // The stats cache is updated asynchronously, so we may get some hits + // before we get a miss. + r1.CheckQueryResults(t, "SELECT * FROM t", [][]string{{"1", "1"}}) + if err := h.CheckStats(t, hits, 2 /* misses */); err != nil { + hits++ + return err + } + return nil + }) }) // Test that a schema change triggers cache invalidation. From 42e24c4bff991b615d197d646f0f771a8bfbc4a2 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Thu, 23 Jul 2020 08:30:22 -0700 Subject: [PATCH 3/4] sql: don't show the "hidden" column flag in EXPLAIN The ResultColumns.Hidden flag is used internally; it does not carry useful information for users and should not be visible in EXPLAIN. Release note (sql change): EXPLAIN no longer shows the "hidden" annotation for columns. --- pkg/sql/opt/exec/execbuilder/testdata/explain | 20 +++--- pkg/sql/opt/exec/execbuilder/testdata/insert | 32 +++++----- pkg/sql/opt/exec/execbuilder/testdata/orderby | 24 +++---- pkg/sql/opt/exec/execbuilder/testdata/select | 20 +++--- .../exec/execbuilder/testdata/select_index | 52 ++++++++-------- .../opt/exec/execbuilder/testdata/subquery | 24 +++---- pkg/sql/opt/exec/execbuilder/testdata/tuple | 20 +++--- pkg/sql/opt/exec/execbuilder/testdata/update | 62 +++++++++---------- pkg/sql/opt/exec/execbuilder/testdata/upsert | 4 +- pkg/sql/opt/exec/execbuilder/testdata/with | 4 +- pkg/sql/opt/exec/explain/output.go | 2 +- pkg/sql/sqlbase/result_columns.go | 7 ++- 12 files changed, 136 insertions(+), 135 deletions(-) diff --git a/pkg/sql/opt/exec/execbuilder/testdata/explain b/pkg/sql/opt/exec/execbuilder/testdata/explain index 456c41716c98..558484b68d9d 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/explain +++ b/pkg/sql/opt/exec/execbuilder/testdata/explain @@ -601,16 +601,16 @@ CREATE TABLE tc (a INT, b INT, INDEX c(a), FAMILY "primary" (a, b, rowid)) query TTTTT EXPLAIN (VERBOSE) SELECT * FROM tc WHERE a = 10 ORDER BY b ---- -· distribution local · · -· vectorized true · · -sort · · (a, b) +b - │ order +b · · - └── index-join · · (a, b) · - │ table tc@primary · · - │ key columns rowid · · - └── scan · · (a, rowid[hidden]) · -· table tc@c · · -· spans /10-/11 · · +· distribution local · · +· vectorized true · · +sort · · (a, b) +b + │ order +b · · + └── index-join · · (a, b) · + │ table tc@primary · · + │ key columns rowid · · + └── scan · · (a, rowid) · +· table tc@c · · +· spans /10-/11 · · query TTTTT colnames EXPLAIN (TYPES) INSERT INTO t VALUES (1, 2) diff --git a/pkg/sql/opt/exec/execbuilder/testdata/insert b/pkg/sql/opt/exec/execbuilder/testdata/insert index 071ef7c1b116..1750184734fc 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/insert +++ b/pkg/sql/opt/exec/execbuilder/testdata/insert @@ -428,8 +428,8 @@ INSERT INTO insert_t (SELECT length(k), 2 FROM kv ORDER BY k || v LIMIT 10) RETU · vectorized false · · render · · ("?column?") · │ render 0 x + v · · - └── run · · (x, v, rowid[hidden]) · - └── insert · · (x, v, rowid[hidden]) · + └── run · · (x, v, rowid) · + └── insert · · (x, v, rowid) · │ into insert_t(x, v, rowid) · · │ strategy inserter · · └── render · · (length, "?column?", column11) · @@ -462,18 +462,18 @@ BEGIN; ALTER TABLE mutation DROP COLUMN y query TTTTT EXPLAIN (VERBOSE) INSERT INTO mutation(x) VALUES (2) RETURNING * ---- -· distribution local · · -· vectorized false · · -render · · (x) · - │ render 0 x · · - └── run · · (x, rowid[hidden]) · - └── insert-fast-path · · (x, rowid[hidden]) · -· into mutation(x, rowid, y) · · -· strategy inserter · · -· size 3 columns, 1 row · · -· row 0, expr 0 2 · · -· row 0, expr 1 unique_rowid() · · -· row 0, expr 2 10 · · +· distribution local · · +· vectorized false · · +render · · (x) · + │ render 0 x · · + └── run · · (x, rowid) · + └── insert-fast-path · · (x, rowid) · +· into mutation(x, rowid, y) · · +· strategy inserter · · +· size 3 columns, 1 row · · +· row 0, expr 0 2 · · +· row 0, expr 1 unique_rowid() · · +· row 0, expr 2 10 · · statement ok ROLLBACK @@ -527,8 +527,8 @@ root · · └── spool · · (z) · └── render · · (z) · │ render 0 z · · - └── run · · (z, rowid[hidden]) · - └── insert · · (z, rowid[hidden]) · + └── run · · (z, rowid) · + └── insert · · (z, rowid) · │ into xyz(x, y, z, rowid) · · │ strategy inserter · · └── render · · (a, b, c, column11) · diff --git a/pkg/sql/opt/exec/execbuilder/testdata/orderby b/pkg/sql/opt/exec/execbuilder/testdata/orderby index c3ff806c8e5c..c743d73feb80 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/orderby +++ b/pkg/sql/opt/exec/execbuilder/testdata/orderby @@ -907,15 +907,15 @@ CREATE TABLE xyz (x INT, y INT, z INT, INDEX(z,y)) query TTTTT EXPLAIN (VERBOSE) SELECT * FROM xyz WHERE z=1 AND x=y ORDER BY x; ---- -· distribution local · · -· vectorized true · · -sort · · (x, y, z) +x - │ order +x · · - └── filter · · (x, y, z) · - │ filter x = y · · - └── index-join · · (x, y, z) · - │ table xyz@primary · · - │ key columns rowid · · - └── scan · · (y, z, rowid[hidden]) · -· table xyz@xyz_z_y_idx · · -· spans /1/!NULL-/2 · · +· distribution local · · +· vectorized true · · +sort · · (x, y, z) +x + │ order +x · · + └── filter · · (x, y, z) · + │ filter x = y · · + └── index-join · · (x, y, z) · + │ table xyz@primary · · + │ key columns rowid · · + └── scan · · (y, z, rowid) · +· table xyz@xyz_z_y_idx · · +· spans /1/!NULL-/2 · · diff --git a/pkg/sql/opt/exec/execbuilder/testdata/select b/pkg/sql/opt/exec/execbuilder/testdata/select index 35ec87023f75..7e1d39fa951e 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/select +++ b/pkg/sql/opt/exec/execbuilder/testdata/select @@ -238,11 +238,11 @@ scan · · () · query TTTTT EXPLAIN (VERBOSE) SELECT rowid FROM [54(3) AS num_ref_alias] ---- -· distribution local · · -· vectorized true · · -scan · · (rowid[hidden]) · -· table num_ref_hidden@primary · · -· spans FULL SCAN · · +· distribution local · · +· vectorized true · · +scan · · (rowid) · +· table num_ref_hidden@primary · · +· spans FULL SCAN · · query error pq: \[666\(1\) AS num_ref_alias\]: relation \"\[666\]\" does not exist EXPLAIN (VERBOSE) SELECT * FROM [666(1) AS num_ref_alias] @@ -400,11 +400,11 @@ scan · · (x, y) · query TTTTT EXPLAIN (VERBOSE) SELECT x, y, rowid FROM b WHERE rowid > 0 ---- -· distribution local · · -· vectorized true · · -scan · · (x, y, rowid[hidden]) · -· table b@primary · · -· spans /1- · · +· distribution local · · +· vectorized true · · +scan · · (x, y, rowid) · +· table b@primary · · +· spans /1- · · statement ok DROP TABLE b diff --git a/pkg/sql/opt/exec/execbuilder/testdata/select_index b/pkg/sql/opt/exec/execbuilder/testdata/select_index index b03391774600..6d5e41b48914 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/select_index +++ b/pkg/sql/opt/exec/execbuilder/testdata/select_index @@ -966,40 +966,40 @@ CREATE TABLE xy (x INT, y INT, INDEX (y)) query TTTTT EXPLAIN (VERBOSE) SELECT * FROM xy WHERE y IS NOT DISTINCT FROM NULL ---- -· distribution local · · -· vectorized true · · -index-join · · (x, y) · - │ table xy@primary · · - │ key columns rowid · · - └── scan · · (y, rowid[hidden]) · -· table xy@xy_y_idx · · -· spans /NULL-/!NULL · · +· distribution local · · +· vectorized true · · +index-join · · (x, y) · + │ table xy@primary · · + │ key columns rowid · · + └── scan · · (y, rowid) · +· table xy@xy_y_idx · · +· spans /NULL-/!NULL · · query TTTTT EXPLAIN (VERBOSE) SELECT * FROM xy WHERE y IS NOT DISTINCT FROM 4 ---- -· distribution local · · -· vectorized true · · -index-join · · (x, y) · - │ table xy@primary · · - │ key columns rowid · · - └── scan · · (y, rowid[hidden]) · -· table xy@xy_y_idx · · -· spans /4-/5 · · +· distribution local · · +· vectorized true · · +index-join · · (x, y) · + │ table xy@primary · · + │ key columns rowid · · + └── scan · · (y, rowid) · +· table xy@xy_y_idx · · +· spans /4-/5 · · query TTTTT EXPLAIN (VERBOSE) SELECT x FROM xy WHERE y > 0 AND y < 2 ORDER BY y ---- -· distribution local · · -· vectorized true · · -render · · (x) · - │ render 0 x · · - └── index-join · · (x, y) · - │ table xy@primary · · - │ key columns rowid · · - └── scan · · (y, rowid[hidden]) · -· table xy@xy_y_idx · · -· spans /1-/2 · · +· distribution local · · +· vectorized true · · +render · · (x) · + │ render 0 x · · + └── index-join · · (x, y) · + │ table xy@primary · · + │ key columns rowid · · + └── scan · · (y, rowid) · +· table xy@xy_y_idx · · +· spans /1-/2 · · query TTTTT EXPLAIN (VERBOSE) SELECT * FROM xy WHERE y IS DISTINCT FROM NULL diff --git a/pkg/sql/opt/exec/execbuilder/testdata/subquery b/pkg/sql/opt/exec/execbuilder/testdata/subquery index 46e1364323c4..384a890bd010 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/subquery +++ b/pkg/sql/opt/exec/execbuilder/testdata/subquery @@ -144,18 +144,18 @@ WHERE OR (col4 IN (SELECT col1 FROM tab4 WHERE col1 > 8.27)) AND (col3 <= 5 AND (col3 BETWEEN 7 AND 9)) ---- -· distribution local · · -· vectorized true · · -render · · (col0) · - │ render 0 col0 · · - └── index-join · · (col0, col3, col4, rowid[hidden]) · - │ table tab4@primary · · - │ key columns rowid · · - └── filter · · (col0, col4, rowid[hidden]) · - │ filter col0 <= 0 · · - └── scan · · (col0, col4, rowid[hidden]) · -· table tab4@idx_tab4_0 · · -· spans /!NULL-/5.38/1 · · +· distribution local · · +· vectorized true · · +render · · (col0) · + │ render 0 col0 · · + └── index-join · · (col0, col3, col4, rowid) · + │ table tab4@primary · · + │ key columns rowid · · + └── filter · · (col0, col4, rowid) · + │ filter col0 <= 0 · · + └── scan · · (col0, col4, rowid) · +· table tab4@idx_tab4_0 · · +· spans /!NULL-/5.38/1 · · # ------------------------------------------------------------------------------ # Correlated subqueries. diff --git a/pkg/sql/opt/exec/execbuilder/testdata/tuple b/pkg/sql/opt/exec/execbuilder/testdata/tuple index 89a8f167c922..41decf457ea4 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/tuple +++ b/pkg/sql/opt/exec/execbuilder/testdata/tuple @@ -87,16 +87,16 @@ CREATE TABLE abc (a INT, b INT, c INT, INDEX(a, b)) query TTTTT EXPLAIN (VERBOSE) SELECT * FROM abc WHERE (a, b, c) > (1, 2, 3) AND (a,b,c) < (8, 9, 10) ---- -· distribution local · · -· vectorized true · · -filter · · (a, b, c) · - │ filter ((a, b, c) > (1, 2, 3)) AND ((a, b, c) < (8, 9, 10)) · · - └── index-join · · (a, b, c) · - │ table abc@primary · · - │ key columns rowid · · - └── scan · · (a, b, rowid[hidden]) · -· table abc@abc_a_b_idx · · -· spans /1/2-/8/10 · · +· distribution local · · +· vectorized true · · +filter · · (a, b, c) · + │ filter ((a, b, c) > (1, 2, 3)) AND ((a, b, c) < (8, 9, 10)) · · + └── index-join · · (a, b, c) · + │ table abc@primary · · + │ key columns rowid · · + └── scan · · (a, b, rowid) · +· table abc@abc_a_b_idx · · +· spans /1/2-/8/10 · · statement ok DROP TABLE abc diff --git a/pkg/sql/opt/exec/execbuilder/testdata/update b/pkg/sql/opt/exec/execbuilder/testdata/update index ca9c8d414d40..f5a010104a6c 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/update +++ b/pkg/sql/opt/exec/execbuilder/testdata/update @@ -339,37 +339,37 @@ CREATE TABLE abc (a INT, b INT, c INT, INDEX(c) STORING(a,b)) query TTTTT EXPLAIN (VERBOSE) SELECT * FROM [ UPDATE abc SET a=c RETURNING a ] ORDER BY a ---- -· distribution local · · -· vectorized false · · -root · · (a) +a - ├── sort · · (a) +a - │ │ order +a · · - │ └── scan buffer node · · (a) · - │ label buffer 1 · · - └── subquery · · · · - │ id @S1 · · - │ original sql UPDATE abc SET a = c RETURNING a · · - │ exec mode all rows · · - └── buffer node · · (a) · - │ label buffer 1 · · - └── spool · · (a) · - └── render · · (a) · - │ render 0 a · · - └── run · · (a, rowid[hidden]) · - └── update · · (a, rowid[hidden]) · - │ table abc · · - │ set a · · - │ strategy updater · · - └── render · · (a, b, c, rowid, c) · - │ render 0 a · · - │ render 1 b · · - │ render 2 c · · - │ render 3 rowid · · - │ render 4 c · · - └── scan · · (a, b, c, rowid[hidden]) · -· table abc@primary · · -· spans FULL SCAN · · -· locking strength for update · · +· distribution local · · +· vectorized false · · +root · · (a) +a + ├── sort · · (a) +a + │ │ order +a · · + │ └── scan buffer node · · (a) · + │ label buffer 1 · · + └── subquery · · · · + │ id @S1 · · + │ original sql UPDATE abc SET a = c RETURNING a · · + │ exec mode all rows · · + └── buffer node · · (a) · + │ label buffer 1 · · + └── spool · · (a) · + └── render · · (a) · + │ render 0 a · · + └── run · · (a, rowid) · + └── update · · (a, rowid) · + │ table abc · · + │ set a · · + │ strategy updater · · + └── render · · (a, b, c, rowid, c) · + │ render 0 a · · + │ render 1 b · · + │ render 2 c · · + │ render 3 rowid · · + │ render 4 c · · + └── scan · · (a, b, c, rowid) · +· table abc@primary · · +· spans FULL SCAN · · +· locking strength for update · · # ------------------------------------------------------------------------------ # Regression for #35364. This tests behavior that is different between the CBO diff --git a/pkg/sql/opt/exec/execbuilder/testdata/upsert b/pkg/sql/opt/exec/execbuilder/testdata/upsert index fb778e6f2bfe..be586eeec24a 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/upsert +++ b/pkg/sql/opt/exec/execbuilder/testdata/upsert @@ -365,8 +365,8 @@ root · · └── spool · · (z) · └── render · · (z) · │ render 0 z · · - └── run · · (z, rowid[hidden]) · - └── upsert · · (z, rowid[hidden]) · + └── run · · (z, rowid) · + └── upsert · · (z, rowid) · │ into xyz(x, y, z, rowid) · · │ strategy opt upserter · · └── render · · (a, b, c, column11, a, b, c) · diff --git a/pkg/sql/opt/exec/execbuilder/testdata/with b/pkg/sql/opt/exec/execbuilder/testdata/with index 07757f8f9974..d741c1f2fef8 100644 --- a/pkg/sql/opt/exec/execbuilder/testdata/with +++ b/pkg/sql/opt/exec/execbuilder/testdata/with @@ -59,8 +59,8 @@ root · · └── spool · · (a) · └── render · · (a) · │ render 0 a · · - └── run · · (a, rowid[hidden]) · - └── insert · · (a, rowid[hidden]) · + └── run · · (a, rowid) · + └── insert · · (a, rowid) · │ into x(a, rowid) · · │ strategy inserter · · └── values · · (column1, column5) · diff --git a/pkg/sql/opt/exec/explain/output.go b/pkg/sql/opt/exec/explain/output.go index afac0b323a18..b88f7a114a7e 100644 --- a/pkg/sql/opt/exec/explain/output.go +++ b/pkg/sql/opt/exec/explain/output.go @@ -61,7 +61,7 @@ func (ob *OutputBuilder) EnterNode( ) { var colStr, ordStr string if ob.flags.Verbose { - colStr = columns.String(ob.flags.ShowTypes) + colStr = columns.String(ob.flags.ShowTypes, false /* showHidden */) ordStr = ordering.String(columns) } ob.enterNode(name, colStr, ordStr) diff --git a/pkg/sql/sqlbase/result_columns.go b/pkg/sql/sqlbase/result_columns.go index e74fc8cf2e5f..3a227e868fc4 100644 --- a/pkg/sql/sqlbase/result_columns.go +++ b/pkg/sql/sqlbase/result_columns.go @@ -103,8 +103,9 @@ func (r ResultColumns) NodeFormatter(colIdx int) tree.NodeFormatter { } // String formats result columns to a string. -// The column types are printed if the argument specifies so. -func (r ResultColumns) String(printTypes bool) string { +// The column types are printed if printTypes is true. +// The hidden property is printed if showHidden is true. +func (r ResultColumns) String(printTypes bool, showHidden bool) string { f := tree.NewFmtCtx(tree.FmtSimple) f.WriteByte('(') for i := range r { @@ -124,7 +125,7 @@ func (r ResultColumns) String(printTypes bool) string { hasProps = true f.WriteString(prop) } - if rCol.Hidden { + if showHidden && rCol.Hidden { outputProp("hidden") } if hasProps { From eaeb7c17f21c7c0934828667416306a7a5cc99d5 Mon Sep 17 00:00:00 2001 From: Rohan Yadav Date: Tue, 21 Jul 2020 23:07:35 -0400 Subject: [PATCH 4/4] descs: push descriptor type hydration into the desc.Collection Fixes #49484. Preparation for #51385. Up until now, the planner was responsible for installing user defined type metadata in tables that contained user defined types. This was slightly messy and caused leakage of responsibility regarding when descriptors had user defined types vs when they didn't. This commit pushes that responsibility into the descs.Collection. It also paves the way for work to avoid copying ImmutableTableDescriptors that contain user defined types every time that they need hydration. Release note: None --- pkg/sql/backfill.go | 16 ------- pkg/sql/catalog/descs/collection.go | 66 +++++++++++++++++++++++++++-- pkg/sql/planner.go | 8 +--- pkg/sql/resolver.go | 42 ------------------ 4 files changed, 63 insertions(+), 69 deletions(-) diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 3ae80f31c355..7921a4e5a41d 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -28,7 +28,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" @@ -655,21 +654,6 @@ func (sc *SchemaChanger) truncateIndexes( if err != nil { return err } - - // Hydrate types used in the retrieved table. - // TODO (rohany): This can be removed once table access from the - // desc.Collection returns tables with hydrated types. - typLookup := func(ctx context.Context, id sqlbase.ID) (*tree.TypeName, sqlbase.TypeDescriptorInterface, error) { - return resolver.ResolveTypeDescByID(ctx, txn, sc.execCfg.Codec, id, tree.ObjectLookupFlags{}) - } - if err := sqlbase.HydrateTypesInTableDescriptor( - ctx, - tableDesc.TableDesc(), - sqlbase.TypeLookupFunc(typLookup), - ); err != nil { - return err - } - rd, err := row.MakeDeleter( ctx, txn, diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index 4d1bc55abcb2..7da8859089b5 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -29,6 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" ) @@ -162,7 +163,11 @@ func (tc *Collection) GetMutableTableDescriptor( if !ok { return nil, nil } - return mutDesc, nil + hydrated, err := tc.hydrateTypesInTableDesc(ctx, txn, mutDesc) + if err != nil { + return nil, err + } + return hydrated.(*sqlbase.MutableTableDescriptor), nil } func (tc *Collection) getMutableObjectDescriptor( @@ -288,7 +293,11 @@ func (tc *Collection) GetTableVersion( } return nil, nil } - return table, nil + hydrated, err := tc.hydrateTypesInTableDesc(ctx, txn, table) + if err != nil { + return nil, err + } + return hydrated.(*sqlbase.ImmutableTableDescriptor), nil } func (tc *Collection) getObjectVersion( @@ -434,7 +443,11 @@ func (tc *Collection) GetTableVersionByID( return nil, sqlbase.NewUndefinedRelationError( &tree.TableRef{TableID: int64(tableID)}) } - return table, nil + hydrated, err := tc.hydrateTypesInTableDesc(ctx, txn, table) + if err != nil { + return nil, err + } + return hydrated.(*sqlbase.ImmutableTableDescriptor), nil } func (tc *Collection) getDescriptorVersionByID( @@ -506,7 +519,12 @@ func (tc *Collection) GetMutableTableVersionByID( if err != nil { return nil, err } - return desc.(*sqlbase.MutableTableDescriptor), nil + table := desc.(*sqlbase.MutableTableDescriptor) + hydrated, err := tc.hydrateTypesInTableDesc(ctx, txn, table) + if err != nil { + return nil, err + } + return hydrated.(*sqlbase.MutableTableDescriptor), nil } func (tc *Collection) getMutableDescriptorByID( @@ -528,6 +546,46 @@ func (tc *Collection) getMutableDescriptorByID( return desc, nil } +// hydrateTypesInTableDesc installs user defined type metadata in all types.T +// present in the input TableDescriptor. It always returns the same type of +// TableDescriptor that was passed in. It ensures that ImmutableTableDescriptors +// are not modified during the process of metadata installation. +func (tc *Collection) hydrateTypesInTableDesc( + ctx context.Context, txn *kv.Txn, desc sqlbase.TableDescriptorInterface, +) (sqlbase.TableDescriptorInterface, error) { + getType := func(ctx context.Context, id sqlbase.ID) (*tree.TypeName, sqlbase.TypeDescriptorInterface, error) { + // TODO (rohany): Use the collection API's here. + return resolver.ResolveTypeDescByID(ctx, txn, tc.codec(), id, tree.ObjectLookupFlags{}) + } + switch t := desc.(type) { + case *sqlbase.MutableTableDescriptor: + // It is safe to hydrate directly into MutableTableDescriptor since it is + // not shared. + return desc, sqlbase.HydrateTypesInTableDescriptor(ctx, t.TableDesc(), sqlbase.TypeLookupFunc(getType)) + case *sqlbase.ImmutableTableDescriptor: + // ImmutableTableDescriptors need to be copied before hydration, because + // they are potentially read by multiple threads. If there aren't any user + // defined types in the descriptor, then return early. + if !t.ContainsUserDefinedTypes() { + return desc, nil + } + + // TODO (rohany, ajwerner): Here we would look into the cached set of + // hydrated table descriptors and potentially return without having to + // make a copy. However, we could avoid hitting the cache if any of the + // user defined types have been modified in this transaction. + + // Make a copy of the underlying descriptor before hydration. + descBase := protoutil.Clone(t.TableDesc()).(*sqlbase.TableDescriptor) + if err := sqlbase.HydrateTypesInTableDescriptor(ctx, descBase, sqlbase.TypeLookupFunc(getType)); err != nil { + return nil, err + } + return sqlbase.NewImmutableTableDescriptor(*descBase), nil + default: + return desc, nil + } +} + // ReleaseSpecifiedLeases releases the leases for the descriptors with ids in // the passed slice. Errors are logged but ignored. func (tc *Collection) ReleaseSpecifiedLeases(ctx context.Context, descs []lease.IDVersion) { diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index 873f0560e324..bc6af2019d4f 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -490,13 +490,7 @@ func (p *planner) LookupTableByID( } return catalog.TableEntry{}, err } - // TODO (rohany): This shouldn't be needed once the descs.Collection always - // returns descriptors with hydrated types. - hydratedDesc, err := p.maybeHydrateTypesInDescriptor(ctx, table) - if err != nil { - return catalog.TableEntry{}, err - } - return catalog.TableEntry{Desc: hydratedDesc.(*sqlbase.ImmutableTableDescriptor)}, nil + return catalog.TableEntry{Desc: table}, nil } // TypeAsString enforces (not hints) that the given expression typechecks as a diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index 081713f9d4c4..fa777f525b06 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -16,7 +16,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" - "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -25,7 +24,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/sql/types" - "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" ) @@ -129,14 +127,6 @@ func (p *planner) LookupObject( lookupFlags.CommonLookupFlags = p.CommonLookupFlags(false /* required */) objDesc, err := sc.GetObjectDesc(ctx, p.txn, p.ExecCfg().Settings, p.ExecCfg().Codec, dbName, scName, tbName, lookupFlags) - // The returned object may contain types.T that need hydrating. - if objDesc != nil { - objDesc, err = p.maybeHydrateTypesInDescriptor(ctx, objDesc) - if err != nil { - return false, nil, err - } - } - return objDesc != nil, objDesc, err } @@ -205,38 +195,6 @@ func (p *planner) ResolveTypeByID(ctx context.Context, id uint32) (*types.T, err return desc.MakeTypesT(ctx, name, p) } -// maybeHydrateTypesInDescriptor hydrates any types.T's in the input descriptor. -// TODO (rohany): Once we lease types, this should be pushed down into the -// leased object collection. -func (p *planner) maybeHydrateTypesInDescriptor( - ctx context.Context, objDesc catalog.Descriptor, -) (catalog.Descriptor, error) { - // As of now, only {Mutable,Immutable}TableDescriptor have types.T that - // need to be hydrated. - switch t := objDesc.(type) { - case *sqlbase.MutableTableDescriptor: - // MutableTableDescriptors are safe to modify in place. - if err := sqlbase.HydrateTypesInTableDescriptor(ctx, t.TableDesc(), p); err != nil { - return nil, err - } - return objDesc, nil - case *sqlbase.ImmutableTableDescriptor: - // ImmutableTableDescriptors need to be copied before hydration. If there - // aren't any user defined types in the descriptor, then just return. - if !t.ContainsUserDefinedTypes() { - return objDesc, nil - } - // Make a copy of the underlying TableDescriptor. - desc := protoutil.Clone(t.TableDesc()).(*sqlbase.TableDescriptor) - if err := sqlbase.HydrateTypesInTableDescriptor(ctx, desc, p); err != nil { - return nil, err - } - return sqlbase.NewImmutableTableDescriptor(*desc), nil - default: - return objDesc, nil - } -} - // ObjectLookupFlags is part of the resolver.SchemaResolver interface. func (p *planner) ObjectLookupFlags(required, requireMutable bool) tree.ObjectLookupFlags { return tree.ObjectLookupFlags{