pgsql: Remove over-optimistic Assert.
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t47351psql -h localhost -U postgresBuilt from patchset v3 (message #3), July 28, 2026 at 01:13 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t47351_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t47351_3 && git checkout t47351_3Patchset v3 (message #3) is on t47351_3
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: /messages/by-id/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(-)
On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
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 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.
Currently for the tlist of a childless Result, we special-case ROWID_VAR
Vars in set_plan_refs and thus keep assertions about varno != ROWID_VAR
in fix_scan_expr. Do you think we can special-case PHVs at the same
place by setting its phnullingrels to NULL? I'm imagining something
like attached.
Thanks
Richard
Attachments:
v1-0001-Adjust-phnullingrels-for-childless-Result.patchapplication/octet-stream; name=v1-0001-Adjust-phnullingrels-for-childless-Result.patchDownload+22-7
Resend this email to -hackers. Sorry for the noise.
Thanks
Richard
---------- Forwarded message ---------
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, Feb 2, 2023 at 9:51 AM
Subject: Re: pgsql: Remove over-optimistic Assert.
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: <pgsql-committers@lists.postgresql.org>, PostgreSQL-development <
pgsql-hackers@postgresql.org>
On Thu, Feb 2, 2023 at 8:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
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 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.
Currently for the tlist of a childless Result, we special-case ROWID_VAR
Vars in set_plan_refs and thus keep assertions about varno != ROWID_VAR
in fix_scan_expr. Do you think we can special-case PHVs at the same
place by setting its phnullingrels to NULL? I'm imagining something
like attached.
Thanks
Richard
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