Thread: pgsql: Remove over-optimistic Assert.

Remove over-optimistic Assert.

In commit 2489d76c4, I'd thought it'd be safe to assert that a
PlaceHolderVar appearing in a scan-level expression has empty
nullingrels.  However this is not so, as when we determine that a
join relation is certainly empty we'll put its targetlist into a
Result-with-constant-false-qual node, and nothing is done to adjust
the nullingrels of the Vars or PHVs therein.  (Arguably, a Result
used in this way isn't really a scan-level node, but it certainly
isn't an upper node either ...)

It's not clear this is worth any close analysis, so let's just
take out the faulty Assert.

Per report from Robins Tharakan.  I added a test case based on
his example, just in case somebody tries to tighten this up.

Discussion: https://postgr.es/m/CAEP4nAz7Enq3+DEthGG7j27DpuwSRZnW0Nh6jtNh75yErQ_nbA@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/eae0e20deffb0a73f7cb0e94746f94a1347e71b1

Modified Files
--------------
src/backend/optimizer/plan/setrefs.c |  2 +-
src/test/regress/expected/join.out   | 14 ++++++++++++++
src/test/regress/sql/join.sql        |  8 ++++++++
3 files changed, 23 insertions(+), 1 deletion(-)

Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In commit 2489d76c4, I'd thought it'd be safe to assert that a
>> PlaceHolderVar appearing in a scan-level expression has empty
>> nullingrels.  However this is not so, as when we determine that a
>> join relation is certainly empty we'll put its targetlist into a
>> Result-with-constant-false-qual node, and nothing is done to adjust
>> the nullingrels of the Vars or PHVs therein.  (Arguably, a Result
>> used in this way isn't really a scan-level node, but it certainly
>> isn't an upper node either ...)

> It seems this is the only case we can have PlaceHolderVar with non-empty
> nullingrels at scan level.  So I wonder if we can manually adjust the
> nullingrels of PHVs in this special case, and keep the assertion about
> phnullingrels being NULL in fix_scan_expr.  I think that assertion is
> asserting the right thing in most cases.  It's a pity to lose it.

Well, if we change the nullingrels of the PHV in the Result, then we
will likely have to loosen the nullingrels cross-check in the next
plan level up.  That doesn't seem like much of an improvement.
Keeping the Result's tlist the same as what we would have generated for
a non-dummy join node seems right to me.

We could perhaps use a weaker assert like "phv->phnullingrels == NULL ||
we-are-at-a-dummy-Result", but I didn't think it was worth passing down
the extra flag needed to make that happen.  (Also, it's fair to wonder
whether setrefs.c actually knows whether a Result arose this way.)

Also, there are other places in setrefs.c that are punting on checking
phnullingrels.  If we don't tighten all of them, I doubt we've moved
the ball very far.

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In commit 2489d76c4, I'd thought it'd be safe to assert that a
>> PlaceHolderVar appearing in a scan-level expression has empty
>> nullingrels.  However this is not so, as when we determine that a
>> join relation is certainly empty we'll put its targetlist into a
>> Result-with-constant-false-qual node, and nothing is done to adjust
>> the nullingrels of the Vars or PHVs therein.  (Arguably, a Result
>> used in this way isn't really a scan-level node, but it certainly
>> isn't an upper node either ...)

> It seems this is the only case we can have PlaceHolderVar with non-empty
> nullingrels at scan level.  So I wonder if we can manually adjust the
> nullingrels of PHVs in this special case, and keep the assertion about
> phnullingrels being NULL in fix_scan_expr.  I think that assertion is
> asserting the right thing in most cases.  It's a pity to lose it.

Well, if we change the nullingrels of the PHV in the Result, then we
will likely have to loosen the nullingrels cross-check in the next
plan level up.  That doesn't seem like much of an improvement.
Keeping the Result's tlist the same as what we would have generated for
a non-dummy join node seems right to me.

We could perhaps use a weaker assert like "phv->phnullingrels == NULL ||
we-are-at-a-dummy-Result", but I didn't think it was worth passing down
the extra flag needed to make that happen.  (Also, it's fair to wonder
whether setrefs.c actually knows whether a Result arose this way.)

Also, there are other places in setrefs.c that are punting on checking
phnullingrels.  If we don't tighten all of them, I doubt we've moved
the ball very far.

            regards, tom lane