From: | Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c) |
Date: | 2024-05-23 11:54:12 |
Message-ID: | CAEudQArVc0PMAqFx+BefytGBMWVYp-PJhi3N2Cca3XHaJYNGVQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Em qui., 23 de mai. de 2024 às 06:27, Ashutosh Bapat <
ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> escreveu:
>
>
> On Thu, May 23, 2024 at 5:52 AM Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
>
>> On Wed, May 22, 2024 at 03:28:48PM -0300, Ranier Vilela wrote:
>> > 1. Another concern is the function *get_partition_ancestors*,
>> > which may return NIL, which may affect *llast_oid*, which does not
>> handle
>> > NIL entries.
>>
>> Hm? We already know in the code path that the relation we are dealing
>> with when calling get_partition_ancestors() *is* a partition thanks to
>> the check on relispartition, no? In this case, calling
>> get_partition_ancestors() is valid and there should be a top-most
>> parent in any case all the time. So I don't get the point of checking
>> get_partition_ancestors() for NIL-ness just for the sake of assuming
>> that it would be possible.
>>
>
> +1.
>
>
>>
>> > 2. Is checking *relispartition* enough?
>> > There a function *check_rel_can_be_partition*
>> > (src/backend/utils/adt/partitionfuncs.c),
>> > which performs a much more robust check, would it be worth using it?
>> >
>> > With the v2 attached, 1 is handled, but, in this case,
>> > will it be the most correct?
>>
>> Saying that, your point about the result of SearchSysCacheAttName not
>> checked if it is a valid tuple is right. We paint errors in these
>> cases even if they should not happen as that's useful when it comes to
>> debugging, at least.
>>
>
> I think an Assert would do instead of whole ereport().
>
IMO, Assert there is no better solution here.
> The callers have already resolved attribute name to attribute number.
> Hence the attribute *should* exist in both partition as well as topmost
> partitioned table.
>
> relid = llast_oid(ancestors);
> +
> ptup = SearchSysCacheAttName(relid, attname);
> + if (!HeapTupleIsValid(ptup))
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_COLUMN),
> + errmsg("column \"%s\" of relation \"%s\" does not exist",
> + attname, RelationGetRelationName(rel))));
>
> We changed the relid from OID of partition to that of topmost partitioned
> table but didn't change rel; which still points to partition relation. We
> have to invoke relation_open() with new relid, in order to use rel in the
> error message. I don't think all that is worth it, unless we find a
> scenario when SearchSysCacheAttName() returns NULL.
>
All calls to functions like SearchSysCacheAttName, in the whole codebase,
checks if returns are valid.
It must be for a very strong reason, such a style.
So, v3, implements it this way.
best regards,
Ranier Vilela
Attachment | Content-Type | Size |
---|---|---|
v3-fix-catalog-cache-search-check.patch | application/octet-stream | 1.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2024-05-23 12:00:13 | Re: PostgreSQL 17 Beta 1 release announcement draft |
Previous Message | Ranier Vilela | 2024-05-23 11:23:18 | Re: Avoid possible dereference null pointer (src/backend/catalog/pg_depend.c) |