On Thu, May 23, 2024 at 5:52 AM Michael Paquier <[email protected]> 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(). 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.
--
Best Wishes,
Ashutosh Bapat
