Re: Significant Execution Time Difference Between PG13.14 and PG16.4 for Query on information_schema Tables. - Mailing list pgsql-hackers

[ switching to -hackers list ]

David Rowley <dgrowleyml@gmail.com> writes:
> In case it saves you a bit of time, I stripped as much of the
> unrelated stuff out as I could and got:

> create table t (a name, b int);
> explain select * from (select a::varchar,b from (select distinct a,b
> from t) st) t right join t t2 on t.b=t2.b where t.a='test';

> getting rid of the cast or swapping to INNER JOIN rather than RIGHT
> JOIN means that qual_is_pushdown_safe() gets a Var rather than a
> PlaceHolderVar.

Thanks.  So it seems that what's happening is that we stick a
PlaceHolderVar on the intermediate subquery's output ("a::varchar"),
and then later when we realize that the RIGHT JOIN can be reduced to
an inner join we run around and remove the right join from the
PlaceHolderVar's nullingrels, leaving a useless PHV with no
nullingrels.  remove_nulling_relids explains

             * Note: it might seem desirable to remove the PHV altogether if
             * phnullingrels goes to empty.  Currently we dare not do that
             * because we use PHVs in some cases to enforce separate identity
             * of subexpressions; see wrap_non_vars usages in prepjointree.c.

However, then when we consider whether the upper WHERE condition
can be pushed down into the unflattened lower subquery,
qual_is_pushdown_safe punts:

         * XXX Punt if we find any PlaceHolderVars in the restriction clause.
         * It's not clear whether a PHV could safely be pushed down, and even
         * less clear whether such a situation could arise in any cases of
         * practical interest anyway.  So for the moment, just refuse to push
         * down.

We didn't see this particular behavior before 2489d76c49 because
pullup_replace_vars avoided inserting a PHV:

                 * If it contains a Var of the subquery being pulled up, and
                 * does not contain any non-strict constructs, then it's
                 * certainly nullable so we don't need to insert a
                 * PlaceHolderVar.

I dropped that case in 2489d76c49 because now we need to attach
nullingrels to the expression.  You could imagine attaching the
nullingrels to the contained Var(s) instead of putting a PHV on top,
but that seems like a mess and I'm not quite sure it's semantically
the same.  In any case it wouldn't fix adjacent cases where there is
a non-strict construct in the subquery output expression.

So it seems like we need to fix one or the other of these
implementation shortcuts to restore the previous behavior.
I'm wondering if it'd be okay for qual_is_pushdown_safe to accept
PHVs that have no nullingrels.  I'm not really thrilled about trying
to back-patch any such fix though --- the odds of introducing new bugs
seem nontrivial, and the problem case seems rather narrow.  If we
are willing to accept a HEAD-only fix, it'd likely be better to
attack the other end and make it possible to remove no-op PHVs.
I think that'd require marking PHVs that need to be kept because
they are serving to isolate subexpressions.

            regards, tom lane