| From: | Tender Wang <tndrwang(at)gmail(dot)com> |
|---|---|
| To: | Tomas Vondra <tomas(at)vondra(dot)me> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Richard Guo <guofenglinux(at)gmail(dot)com> |
| Subject: | Re: A gap in preventing bogus parameterizations in a798660ebe3 |
| Date: | 2026-11-02 06:18:35 |
| Message-ID: | CAHewXN=AVxH+E1X=C0bek5h-wa2Lz+WbF8z2VHA-5XmE=4d+CQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Tomas,
Tomas Vondra <tomas(at)vondra(dot)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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Keyerror Smart | 2026-11-02 06:29:53 | Re: [PATCH]Fix planner's strictness check for JsonExpr |
| Previous Message | vignesh C | 2026-11-02 06:04:52 | Re: WAL record CRC calculated incorrectly because of underlying buffer modification |