Skip to content

Commit

Permalink
Merge cockroachdb#126409
Browse files Browse the repository at this point in the history 
126409: sql: fix nil pointer caused by "".crdb_internal.create_statements r=rafiss a=rafiss

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.

Epic: None
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
  • Loading branch information
@rafiss
craig[bot] and rafiss committed Jun 28, 2024
2 parents 3f43958 + a5e95a2 commit 5a294d4
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 12 deletions.
38 changes: 31 additions & 7 deletions pkg/sql/logictest/testdata/logic_test/crdb_internal
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
----
Expand Down Expand Up @@ -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';
Expand All @@ -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;
Expand Down
18 changes: 13 additions & 5 deletions pkg/sql/pg_catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -5222,16 +5222,17 @@ 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
}
// Skip over tables from a different DB, ones which aren't visible
// 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()
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down

0 comments on commit 5a294d4

Please sign in to comment.