Re: Remove no-op PlaceHolderVars

On Tue, Sep 3, 2024 at 11:31 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Yeah. I've been mulling over how we could do this, and the real
> problem is that the expression containing the PHV *has* been fully
> preprocessed by the time we get to outer join strength reduction
> (cf. file header comment in prepjointree.c). Simply dropping the PHV
> can break various invariants that expression preprocessing is supposed
> to establish, such as "no RelabelType directly above another" or "no
> AND clause directly above another".

Yeah, this is what the problem is.

> I haven't thought of a reliable
> way to fix that short of re-running eval_const_expressions afterwards,
> which seems like a mighty expensive answer.

This seems likely to result in a lot of duplicate work.

> We could try to make
> remove_nulling_relids_mutator preserve all these invariants, but
> keeping it in sync with what eval_const_expressions does seems like
> a maintenance nightmare.

Yeah, and it seems that we also need to know the EXPRKIND code for the
expression containing the to-be-dropped PHV in remove_nulling_relids
to know how we should preserve all these invariants, which seems to
require a lot of code changes.

Looking at the routines run in preprocess_expression, most of them
recurse into PlaceHolderVars and preprocess the contained expressions.
The two exceptions are canonicalize_qual and make_ands_implicit. I
wonder if we can modify them to also recurse into PlaceHolderVars.
Will this resolve our problem here?

> I believe this is an area worth working on. I've been wondering
> whether it'd be better to handle the expression-identity problems by
> introducing an "expression wrapper" node type that is distinct from
> PHV and has the sole purpose of demarcating a subexpression we don't
> want to be folded into its parent. I think that doesn't really move
> the football in terms of fixing the problems mentioned above, but
> perhaps it's conceptually cleaner than adding a bool field to PHV.
>
> Another thought is that grouping sets provide one of the big reasons
> why we need an "expression wrapper" or equivalent functionality.
> So maybe we should try to move forward on your other patch to change
> the representation of those before we spend too much effort here.

Hmm, in the case of grouping sets, we have a similar situation where
we do not want a subexpression that is part of grouping items to be
folded into its parent, because otherwise we may fail to match these
subexpressions to lower target items.

For grouping sets, this problem is much easier to resolve, because
we've already replaced such subexpressions with Vars referencing the
GROUP RTE. As a result, during expression preprocessing, these
subexpressions will not be folded into their parents.

An ensuing effect of this approach is that a HAVING clause may contain
expressions that are not fully preprocessed if they are part of
grouping items. This is not an issue as long as the clause remains in
HAVING, because these expressions will be matched to lower target
items in setrefs.c. However, if the clause is moved or copied into
WHERE, we need to re-preprocess these expressions. This should not be
too expensive, as we only need to re-preprocess the HAVING clauses
that are moved to WHERE, not the entire tree. The v13 patch in that
thread implements this approach.

Thanks
Richard