Re: pgsql: Remove over-optimistic Assert.

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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