Skip to content

Commit

Permalink
opt: track UDT references by name in the Metadata
Browse files Browse the repository at this point in the history
Previously, it was possible for invalid queries to be kept in the cache
after a schema change, since user-defined types were tracked only by OID.
This missed cases where the search path changed which object a name in
the query resolved to (or whether it resolved at all).

This patch fixes this behavior by tracking UDT references by name, similar
to what was already done for data sources. This ensures that the query
staleness check doesn't miss changes to the search path.

Fixes cockroachdb#96674

Release note (bug fix): Fixed a bug that could prevent a cached query with
a user-defined type reference from being invalidated even after a schema
change that should prevent the type from being resolved.
  • Loading branch information
DrewKimball committed Mar 10, 2023
1 parent f8ccab3 commit 1886dd9
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 9 deletions.
75 changes: 75 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/schema
Original file line number Diff line number Diff line change
Expand Up @@ -989,3 +989,78 @@ statement ok
USE test;
DROP DATABASE d;
DROP TABLE xy;

# Regression tests for #96674.
subtest alter_udt_schema

# Renaming the schema should invalidate a schema-qualified UDT reference.
statement ok
CREATE SCHEMA sc;
CREATE TYPE sc.t AS ENUM ('HELLO');

query T
SELECT 'HELLO'::sc.t;
----
HELLO

statement ok
ALTER SCHEMA sc RENAME TO sc1;

query error pq: type "sc.t" does not exist
SELECT 'HELLO'::sc.t;

query T
SELECT 'HELLO'::sc1.t;
----
HELLO

statement ok
DROP SCHEMA sc1 CASCADE;

# Renaming the database should invalidate a database-qualified UDT reference.
statement ok
CREATE DATABASE d;
USE d;
CREATE TYPE d.t AS ENUM ('HELLO');

query T
SELECT 'HELLO'::d.t;
----
HELLO

statement ok
ALTER DATABASE d RENAME TO d1;
USE d1;

query error pq: type "d.t" does not exist
SELECT 'HELLO'::d.t;

query T
SELECT 'HELLO'::d1.t;
----
HELLO

statement ok
USE test;
DROP DATABASE d1 CASCADE;

# Changing the current database should invalidate an unqualified UDT reference.
statement ok
CREATE TYPE t AS ENUM ('HELLO');

query T
SELECT 'HELLO'::t;
----
HELLO

statement ok
CREATE DATABASE d;
USE d;

query error pq: type "t" does not exist
SELECT 'HELLO'::t;

statement ok
USE test;
DROP DATABASE d;
DROP TYPE t;
1 change: 1 addition & 0 deletions pkg/sql/opt/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ go_library(
"//pkg/sql/pgwire/pgcode",
"//pkg/sql/pgwire/pgerror",
"//pkg/sql/privilege",
"//pkg/sql/sem/catid",
"//pkg/sql/sem/eval",
"//pkg/sql/sem/tree",
"//pkg/sql/sem/tree/treebin",
Expand Down
27 changes: 22 additions & 5 deletions pkg/sql/opt/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/types"
Expand Down Expand Up @@ -355,11 +356,22 @@ func (md *Metadata) CheckDependencies(
}
}

// Check that all of the user defined types present have not changed.
// Check that no referenced user defined types have changed.
for _, typ := range md.AllUserDefinedTypes() {
toCheck, err := optCatalog.ResolveTypeByOID(ctx, typ.Oid())
if err != nil || typ.TypeMeta.Version != toCheck.TypeMeta.Version {
return false, maybeSwallowMetadataResolveErr(err)
id := cat.StableID(catid.UserDefinedOIDToID(typ.Oid()))
if names, ok := md.objectRefsByName[id]; ok {
for _, name := range names {
toCheck, err := optCatalog.ResolveType(ctx, name)
if err != nil || typ.Oid() != toCheck.Oid() ||
typ.TypeMeta.Version != toCheck.TypeMeta.Version {
return false, maybeSwallowMetadataResolveErr(err)
}
}
} else {
toCheck, err := optCatalog.ResolveTypeByOID(ctx, typ.Oid())
if err != nil || typ.TypeMeta.Version != toCheck.TypeMeta.Version {
return false, maybeSwallowMetadataResolveErr(err)
}
}
}

Expand Down Expand Up @@ -425,7 +437,8 @@ func (md *Metadata) Schema(schID SchemaID) cat.Schema {
}

// AddUserDefinedType adds a user defined type to the metadata for this query.
func (md *Metadata) AddUserDefinedType(typ *types.T) {
// If the type was resolved by name, the name will be tracked as well.
func (md *Metadata) AddUserDefinedType(typ *types.T, name *tree.UnresolvedObjectName) {
if !typ.UserDefined() {
return
}
Expand All @@ -436,6 +449,10 @@ func (md *Metadata) AddUserDefinedType(typ *types.T) {
md.userDefinedTypes[typ.Oid()] = struct{}{}
md.userDefinedTypesSlice = append(md.userDefinedTypesSlice, typ)
}
if name != nil {
id := cat.StableID(catid.UserDefinedOIDToID(typ.Oid()))
md.objectRefsByName[id] = append(md.objectRefsByName[id], name)
}
}

// AllUserDefinedTypes returns all user defined types contained in this query.
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestMetadata(t *testing.T) {
tabID := md.AddTable(&testcat.Table{}, &tree.TableName{})
seqID := md.AddSequence(&testcat.Sequence{})
md.AddView(&testcat.View{})
md.AddUserDefinedType(types.MakeEnum(152100, 154180))
md.AddUserDefinedType(types.MakeEnum(152100, 154180), nil /* name */)

// Call Init and add objects from catalog, verifying that IDs have been reset.
testCat := testcat.New()
Expand Down Expand Up @@ -92,7 +92,7 @@ func TestMetadata(t *testing.T) {
t.Fatalf("unexpected views")
}

md.AddUserDefinedType(types.MakeEnum(151500, 152510))
md.AddUserDefinedType(types.MakeEnum(151500, 152510), nil /* name */)
if len(md.AllUserDefinedTypes()) != 1 {
fmt.Println(md)
t.Fatalf("unexpected types")
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/optbuilder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ func (o *optTrackingTypeResolver) ResolveType(
if err != nil {
return nil, err
}
o.metadata.AddUserDefinedType(typ)
o.metadata.AddUserDefinedType(typ, name)
return typ, nil
}

Expand All @@ -518,6 +518,6 @@ func (o *optTrackingTypeResolver) ResolveTypeByOID(
if err != nil {
return nil, err
}
o.metadata.AddUserDefinedType(typ)
o.metadata.AddUserDefinedType(typ, nil /* name */)
return typ, nil
}

0 comments on commit 1886dd9

Please sign in to comment.