On Fri, 8 Mar 2024 at 23:14, Richard Guo <guofenglinux@gmail.com> wrote:
> I've looked at this patch a bit. I once wondered why we don't check
> pathkey->pk_eclass->ec_has_const with EC_MUST_BE_REDUNDANT to see if the
> pathkey is not needed. Then I realized that a child member would not be
> marked as constant even if the child expr is a Const, as explained in
> add_child_rel_equivalences().
This situation where the child member is a Const but the parent isn't
is unique to UNION ALL queries. The only other cases where we have
child members are with partitioned and inheritance tables. In those
cases, the parent member just maps to the equivalent child member
replacing parent Vars with the corresponding child Var according to
the column mapping between the parent and child. It might be nice if
partitioning supported mapping to a Const as in many cases that could
save storing the same value in the table every time, but we don't
support that, so this can only happen with UNION ALL queries.
> BTW, I wonder if it is possible that we have a pseudoconstant expression
> that is not of type Const. In such cases we would need to check
> 'bms_is_empty(pull_varnos(em_expr))' instead of 'IsA(em_expr, Const)'.
> However, I'm unable to think of such an expression in this context.
I can't see how there'd be any problems with a misinterpretation of a
pseudoconstant value as an ordinal column position on the remote
server. Surely it's only actual "Const" node types that we're just
going to call the type's output function which risks it yielding a
string of digits and the remote server thinking that we must mean an
ordinal column position.
> The patch looks good to me otherwise.
Thanks
David