| From: | Richard Guo <guofenglinux(at)gmail(dot)com> |
|---|---|
| To: | Nitin Motiani <nitinmotiani(at)google(dot)com> |
| Cc: | Tender Wang <tndrwang(at)gmail(dot)com>, leis(at)in(dot)tum(dot)de, pgsql-bugs(at)lists(dot)postgresql(dot)org, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Subject: | Re: BUG #19765: Wrong result from a chain of LEFT JOINs since v16 |
| Date: | 2026-10-23 07:40:19 |
| Message-ID: | CAMbWs4_a50f8b3LyaXCRf85Z2A8BYfHAF868OfTQvhNnng4GkQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-bugs |
On Thu, Oct 22, 2026 at 10:50 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> Thanks for the patch! It's on my radar, but I'm currently occupied
> with a few other items. I'll do my best to get to it before the minor
> release.
I reviewed your v2 patch. It fixes this bug, but I don't think the
narrower check should rely on incompatible_relids. AFAIU, that field
is a placement filter: it lists the outer joins above which a clone
must not be evaluated, so that only one version of the clause is
accepted at any join level. The v2 patch reads it as "the set of
commuting joins whose nulling bit this clone lacks". That happens to
be true today, because of how deconstruct_distribute_oj_quals() builds
it, but it's not what the field means.
What matters is whether OUTERVAR lacks the nulling bit of a commuting
outer join whose relid can be withheld, and that can be checked
directly on the outer side's relids, which include its nullingrels:
if ((bms_is_member(othersj->ojrelid, sjinfo->commute_below_l) ||
bms_is_member(othersj->ojrelid, sjinfo->commute_above_r)) &&
!bms_is_member(othersj->ojrelid, outer_relids) &&
!bms_is_empty(othersj->commute_below_l))
return true;
That said, as Tender showed, the narrower check doesn't avoid all
redundant clauses either. So I still lean slightly toward simply
keeping all cloned clauses as-is (marked as having selectivity 1.0),
as v15 and earlier did. For example:
/* keep a cloned clause as-is */
if (rinfo->has_clone || rinfo->is_clone)
{
/* but the kept clause has no extra selectivity */
rinfo->norm_selec = 1.0;
rinfo->outer_selec = 1.0;
distribute_restrictinfo_to_rels(root, rinfo);
continue;
}
It's simpler, and its correctness doesn't depend on mirroring the
rules in add_outer_joins_to_relids(). This is also in line with
commit 716a051aa, where we stopped performing NullTest deduction for
clone clauses because their nullingrel bits may not reflect reality.
I don't think the redundant clauses cost enough to justify the extra
complexity of the narrower check. But this is a judgment call, and
neither approach is a clear win.
(Cc'ing Tom to have a look.)
- Richard
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-10-23 08:02:06 | Re: pg_stat_statements replaces SQL/JSON ON EMPTY / ON ERROR keywords with parameters |
| Previous Message | weijie JL | 2026-10-23 07:15:43 | Re: BUG #19725: PostgreSQL 18.6: pg_restore read failure with io_uring, not observed with worker |