Richard Guo <guofengli...@gmail.com> writes:
> On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <t...@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
