A gap in preventing bogus parameterizations in a798660ebe3
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:t254508psql -h localhost -U postgresBuilt from patchset v1 (message #1), November 01, 2026 at 08:53 PM.
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 t254508_1 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 t254508_1 && git checkout t254508_1Patchset v1 (message #1) is on t254508_1
Hi,
The second bug / assert failure I ran into when running the randomized
join generator seems to be hitting a gap in a798660ebe3:
Defend against bogus parameterization of join input paths.
A trivial reproducer for this is:
CREATE TABLE t (a integer, b integer);
CREATE INDEX ON t (a, b);
EXPLAIN
SELECT 1 FROM t t1
WHERE t1.a IN (SELECT t2.a
FROM t t2
LEFT JOIN t t3 ON (t2.a = t3.a)
LEFT JOIN (t t4 LEFT JOIN t t5 ON (t4.a = t5.a))
ON (t3.a = t4.a)
WHERE coalesce(t5.b, 0) = t2.b);
Which trips over this asssert in get_baserel_parampathinfo():
Assert(!bms_overlap(joinrel->relids, required_outer));
This is not an area I know very well, but it seems the defense added by
a798660ebe3 considers only sjinfo->ojrelid. But that does not seem to be
enough for joins pushed down through identity 3. IIUC those are added to
ojrelid only for the join that completes it, and so the sjinfo->ojrelid
may not be entirely "up to date", and the parameterized paths have the
same issue as those rejected by a798660ebe3.
The attached patch fixes that by adding a new JoinPathExtraData field
ojrelids, and calculating it from joinrel/inner/outer like this:
if (sjinfo->ojrelid != 0)
{
extra.ojrelids = bms_difference(joinrel->relids, outerrel->relids);
extra.ojrelids = bms_del_members(extra.ojrelids, innerrel->relids);
extra.ojrelids = bms_add_member(extra.ojrelids, sjinfo->ojrelid);
}
There's a comment in add_paths_to_joinrel explaining why I think this is
correct, but is it? Perhaps I'm forgetting something.
Also, the new field would be a problem for backpatching (the problem
exists since 16), and a new field might break ABI. Perhaps it'd be
better to just calculate it in the handful of places that need it.
regards
--
Tomas Vondra
Hi Tomas,
Tomas Vondra <tomas@vondra.me> 于2026年11月2日周一 04:39写道：
Hi,
The second bug / assert failure I ran into when running the randomized
join generator seems to be hitting a gap in a798660ebe3:Defend against bogus parameterization of join input paths.
A trivial reproducer for this is:
CREATE TABLE t (a integer, b integer);
CREATE INDEX ON t (a, b);EXPLAIN
SELECT 1 FROM t t1
WHERE t1.a IN (SELECT t2.a
FROM t t2
LEFT JOIN t t3 ON (t2.a = t3.a)
LEFT JOIN (t t4 LEFT JOIN t t5 ON (t4.a = t5.a))
ON (t3.a = t4.a)
WHERE coalesce(t5.b, 0) = t2.b);Which trips over this asssert in get_baserel_parampathinfo():
Assert(!bms_overlap(joinrel->relids, required_outer));
Also, the new field would be a problem for backpatching (the problem
exists since 16), and a new field might break ABI. Perhaps it'd be
better to just calculate it in the handful of places that need it.
The existing checks only consider sjinfo->ojrelid, but outer-join
identity 3 can add additional OJ relids to the joinrel.
An input path parameterized by one of those OJs can therefore pass the
checks, leaving the resulting path dependent on the joinrel itself.
As you noted, adding a field to JoinPathExtraData raises an ABI
concern, so that version of the fix might need to be master-only.
For backpatching, perhaps a minimal fix would be to check
required_outer immediately after calculating it in
try_nestloop_path(), try_mergejoin_path(), and try_hashjoin_path():
if (bms_overlap(required_outer, joinrel->relids))
{
bms_free(required_outer);
return;
}
This would reject the invalid combination before creating the join
path, without changing JoinPathExtraData.
The existing sjinfo->ojrelid checks would remain necessary, since the
current OJ's relid might not yet be included in joinrel->relids when
it has been pushed down.
All other existing checks would remain unchanged.
Would this be a reasonable minimal fix for the back branches?
--
Thanks,
Tender Wang
On 2026-Nov-02, Tender Wang wrote:
As you noted, adding a field to JoinPathExtraData raises an ABI
concern, so that version of the fix might need to be master-only.
But nothing allocates JoinPathExtraData anywhere except
add_paths_to_joinrel in its stack; everybody else just receives a
pointer to that. Even the hooks for it receive it as a pointer. I
think the ABI concern is overblown in this case; just make sure to add
the new field at the end.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)