On Thu, May 23, 2024 at 5:52 AM Michael Paquier <michael@paquier.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.
