Re: Remove no-op PlaceHolderVars

On Tue, Sep 3, 2024 at 5:50 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> 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.

While fixing some performance issues caused by PHVs recently, it
struck me again that we should be removing "no-op" PHVs whenever
possible, because PHVs can be optimization barriers in several cases.

I still do not have a good idea for ensuring that removing the PHV
wrapper does not break the expression tree invariants. But maybe we
can use a conservative approach: we only strip the PHV if the
contained expression is known to be safe (for example, a simple Var).

I've also realized that we cannot remove no-op PHVs in every case.
For example, deconstruct_distribute_oj_quals() relies on
remove_nulling_relids() to temporarily strip out the nullingrels bits
that are later restored as we crawl up the join stack. If the PHV
were removed, we would be unable to restore its nullingrels bits for
next level up. To handle this, I added a new parameter to
remove_nulling_relids() so the caller can decide whether to allow
removal or not.

Attached is a draft patch for the code changes. It currently causes
plan changes in 28 regression test queries, which is a surprisingly
high number. We'll have to go through these tests one by one, but
before doing that, I would like to hear others' thoughts on this
patch.

- Richard