Skip to content

Commit 03d7f3b

Browse files
tglsfdctglsfdc
committed
Remove an unsafe Assert, and explain join_clause_is_movable_into() better.
join_clause_is_movable_into() is approximate, in the sense that it might sometimes return "false" when actually it would be valid to push the given join clause down to the specified level. This is okay ... but there was an Assert in get_joinrel_parampathinfo() that's only safe if the answers are always exact. Comment out the Assert, and add a bunch of commentary to clarify what's going on. Per fuzz testing by Andreas Seltenreich. The added regression test is a pretty silly query, but it's based on his crasher example. Back-patch to 9.2 where the faulty logic was introduced.
1 parent 588f50f commit 03d7f3b

File tree

4 files changed

+116
-7
lines changed

4 files changed

+116
-7
lines changed

‎src/backend/optimizer/util/relnode.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -939,9 +939,18 @@ get_joinrel_parampathinfo(PlannerInfo *root, RelOptInfo *joinrel,
939939
{
940940
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
941941

942+
/*
943+
* In principle, join_clause_is_movable_into() should accept anything
944+
* returned by generate_join_implied_equalities(); but because its
945+
* analysis is only approximate, sometimes it doesn't. So we
946+
* currently cannot use this Assert; instead just assume it's okay to
947+
* apply the joinclause at this level.
948+
*/
949+
#ifdef NOT_USED
942950
Assert(join_clause_is_movable_into(rinfo,
943951
joinrel->relids,
944952
join_and_req));
953+
#endif
945954
if (!join_clause_is_movable_into(rinfo,
946955
outer_path->parent->relids,
947956
outer_and_req) &&

‎src/backend/optimizer/util/restrictinfo.c

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -651,10 +651,9 @@ extract_actual_join_clauses(List *restrictinfo_list,
651651
* outer join, as that would change the results (rows would be suppressed
652652
* rather than being null-extended).
653653
*
654-
* Also the target relation must not be in the clause's nullable_relids, i.e.,
655-
* there must not be an outer join below the clause that would null the Vars
656-
* coming from the target relation. Otherwise the clause might give results
657-
* different from what it would give at its normal semantic level.
654+
* Also there must not be an outer join below the clause that would null the
655+
* Vars coming from the target relation. Otherwise the clause might give
656+
* results different from what it would give at its normal semantic level.
658657
*
659658
* Also, the join clause must not use any relations that have LATERAL
660659
* references to the target relation, since we could not put such rels on
@@ -703,10 +702,31 @@ join_clause_is_movable_to(RestrictInfo *rinfo, RelOptInfo *baserel)
703702
* not pushing the clause into its outer-join outer side, nor down into
704703
* a lower outer join's inner side.
705704
*
705+
* The check about pushing a clause down into a lower outer join's inner side
706+
* is only approximate; it sometimes returns "false" when actually it would
707+
* be safe to use the clause here because we're still above the outer join
708+
* in question. This is okay as long as the answers at different join levels
709+
* are consistent: it just means we might sometimes fail to push a clause as
710+
* far down as it could safely be pushed. It's unclear whether it would be
711+
* worthwhile to do this more precisely. (But if it's ever fixed to be
712+
* exactly accurate, there's an Assert in get_joinrel_parampathinfo() that
713+
* should be re-enabled.)
714+
*
706715
* There's no check here equivalent to join_clause_is_movable_to's test on
707716
* lateral_referencers. We assume the caller wouldn't be inquiring unless
708717
* it'd verified that the proposed outer rels don't have lateral references
709-
* to the current rel(s).
718+
* to the current rel(s). (If we are considering join paths with the outer
719+
* rels on the outside and the current rels on the inside, then this should
720+
* have been checked at the outset of such consideration; see join_is_legal
721+
* and the path parameterization checks in joinpath.c.) On the other hand,
722+
* in join_clause_is_movable_to we are asking whether the clause could be
723+
* moved for some valid set of outer rels, so we don't have the benefit of
724+
* relying on prior checks for lateral-reference validity.
725+
*
726+
* Note: if this returns true, it means that the clause could be moved to
727+
* this join relation, but that doesn't mean that this is the lowest join
728+
* it could be moved to. Caller may need to make additional calls to verify
729+
* that this doesn't succeed on either of the inputs of a proposed join.
710730
*
711731
* Note: get_joinrel_parampathinfo depends on the fact that if
712732
* current_and_outer is NULL, this function will always return false
@@ -721,15 +741,20 @@ join_clause_is_movable_into(RestrictInfo *rinfo,
721741
if (!bms_is_subset(rinfo->clause_relids, current_and_outer))
722742
return false;
723743

724-
/* Clause must physically reference target rel(s) */
744+
/* Clause must physically reference at least one target rel */
725745
if (!bms_overlap(currentrelids, rinfo->clause_relids))
726746
return false;
727747

728748
/* Cannot move an outer-join clause into the join's outer side */
729749
if (bms_overlap(currentrelids, rinfo->outer_relids))
730750
return false;
731751

732-
/* Target rel(s) must not be nullable below the clause */
752+
/*
753+
* Target rel(s) must not be nullable below the clause. This is
754+
* approximate, in the safe direction, because the current join might be
755+
* above the join where the nulling would happen, in which case the clause
756+
* would work correctly here. But we don't have enough info to be sure.
757+
*/
733758
if (bms_overlap(currentrelids, rinfo->nullable_relids))
734759
return false;
735760

‎src/test/regress/expected/join.out

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2184,6 +2184,54 @@ select aa, bb, unique1, unique1
21842184
----+----+---------+---------
21852185
(0 rows)
21862186

2187+
--
2188+
-- regression test: check a case where join_clause_is_movable_into() gives
2189+
-- an imprecise result
2190+
--
2191+
analyze pg_enum;
2192+
explain (costs off)
2193+
select anname, outname, enumtypid
2194+
from
2195+
(select pa.proname as anname, coalesce(po.proname, typname) as outname
2196+
from pg_type t
2197+
left join pg_proc po on po.oid = t.typoutput
2198+
join pg_proc pa on pa.oid = t.typanalyze) ss,
2199+
pg_enum,
2200+
pg_type t2
2201+
where anname = enumlabel and outname = t2.typname and enumtypid = t2.oid;
2202+
QUERY PLAN
2203+
-----------------------------------------------------------------------
2204+
Nested Loop
2205+
Join Filter: (pg_enum.enumtypid = t2.oid)
2206+
-> Nested Loop Left Join
2207+
-> Hash Join
2208+
Hash Cond: ((t.typanalyze)::oid = pa.oid)
2209+
-> Seq Scan on pg_type t
2210+
-> Hash
2211+
-> Hash Join
2212+
Hash Cond: (pa.proname = pg_enum.enumlabel)
2213+
-> Seq Scan on pg_proc pa
2214+
-> Hash
2215+
-> Seq Scan on pg_enum
2216+
-> Index Scan using pg_proc_oid_index on pg_proc po
2217+
Index Cond: (oid = (t.typoutput)::oid)
2218+
-> Index Scan using pg_type_typname_nsp_index on pg_type t2
2219+
Index Cond: (typname = COALESCE(po.proname, t.typname))
2220+
(16 rows)
2221+
2222+
select anname, outname, enumtypid
2223+
from
2224+
(select pa.proname as anname, coalesce(po.proname, typname) as outname
2225+
from pg_type t
2226+
left join pg_proc po on po.oid = t.typoutput
2227+
join pg_proc pa on pa.oid = t.typanalyze) ss,
2228+
pg_enum,
2229+
pg_type t2
2230+
where anname = enumlabel and outname = t2.typname and enumtypid = t2.oid;
2231+
anname | outname | enumtypid
2232+
--------+---------+-----------
2233+
(0 rows)
2234+
21872235
--
21882236
-- Clean up
21892237
--

‎src/test/regress/sql/join.sql

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,33 @@ select aa, bb, unique1, unique1
365365
from tenk1 right join b on aa = unique1
366366
where bb < bb and bb is null;
367367

368+
--
369+
-- regression test: check a case where join_clause_is_movable_into() gives
370+
-- an imprecise result
371+
--
372+
analyze pg_enum;
373+
explain (costs off)
374+
select anname, outname, enumtypid
375+
from
376+
(select pa.proname as anname, coalesce(po.proname, typname) as outname
377+
from pg_type t
378+
left join pg_proc po on po.oid = t.typoutput
379+
join pg_proc pa on pa.oid = t.typanalyze) ss,
380+
pg_enum,
381+
pg_type t2
382+
where anname = enumlabel and outname = t2.typname and enumtypid = t2.oid;
383+
384+
select anname, outname, enumtypid
385+
from
386+
(select pa.proname as anname, coalesce(po.proname, typname) as outname
387+
from pg_type t
388+
left join pg_proc po on po.oid = t.typoutput
389+
join pg_proc pa on pa.oid = t.typanalyze) ss,
390+
pg_enum,
391+
pg_type t2
392+
where anname = enumlabel and outname = t2.typname and enumtypid = t2.oid;
393+
394+
368395
--
369396
-- Clean up
370397
--

0 commit comments

Comments
 (0)