8000 Fix thinkos in have_unsafe_outer_join_ref; reduce to Assert check. · postgres/postgres@f50f029 · GitHub
[go: up one dir, main page]
Skip to content

Commit f50f029

Browse files
tglsfdctglsfdc
committed
Fix thinkos in have_unsafe_outer_join_ref; reduce to Assert check.
Late in the development of commit 2489d76, I (tgl) inco 8000 rrectly concluded that the new function have_unsafe_outer_join_ref couldn't ever reach its inner loop. That should be the case if the inner rel's parameterization is based on just one Var, but it could be based on Vars from several relations, and then not only is the inner loop reachable but it's wrongly coded. Despite those errors, it still appears that the whole thing is redundant given previous join_is_legal checks, so let's arrange to only run it in assert-enabled builds. Diagnosis and patch by Richard Guo, per fuzz testing by Justin Pryzby. Discussion: https://postgr.es/m/20230212235823.GW1653@telsasoft.com
1 parent b16259b commit f50f029

File tree

3 files changed

+57
-15
lines changed

3 files changed

+57
-15
lines changed

‎src/backend/optimizer/path/joinpath.c

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -378,21 +378,20 @@ allow_star_schema_join(PlannerInfo *root,
378378
* restrictions prevent us from attempting a join that would cause a problem.
379379
* (That's unsurprising, because the code worked before we ever added
380380
* outer-join relids to expression relids.) It still seems worth checking
381-
* as a backstop, but we don't go to a lot of trouble: just reject if the
382-
* unsatisfied part includes any outer-join relids at all.
381+
* as a backstop, but we only do so in assert-enabled builds.
383382
*/
383+
#ifdef USE_ASSERT_CHECKING
384384
static inline bool
385385
have_unsafe_outer_join_ref(PlannerInfo *root,
386386
Relids outerrelids,
387387
Relids inner_paramrels)
388388
{
389389
bool result = false;
390390
Relids unsatisfied = bms_difference(inner_paramrels, outerrelids);
391+
Relids satisfied = bms_intersect(inner_paramrels, outerrelids);
391392

392-
if (unlikely(bms_overlap(unsatisfied, root->outer_join_rels)))
393+
if (bms_overlap(unsatisfied, root->outer_join_rels))
393394
{
394-
#ifdef NOT_USED
395-
/* If we ever weaken the join order restrictions, we might need this */
396395
ListCell *lc;
397396

398397
foreach(lc, root->join_info_list)
@@ -401,25 +400,23 @@ have_unsafe_outer_join_ref(PlannerInfo *root,
401400

402401
if (!bms_is_member(sjinfo->ojrelid, unsatisfied))
403402
continue; /* not relevant */
404-
if (bms_overlap(inner_paramrels, sjinfo->min_righthand) ||
403+
if (bms_overlap(satisfied, sjinfo->min_righthand) ||
405404
(sjinfo->jointype == JOIN_FULL &&
406-
bms_overlap(inner_paramrels, sjinfo->min_lefthand)))
405+
bms_overlap(satisfied, sjinfo->min_lefthand)))
407406
{
408407
result = true; /* doesn't work */
409408
break;
410409
}
411410
}
412-
#else
413-
/* For now, if we do see an overlap, just assume it's trouble */
414-
result = true;
415-
#endif
416411
}
417412

418413
/* Waste no memory when we reject a path here */
419414
bms_free(unsatisfied);
415+
bms_free(satisfied);
420416

421417
return result;
422418
}
419+
#endif /* USE_ASSERT_CHECKING */
423420

424421
/*
425422
* paraminfo_get_equal_hashops
@@ -713,23 +710,25 @@ try_nestloop_path(PlannerInfo *root,
713710
/*
714711
* Check to see if proposed path is still parameterized, and reject if the
715712
* parameterization wouldn't be sensible --- unless allow_star_schema_join
716-
* says to allow it anyway. Also, we must reject if either
717-
* have_unsafe_outer_join_ref or have_dangerous_phv don't like the look of
718-
* it, which could only happen if the nestloop is still parameterized.
713+
* says to allow it anyway. Also, we must reject if have_dangerous_phv
714+
* doesn't like the look of it, which could only happen if the nestloop is
715+
* still parameterized.
719716
*/
720717
required_outer = calc_nestloop_required_outer(outerrelids, outer_paramrels,
721718
innerrelids, inner_paramrels);
722719
if (required_outer &&
723720
((!bms_overlap(required_outer, extra->param_source_rels) &&
724721
!allow_star_schema_join(root, outerrelids, inner_paramrels)) ||
725-
have_unsafe_outer_join_ref(root, outerrelids, inner_paramrels) ||
726722
have_dangerous_phv(root, outerrelids, inner_paramrels)))
727723
{
728724
/* Waste no memory when we reject a path here */
729725
bms_free(required_outer);
730726
return;
731727
}
732728

729+
/* If we got past that, we shouldn't have any unsafe outer-join refs */
730+
Assert(!have_unsafe_outer_join_ref(root, outerrelids, inner_paramrels));
731+
733732
/*
734733
* Do a precheck to quickly eliminate obviously-inferior paths. We
735734
* calculate a cheap lower bound on the path's cost and then use

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4653,6 +4653,32 @@ where tt1.f1 = ss1.c0;
46534653
----------
46544654
(0 rows)
46554655

4656+
explain (verbose, costs off)
4657+
select 1 from
4658+
int4_tbl as i4
4659+
inner join
4660+
((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
4661+
right join (select 1 as z) as ss2 on true)
4662+
on false,
4663+
lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
4664+
QUERY PLAN
4665+
--------------------------
4666+
Result
4667+
Output: 1
4668+
One-Time Filter: false
4669+
(3 rows)
4670+
4671+
select 1 from
4672+
int4_tbl as i4
4673+
inner join
4674+
((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
4675+
right join (select 1 as z) as ss2 on true)
4676+
on false,
4677+
lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
4678+
?column?
4679+
----------
4680+
(0 rows)
4681+
46564682
--
46574683
-- check a case in which a PlaceHolderVar forces join order
46584684
--

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1609,6 +1609,23 @@ select 1 from
16091609
lateral (select tt4.f1 as c0 from text_tbl as tt5 limit 1) as ss1
16101610
where tt1.f1 = ss1.c0;
16111611

1612+
explain (verbose, costs off)
1613+
select 1 from
1614+
int4_tbl as i4
1615+
inner join
1616+
((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
1617+
right join (select 1 as z) as ss2 on true)
1618+
on false,
1619+
lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
1620+
1621+
select 1 from
1622+
int4_tbl as i4
1623+
inner join
1624+
((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1
1625+
right join (select 1 as z) as ss2 on true)
1626+
on false,
1627+
lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3;
1628+
16121629
--
16131630
-- check a case in which a PlaceHolderVar forces join order
16141631
--

0 commit comments

Comments
 (0)
0