From a5e95a2916ad7896ea77ac84360addb39e530cda Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Fri, 28 Jun 2024 12:56:25 -0400 Subject: [PATCH] sql: fix nil pointer caused by "".crdb_internal.create_statements When querying crdb_internal, "" can be used as the database name so that the result includes information from all databases rather than just the current one. This could cause a nil pointer for tables backed by the populateVirtualIndexForTable helper. This is now fixed. Since this is an internal-only undocumented feature, there is no release note. Release note: None --- .../testdata/logic_test/crdb_internal | 38 +++++++++++++++---- pkg/sql/pg_catalog.go | 18 ++++++--- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal index 02abfb8304e0..73e737ce2e16 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal @@ -1231,6 +1231,30 @@ SELECT is_temporary FROM crdb_internal.create_statements WHERE descriptor_name = ---- true +statement ok +CREATE TABLE defaultdb.public.in_other_db (x INT PRIMARY KEY); +CREATE TABLE public.in_this_db (x INT PRIMARY KEY); + +# Verify that we can inspect all databases by using "" as the database name. +query TTT colnames +SELECT database_name, schema_name, descriptor_name +FROM "".crdb_internal.create_statements +WHERE descriptor_name IN ('in_other_db', 'in_this_db') +ORDER BY 1,2,3 +---- +database_name schema_name descriptor_name +defaultdb public in_other_db +test public in_this_db + +# Also verify that using the virtul index on descriptor_id works. +query TTT colnames +SELECT database_name, schema_name, descriptor_name +FROM "".crdb_internal.create_statements +WHERE descriptor_id = 'defaultdb.public.in_other_db'::regclass::int +---- +database_name schema_name descriptor_name +defaultdb public in_other_db + query TT SELECT * FROM crdb_internal.regions ORDER BY 1 ---- @@ -1586,17 +1610,17 @@ FROM crdb_internal.create_procedure_statements WHERE procedure_name IN ('p', 'p2') ORDER BY procedure_id; ---- -104 test 105 public 137 p CREATE PROCEDURE public.p(INT8) +104 test 105 public 139 p CREATE PROCEDURE public.p(INT8) LANGUAGE SQL AS $$ SELECT 1; $$ -104 test 105 public 138 p CREATE PROCEDURE public.p(STRING, b INT8) +104 test 105 public 140 p CREATE PROCEDURE public.p(STRING, b INT8) LANGUAGE SQL AS $$ SELECT 'hello'; $$ -104 test 140 sc 141 p2 CREATE PROCEDURE sc.p2(STRING) +104 test 142 sc 143 p2 CREATE PROCEDURE sc.p2(STRING) LANGUAGE SQL AS $$ SELECT 'hello'; @@ -1614,22 +1638,22 @@ FROM "".crdb_internal.create_procedure_statements WHERE procedure_name IN ('p', 'p2', 'p_cross_db') ORDER BY procedure_id; ---- -104 test 105 public 137 p CREATE PROCEDURE public.p(INT8) +104 test 105 public 139 p CREATE PROCEDURE public.p(INT8) LANGUAGE SQL AS $$ SELECT 1; $$ -104 test 105 public 138 p CREATE PROCEDURE public.p(STRING, b INT8) +104 test 105 public 140 p CREATE PROCEDURE public.p(STRING, b INT8) LANGUAGE SQL AS $$ SELECT 'hello'; $$ -104 test 140 sc 141 p2 CREATE PROCEDURE sc.p2(STRING) +104 test 142 sc 143 p2 CREATE PROCEDURE sc.p2(STRING) LANGUAGE SQL AS $$ SELECT 'hello'; $$ -142 test_cross_db 143 public 144 p_cross_db CREATE PROCEDURE public.p_cross_db() +144 test_cross_db 145 public 146 p_cross_db CREATE PROCEDURE public.p_cross_db() LANGUAGE SQL AS $$ SELECT 1; diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index 3b2c5ec8fe9a..0d70f71ff1e1 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -5211,7 +5211,7 @@ func funcVolatility(v catpb.Function_Volatility) string { func populateVirtualIndexForTable( ctx context.Context, p *planner, - db catalog.DatabaseDescriptor, + dbContext catalog.DatabaseDescriptor, tableDesc catalog.TableDescriptor, addRow func(...tree.Datum) error, populateFromTable func(ctx context.Context, p *planner, h oidHasher, db catalog.DatabaseDescriptor, @@ -5222,7 +5222,7 @@ func populateVirtualIndexForTable( // Don't include tables that aren't in the current database unless // they're virtual, dropped tables, or ones that the user can't see. - canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, tableDesc, db, true /*allowAdding*/) + canSeeDescriptor, err := userCanSeeDescriptor(ctx, p, tableDesc, dbContext, true /*allowAdding*/) if err != nil { return false, err } @@ -5230,8 +5230,9 @@ func populateVirtualIndexForTable( // or are dropped. From a virtual index viewpoint, we will consider // this result set as populated, since the underlying full table will // also skip the same descriptors. - if (!tableDesc.IsVirtualTable() && tableDesc.GetParentID() != db.GetID()) || - tableDesc.Dropped() || !canSeeDescriptor { + if (!tableDesc.IsVirtualTable() && dbContext != nil && tableDesc.GetParentID() != dbContext.GetID()) || + tableDesc.Dropped() || + !canSeeDescriptor { return true, nil } h := makeOidHasher() @@ -5242,7 +5243,7 @@ func populateVirtualIndexForTable( // Ideally, the catalog API would be able to return the temporary // schemas from other sessions, but it cannot right now. See // https://github.com/cockroachdb/cockroach/issues/97822. - if err := forEachSchema(ctx, p, db, false /* requiresPrivileges*/, func(ctx context.Context, schema catalog.SchemaDescriptor) error { + if err := forEachSchema(ctx, p, dbContext, false /* requiresPrivileges*/, func(ctx context.Context, schema catalog.SchemaDescriptor) error { if schema.GetID() == tableDesc.GetParentSchemaID() { sc = schema } @@ -5257,6 +5258,13 @@ func populateVirtualIndexForTable( return false, err } } + db := dbContext + if db == nil { + db, err = p.Descriptors().ByIDWithLeased(p.txn).WithoutNonPublic().Get().Database(ctx, tableDesc.GetParentID()) + if err != nil { + return false, err + } + } if err := populateFromTable(ctx, p, h, db, sc, tableDesc, scResolver, addRow); err != nil { return false, err }