sql: add composite types to pg_class and pg_attribute #111179
Conversation
|
This change is |
annrpom
changed the title
annrpom
force-pushed
the
populate-comp-typ
branch
from
934d90f
to
945b657
Compare
annrpom
changed the title
annrpom
force-pushed
the
populate-comp-typ
branch
3 times, most recently
from
cc45867
to
86c3224
Compare
annrpom
changed the title
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @annrpom)
pkg/sql/pg_catalog.go line 1248 at r1 (raw file):
// because we know the underlying descriptor will generate no rows, since // the ID being queried is *not* a table. table, ok := desc.(catalog.TableDescriptor)
we'll also need to update this part - it should also check if the descriptor is a type descriptor. this logic is used when querying the table by ID -- doing that makes the execution engine use the "virtual index" that is defined on the table, rather than using the populateAll function defined above. the reason it exists is because it lets us avoid fetching all the descriptors if we are only querying the table for one of them.
to test it out, could you add a few tests that are like select <cols> from pg_class where oid = 'typ_name'::regtype::oid (where typ_name is a composite type)? and similar for pg_attribute
annrpom
force-pushed
the
populate-comp-typ
branch
2 times, most recently
from
4b7230c
to
f468116
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pg_catalog.go line 1175 at r2 (raw file):
) error, ) virtualSchemaTable { isIncludingCompositeTypes := populateFromType != nil
super nit: includesCompositeTypes might be a more clear name
pkg/sql/pg_catalog.go line 1251 at r2 (raw file):
desc, err = p.byIDGetterBuilder().WithoutNonPublic().Get().Desc(ctx, id) if err != nil { // This is under the assumption that the OIDs for user-defined "things" are not hashed.
hm, i think this has one more case that's missing -- composite types also get a matching array type, and that has a different OID. handling that here might get messy.
maybe it would be better to keep the old logic after all, so we don't do a second lookup. if we keep the old return !IsMaybeHashedOid(oid.Oid(id)), nil line here, it should cause the index to fallback to populating the whole table
that would also mean we should change the definition of incomplete to
incomplete: includesIndexEntries || includesCompositeTypes,
pkg/sql/pg_catalog.go line 1270 at r2 (raw file):
var tableDesc catalog.TableDescriptor var typeDesc catalog.TypeDescriptor switch desc.DescriptorType() {
nit: you can use a switch type assertion to make this more concise. e.g.
switch d := desc.(type) {
case catalog.TableDescriptor:
// now you can use `d`, and it will be of the catalog.TableDescriptor type.
case catalog.TypeDescriptor:
// now you can use `d`, and it will be of the catalog.TypeDescriptor type.
default:
return true, nil
}
pkg/sql/logictest/testdata/logic_test/pg_catalog line 2010 at r2 (raw file):
2210 _regclass A false true , 0 2205 0 2211 _regtype A false true , 0 2206 0 2249 record P false true , 2249 0 2287
nit: since this is for the "default" record type, this one should have an oid of zero still. looks like that's what PG has too
pkg/sql/logictest/testdata/logic_test/pg_catalog line 5032 at r2 (raw file):
pg_catalog.pg_class WHERE oid = 'u'::regtype::oid;
could you add one more test for looking up the array type. that is WHERE oid = 'u[]'::regtype::oid;
i think the index lookup won't work, but it should still fallback to populating the full table
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pg_catalog.go line 1175 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
super nit:
includesCompositeTypesmight be a more clear name
sgtm
pkg/sql/pg_catalog.go line 1251 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
hm, i think this has one more case that's missing -- composite types also get a matching array type, and that has a different OID. handling that here might get messy.
maybe it would be better to keep the old logic after all, so we don't do a second lookup. if we keep the old
return !IsMaybeHashedOid(oid.Oid(id)), nilline here, it should cause the index to fallback to populating the whole tablethat would also mean we should change the definition of
incompletetoincomplete: includesIndexEntries || includesCompositeTypes,
(related to the comment below about the test) -> we shouldn't need to worry about the array type, i think
pkg/sql/pg_catalog.go line 1270 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: you can use a switch type assertion to make this more concise. e.g.
switch d := desc.(type) { case catalog.TableDescriptor: // now you can use `d`, and it will be of the catalog.TableDescriptor type. case catalog.TypeDescriptor: // now you can use `d`, and it will be of the catalog.TypeDescriptor type. default: return true, nil }
💯 I forgot my basic coding knowledge for a sec with this one
pkg/sql/logictest/testdata/logic_test/pg_catalog line 2010 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: since this is for the "default" record type, this one should have an oid of zero still. looks like that's what PG has too
ah oversight on my end - ty for catching this
pkg/sql/logictest/testdata/logic_test/pg_catalog line 5032 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
could you add one more test for looking up the array type. that is
WHERE oid = 'u[]'::regtype::oid;i think the index lookup won't work, but it should still fallback to populating the full table
apologies - I could just not be understanding something here, but it seems like the pg_class table does not have context of the array type generated by the composite type (since it still is an array type after all). same with pg_attribute, but i was able to add a test for pg_type
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pg_catalog.go line 1251 at r2 (raw file):
Previously, annrpom (annie pompa) wrote…
(related to the comment below about the test) -> we shouldn't need to worry about the array type, i think
ah nice! great
pkg/sql/pg_catalog.go line 5115 at r2 (raw file):
} func populateVirtualIndexForTable(
nit: these two new functions could use a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/pg_catalog line 5032 at r2 (raw file):
Previously, annrpom (annie pompa) wrote…
apologies - I could just not be understanding something here, but it seems like the pg_class table does not have context of the array type generated by the composite type (since it still is an array type after all). same with pg_attribute, but i was able to add a test for pg_type
nice! looks like you are right, so we can keep this simple
annrpom
force-pushed
the
populate-comp-typ
branch
from
f468116
to
1f7f03b
Compare
There was a problem hiding this comment.
this looks good! just had one more optional comment to consider, then feel free to merge
Reviewable status:
pkg/sql/pg_catalog.go line 1275 at r3 (raw file):
return populateVirtualIndexForTable(ctx, p, db, d, addRow, populateFromTable) case catalog.TypeDescriptor: if !includesCompositeTypes || d.GetKind() != descpb.TypeDescriptor_COMPOSITE {
should it be up to the populateFromType function to decide if non-composite types should be filtered?
that's what the populateAll function indicates above:
// Generate rows for some/all user defined types (depending on function populateFromType).
Previously, user-defined composite types were not populated in 2 of `pg_catalog`'s tables: `pg_class` and `pg_attribute` (where the former's row entries pertain to the type and the latter's pertain to the "columns" of the type). This patch addresses this PostgreSQL-incompatible behavior by populating such tables with user-defined composite types. This patch also ensures that the `typrelid` column in the `pg_type` table has the proper oid for composite types. Epic: none Fixes: cockroachdb#109675 Release note (sql change): `pg_catalog`'s `pg_class` and `pg_attribute` tables now have context of user-defined composite types (in accordance with PostgreSQL's corresponding `pg_catalog` tables). In addition, the `typrelid` column in the `pg_type` table has the proper oid for composite types.
annrpom
force-pushed
the
populate-comp-typ
branch
from
1f7f03b
to
0d79e91
Compare
|
you make a good point! TFTR! ('-')7 bors r=rafiss |
|
Build succeeded: |
Previously, user-defined composite types were not populated
in 2 of
pg_catalog's tables:pg_classandpg_attribute(where the former's row entries pertain to the type and the latter's
pertain to the "columns" of the type). This patch addresses this
PostgreSQL-incompatible behavior by populating such tables with
user-defined composite types.
Epic: none
Fixes: #109675
Release note (sql change):
pg_catalog'spg_classandpg_attributetables now have context of user-defined composite types (in accordance
with PostgreSQL's corresponding
pg_catalogtables).