public inbox for [email protected]help / color / mirror / Atom feed
CheckAttributeType() forgot to recurse into multiranges 4+ messages / 3 participants [nested] [flat]
* CheckAttributeType() forgot to recurse into multiranges @ 2026-04-22 20:56 Heikki Linnakangas <[email protected]> 0 siblings, 2 replies; 4+ messages in thread From: Heikki Linnakangas @ 2026-04-22 20:56 UTC (permalink / raw) To: pgsql-hackers Happened to spot this little bug: create type two_ints as (a int, b int); create type two_ints_range as range (subtype = two_ints); -- CheckAttributeType() forbids this: alter type two_ints add attribute c two_ints_range; ERROR: composite type two_ints cannot be made a member of itself -- But the same with a multirange is allowed: alter type two_ints add attribute c two_ints_multirange; ALTER TYPE That looks like a straightforward oversight in CheckAttributeType(). When multiranges were introduced, it didn't get the memo. Fix attached. Assuming no objections, I'll commit and backpatch that. While working on the fix, I noticed that in case of dropped columns, CheckAttributeType() is called with InvalidOid. It tolerates that, but it seems accidental and it performs a bunch of futile syscache lookups with InvalidOid, so it would be better to not do that. The second patch fixes that. - Heikki Attachments: [text/x-patch] 0001-Don-t-allow-composite-type-to-be-member-of-itself-vi.patch (4.9K, 2-0001-Don-t-allow-composite-type-to-be-member-of-itself-vi.patch) download | inline diff: From 50eecdd6477279785616bb4945b31b243265904e Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Wed, 22 Apr 2026 22:10:06 +0300 Subject: [PATCH 1/2] Don't allow composite type to be member of itself via multirange CheckAttributeType() checks that a composite type is not made a member of itself with ALTER TABLE ADD COLUMN or ALTER TYPE ADD ATTRIBUTE, even indirectly via a domain, array, another composite type or a range type. But it missed checking for multiranges. That was a simple oversight when multiranges were added. Discussion: xxx Backpatch-through: 14 --- src/backend/catalog/heap.c | 10 ++++++++++ src/test/regress/expected/alter_table.out | 9 +++++++++ src/test/regress/expected/rangetypes.out | 4 +++- src/test/regress/sql/alter_table.sql | 3 +++ src/test/regress/sql/rangetypes.sql | 3 ++- 5 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index ae6b7cda3dd..4a71f6a33b4 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -654,6 +654,16 @@ CheckAttributeType(const char *attname, containing_rowtypes, flags); } + else if (att_typtype == TYPTYPE_MULTIRANGE) + { + /* + * If it's a multirange, recurse to check its plain range type. + */ + CheckAttributeType(attname, get_multirange_range(atttypid), + get_range_collation(atttypid), + containing_rowtypes, + flags); + } else if (OidIsValid((att_typelem = get_element_type(atttypid)))) { /* diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index dad9d36937e..fc27e22023b 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2335,6 +2335,15 @@ ERROR: composite type recur1 cannot be made a member of itself create domain array_of_recur1 as recur1[]; alter table recur1 add column f2 array_of_recur1; -- fails ERROR: composite type recur1 cannot be made a member of itself +create type range_of_recur1 as range (subtype=recur1, multirange_type_name='multirange_of_recur1'); +NOTICE: function "range_of_recur1" will be effectively temporary +DETAIL: It depends on temporary type recur1. +NOTICE: function "range_of_recur1" will be effectively temporary +DETAIL: It depends on temporary type recur1. +alter table recur1 add column f2 range_of_recur1; -- fails +ERROR: composite type recur1 cannot be made a member of itself +alter table recur1 add column f2 multirange_of_recur1; -- fails +ERROR: composite type recur1 cannot be made a member of itself create temp table recur2 (f1 int, f2 recur1); alter table recur1 add column f2 recur2; -- fails ERROR: composite type recur1 cannot be made a member of itself diff --git a/src/test/regress/expected/rangetypes.out b/src/test/regress/expected/rangetypes.out index e062a4e5c2c..b2a753fd179 100644 --- a/src/test/regress/expected/rangetypes.out +++ b/src/test/regress/expected/rangetypes.out @@ -1810,9 +1810,11 @@ select *, row_to_json(upper(t)) as u from ["(5,6)","(7,8)") | {"a":7,"b":8} (2 rows) --- this must be rejected to avoid self-inclusion issues: +-- these must be rejected to avoid self-inclusion issues: alter type two_ints add attribute c two_ints_range; ERROR: composite type two_ints cannot be made a member of itself +alter type two_ints add attribute c two_ints_multirange; +ERROR: composite type two_ints cannot be made a member of itself drop type two_ints cascade; NOTICE: drop cascades to type two_ints_range -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index f5f13bbd3e7..6aaa33b6e34 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1535,6 +1535,9 @@ alter table recur1 add column f2 recur1; -- fails alter table recur1 add column f2 recur1[]; -- fails create domain array_of_recur1 as recur1[]; alter table recur1 add column f2 array_of_recur1; -- fails +create type range_of_recur1 as range (subtype=recur1, multirange_type_name='multirange_of_recur1'); +alter table recur1 add column f2 range_of_recur1; -- fails +alter table recur1 add column f2 multirange_of_recur1; -- fails create temp table recur2 (f1 int, f2 recur1); alter table recur1 add column f2 recur2; -- fails alter table recur1 add column f2 int; diff --git a/src/test/regress/sql/rangetypes.sql b/src/test/regress/sql/rangetypes.sql index 5c4b0337b7a..f0354736fe5 100644 --- a/src/test/regress/sql/rangetypes.sql +++ b/src/test/regress/sql/rangetypes.sql @@ -576,8 +576,9 @@ select *, row_to_json(upper(t)) as u from (values (two_ints_range(row(1,2), row(3,4))), (two_ints_range(row(5,6), row(7,8)))) v(t); --- this must be rejected to avoid self-inclusion issues: +-- these must be rejected to avoid self-inclusion issues: alter type two_ints add attribute c two_ints_range; +alter type two_ints add attribute c two_ints_multirange; drop type two_ints cascade; -- 2.47.3 [text/x-patch] 0002-Don-t-call-CheckAttributeType-with-InvalidOid-on-dro.patch (1.6K, 3-0002-Don-t-call-CheckAttributeType-with-InvalidOid-on-dro.patch) download | inline diff: From 17dc5a3887965e0b00704d15d4325474db47ad8d Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Wed, 22 Apr 2026 23:53:24 +0300 Subject: [PATCH 2/2] Don't call CheckAttributeType() with InvalidOid on dropped cols If CheckAttributeType() is called with InvalidOid, it performs a bunch of pointless, futile syscache lookups with InvalidOid, but ultimately tolerates it and has no effect. We were calling it with InvalidOid on dropped columns, but it seems accidental that it works, so let's stop doing it. Discussion: xxx Backpatch-through: 14 --- src/backend/catalog/heap.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 4a71f6a33b4..b3bc454bd48 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -504,11 +504,15 @@ CheckAttributeNamesTypes(TupleDesc tupdesc, char relkind, */ for (i = 0; i < natts; i++) { - CheckAttributeType(NameStr(TupleDescAttr(tupdesc, i)->attname), - TupleDescAttr(tupdesc, i)->atttypid, - TupleDescAttr(tupdesc, i)->attcollation, + Form_pg_attribute attr = TupleDescAttr(tupdesc, i); + + if (attr->attisdropped) + continue; + CheckAttributeType(NameStr(attr->attname), + attr->atttypid, + attr->attcollation, NIL, /* assume we're creating a new rowtype */ - flags | (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL ? CHKATYPE_IS_VIRTUAL : 0)); + flags | (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL ? CHKATYPE_IS_VIRTUAL : 0)); } } -- 2.47.3 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: CheckAttributeType() forgot to recurse into multiranges @ 2026-04-23 03:24 Chao Li <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 4+ messages in thread From: Chao Li @ 2026-04-23 03:24 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: pgsql-hackers > On Apr 23, 2026, at 04:56, Heikki Linnakangas <[email protected]> wrote: > > Happened to spot this little bug: > > create type two_ints as (a int, b int); > create type two_ints_range as range (subtype = two_ints); > > -- CheckAttributeType() forbids this: > alter type two_ints add attribute c two_ints_range; > ERROR: composite type two_ints cannot be made a member of itself > > -- But the same with a multirange is allowed: > alter type two_ints add attribute c two_ints_multirange; > ALTER TYPE > > That looks like a straightforward oversight in CheckAttributeType(). When multiranges were introduced, it didn't get the memo. > > Fix attached. Assuming no objections, I'll commit and backpatch that. > > While working on the fix, I noticed that in case of dropped columns, CheckAttributeType() is called with InvalidOid. It tolerates that, but it seems accidental and it performs a bunch of futile syscache lookups with InvalidOid, so it would be better to not do that. The second patch fixes that. > > - Heikki > <0001-Don-t-allow-composite-type-to-be-member-of-itself-vi.patch><0002-Don-t-call-CheckAttributeType-with-InvalidOid-on-dro.patch> I traced this patch set, 0002 looks good, but I have a suspicion about 0001. ``` + else if (att_typtype == TYPTYPE_MULTIRANGE) + { + /* + * If it's a multirange, recurse to check its plain range type. + */ + CheckAttributeType(attname, get_multirange_range(atttypid), + get_range_collation(atttypid), + containing_rowtypes, + flags); + } ``` Looking at get_range_collation(), it only searches for RANGETYPE, so get_range_collation(atttypid) here will always return InvalidOid. This does not seem to cause a problem, because CheckAttributeType() will recurse into the TYPTYPE_RANGE path, and the collation will be evaluated there. But to make the logic clearer, I think we could just pass InvalidOid as the collation OID in the TYPTYPE_MULTIRANGE case. If we really want to pass the actual collation OID here, I think it would need to be done more like this: ``` else if (att_typtype == TYPTYPE_MULTIRANGE) { Oid multirange_range_typid = get_multirange_range(atttypid); Oid collation = get_range_collation(multirange_range_typid); /* * If it's a multirange, recurse to check its plain range type. */ CheckAttributeType(attname, multirange_range_typid, collation, containing_rowtypes, flags); } ``` Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: CheckAttributeType() forgot to recurse into multiranges @ 2026-04-23 07:22 Michael Paquier <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Michael Paquier @ 2026-04-23 07:22 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: pgsql-hackers On Wed, Apr 22, 2026 at 11:56:12PM +0300, Heikki Linnakangas wrote: > That looks like a straightforward oversight in CheckAttributeType(). When > multiranges were introduced, it didn't get the memo. Nice catch. --- this must be rejected to avoid self-inclusion issues: +-- these must be rejected to avoid self-inclusion issues: alter type two_ints add attribute c two_ints_range; ERROR: composite type two_ints cannot be made a member of itself +alter type two_ints add attribute c two_ints_multirange; +ERROR: composite type two_ints cannot be made a member of itself If you want to create a parallel with multirangetypes.sql, this choking case may be better if placed there rather than rangetypes.sql, as it is a multi case. Not a big deal, still. > While working on the fix, I noticed that in case of dropped columns, > CheckAttributeType() is called with InvalidOid. It tolerates that, but it > seems accidental and it performs a bunch of futile syscache lookups with > InvalidOid, so it would be better to not do that. The second patch fixes > that. This one seems harmless as far as I can see, but we should be careful to bypass any attisdropped while scanning a set of attributes, so a backpatch is in order, indeed. LGTM. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, 2-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: CheckAttributeType() forgot to recurse into multiranges @ 2026-04-23 18:49 Heikki Linnakangas <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Heikki Linnakangas @ 2026-04-23 18:49 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: pgsql-hackers On 23/04/2026 06:24, Chao Li wrote: > I traced this patch set, 0002 looks good, but I have a suspicion about 0001. > > ``` > + else if (att_typtype == TYPTYPE_MULTIRANGE) > + { > + /* > + * If it's a multirange, recurse to check its plain range type. > + */ > + CheckAttributeType(attname, get_multirange_range(atttypid), > + get_range_collation(atttypid), > + containing_rowtypes, > + flags); > + } > ``` > > Looking at get_range_collation(), it only searches for RANGETYPE, so get_range_collation(atttypid) here will always return InvalidOid. This does not seem to cause a problem, because CheckAttributeType() will recurse into the TYPTYPE_RANGE path, and the collation will be evaluated there. > > But to make the logic clearer, I think we could just pass InvalidOid as the collation OID in the TYPTYPE_MULTIRANGE case. If we really want to pass the actual collation OID here, I think it would need to be done more like this: > > ``` > else if (att_typtype == TYPTYPE_MULTIRANGE) > { > Oid multirange_range_typid = get_multirange_range(atttypid); > Oid collation = get_range_collation(multirange_range_typid); > /* > * If it's a multirange, recurse to check its plain range type. > */ > CheckAttributeType(attname, multirange_range_typid, > collation, > containing_rowtypes, > flags); > } > ``` Oh, good catch, this is subtle. I'll change it to InvalidOid. As long as range types are not collatable, that'll do the right thing. If range types were collatable, I'm not sure what the right thing would be. It depends on what exactly the collation of a range type would mean. So, pushed with InvalidOid. Thanks for the review! - Heikki ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-04-23 18:49 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-04-22 20:56 CheckAttributeType() forgot to recurse into multiranges Heikki Linnakangas <[email protected]> 2026-04-23 03:24 ` Chao Li <[email protected]> 2026-04-23 18:49 ` Heikki Linnakangas <[email protected]> 2026-04-23 07:22 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox