Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Don't allow composite type to be member of itself via multirange

  2. Don't call CheckAttributeType() with InvalidOid on dropped cols

  1. CheckAttributeType() forgot to recurse into multiranges

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-04-22T20:56:12Z

    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

  2. Re: CheckAttributeType() forgot to recurse into multiranges

    Chao Li <li.evan.chao@gmail.com> — 2026-04-23T03:24:26Z

    
> On Apr 23, 2026, at 04:56, Heikki Linnakangas <hlinnaka@iki.fi> 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/

  3. Re: CheckAttributeType() forgot to recurse into multiranges

    Michael Paquier <michael@paquier.xyz> — 2026-04-23T07:22:15Z

    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

  4. Re: CheckAttributeType() forgot to recurse into multiranges

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-04-23T18:49:04Z

    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