>On Tue, Oct 28, 2025 at 11:24?PM Kirill Reshke <[email protected]> wrote:
>>
>> I can see that 0 - FirstLowInvalidHeapAttributeNumber is an existing
>> coding practise in sources, but so is
>> InvalidAttrNumber - FirstLowInvalidHeapAttributeNumber (sepgsql
>> contrib extension). I'm voting for the second one, even though it is
>> less used.
>>
>
>hi.
>
>now it's:
>+ /*
>+ * If the partition expression contains a whole-row reference,
>+ * then all columns are indirectly associated with that
>+ * expression.
>+ */
>+ if (bms_is_member(InvalidAttrNumber -
>FirstLowInvalidHeapAttributeNumber,
>+ expr_attrs))
>+ {
>+ if (used_in_expr)
>+ *used_in_expr = true;
>+ return true;
>+ }
>
>also polished the commit message. below is the commit message:
>--------------------------
>For partition key expressions containing whole-row reference, we cannot rely on
>pg_depend lookups to determine whether an individual column can be altered
>(drop, set data type).
>As noted in the comments for find_expr_references_walker, no dependency entries
>are recorded for whole-row expressions. Therefore whole-row reference check is
>needed in has_partition_attrs.
>
>Partition key expressions contain whole-row reference, it is effectively
>equivalent to all user columns being indirectly associated with that
>expression.
>Since columns that in a partition key cannot be altered in any way, the same
>restriction should be applied to whole-row reference expressions in the
>partition key.
>--------------------------
>
>
>--
>jian
>https://www.enterprisedb.com/
Hi all,
Thanks for this patch and the surrounding discussions.
I have several points to share for review consideration:
1.Regarding the varattno value used to identify whole-row Vars: I tend to think
InvalidAttrNumber would be a better choice than 0, which aligns with the
existing
implementation of makeWholeRowVar().
2.While reviewing this patch, I took a close look at makeWholeRowVar(). Most
whole-row Vars
constructed by this function use InvalidAttrNumber for varattno, yet there
exists one edge case
where varattno = 1 is used. Given that partition expressions can be highly
complex with multi-level nesting,
apart from RTE_RELATION, I am not certain which other RTE types could generate
genuine whole-row
references covering all columns of a relation.
I have attempted to reproduce various ways of constructing whole-row Vars to
perform negative testing and
verify the robustness of this patch. Although I tried combinations such as
custom composite types and
user-defined functions, I only successfully reproduced the scenario for
RTE_RELATION so far.
3.This leads to my concern: if we only check for varattno = InvalidAttrNumber
in the newly added logic,
can we reliably capture all legitimate whole-row references that refer to every
column of a target relation?
There is a risk that this check might be too broad and introduce unnecessary
over-restrictions.
4.If the grammar and validation rules for PARTITION BY during table creation
strictly restrict whole-row Vars
to only RTE_RELATION type, then this patch is reasonable. As commented in
ComputePartitionAttrs():
/*
* transformPartitionSpec() should have already
rejected
* subqueries, aggregates, window functions,
and SRFs, based
* on the EXPR_KIND_ for partition expressions.
*/
5.The newly modified function has_partition_attrs() processes each partition
key element following largely
the same logic as RelationBuildPartitionKey() and ComputePartitionAttrs(). Our
handling of whole-row Vars
in this patch is consistent with the logic inside ComputePartitionAttrs() for
expression branches. The only
difference is that the existing code uses 0 for whole-row detection, while this
patch switches to InvalidAttrNumber.
Effectively, both varattno = InvalidAttrNumber and varattno = 0 denote
whole-row references to the target relation.
6.To more accurately identify genuine whole-row Vars belonging to the
partitioned table itself and avoid misjudgments
from other types of whole-row references or future new whole-row
implementations, I would like to propose a stricter
matching rule:
We can collect all Var nodes from the partition expression and validate them
with the following constraints to precisely
target the whole-row references we intend to guard:
```c
Var->vartype == Relation->reltype (pg_class.reltype) && Var->varno == 1 &&
Var->varlevelsup == 0
```c
These are my personal thoughts; please feel free to correct any
misunderstandings or further discuss these points.
regards,
--
ZizhuanLiu (X-MAN)
[email protected]
