Assert !bms_overlap(joinrel->relids, required_outer)
Hi, The attached query makes beta2 crash with attached backtrace. Interestingly the index on ref_6 is needed to make it crash, without it the query works fine. -- Jaime Casanova Director de Servicios Profesionales SYSTEMGUARDS - Consultores de PostgreSQL #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 set = {__val = {4194304, 140734892134240, 2, 6, 7058365, 94467046612912, 4611686018427388799, 140602878769846, 0, 281470681751456, 0, 0, 0, 0, 0, 0}} pid = tid = ret = #1 0x7fe0a7c60535 in __GI_abort () at abort.c:79 save_stage = 1 act = {__sigaction_handler = {sa_handler = 0x0, sa_sigaction = 0x0}, sa_mask = {__val = {0, 0, 0, 0, 0, 140602865618952, 2, 7018074096076324867, 7017846515177828964, 94467046612912, 7003721282549509696, 0, 6603711216602630912, 140734892134480, 0, 140734892135344}}, sa_flags = -759062608, sa_restorer = 0x0} sigs = {__val = {32, 0 }} #2 0x55ead332796a in ExceptionalCondition (conditionName=0x55ead34de1f0 "!bms_overlap(joinrel->relids, required_outer)", fileName=0x55ead34ddcef "relnode.c", lineNumber=1652) at assert.c:66 No locals. #3 0x55ead306f7b8 in get_joinrel_parampathinfo (root=0x7fe09c5bd0a0, joinrel=0x7fe09c399d98, outer_path=0x7fe09c391e68, inner_path=0x7fe09c38fd28, sjinfo=0x7fe09c4766a0, required_outer=0x7fe09c39a218, restrict_clauses=0x7fff6540d548) at relnode.c:1652 ppi = 0x10 join_and_req = 0x7fff6540d4a0 outer_and_req = 0x0 inner_and_req = 0x7fe09c391a78 pclauses = 0x55ead4a896e0 eclauses = 0x386540d4a0 dropped_ecs = 0x68 rows = 4.6672926152656088e-310 lc = 0x55ead3366497 #4 0x55ead30605b6 in create_nestloop_path (root=0x7fe09c5bd0a0, joinrel=0x7fe09c399d98, jointype=JOIN_LEFT, workspace=0x7fff6540d590, extra=0x7fff6540d740, outer_path=0x7fe09c391e68, inner_path=0x7fe09c38fd28, restrict_clauses=0x0, pathkeys=0x0, required_outer=0x7fe09c39a218) at pathnode.c:2462 pathnode = 0x7fe09c399a08 inner_req_outer = 0x0 #5 0x55ead3007a7d in try_nestloop_path (root=0x7fe09c5bd0a0, joinrel=0x7fe09c399d98, outer_path=0x7fe09c391e68, inner_path=0x7fe09c38fd28, pathkeys=0x0, jointype=JOIN_LEFT, extra=0x7fff6540d740) at joinpath.c:775 required_outer = 0x7fe09c39a218 workspace = {startup_cost = 2.1988, total_cost = 10.754533615595889, run_cost = 8.336155958894, inner_run_cost = 6.9466950277108173e-310, inner_rescan_run_cost = 0, outer_rows = 0, inner_rows = 6.9532275375539389e-310, outer_skip_rows = 4.667292444873237e-310, inner_skip_rows = 3.9525251667299724e-322, numbuckets = 0, numbatches = 0, inner_rows_total = 6.946694916976477e-310} innerrel = 0x7fe09c4743d0 outerrel = 0x7fe09c391868 innerrelids = 0x7fe09c474670 outerrelids = 0x7fe09c391a78 inner_paramrels = 0x0 outer_paramrels = 0x7fe09c391d18 #6 0x55ead300904d in match_unsorted_outer (root=0x7fe09c5bd0a0, joinrel=0x7fe09c399d98, outerrel=0x7fe09c391868, innerrel=0x7fe09c4743d0, jointype=JOIN_LEFT, extra=0x7fff6540d740) at joinpath.c:1802 innerpath = 0x7fe09c38fd28 mpath = 0x0 lc2__state = {l = 0x7fe09c38ff68, i = 0} lc2 = 0x7fe09c38ff80 outerpath = 0x7fe09c391e68 merge_pathkeys = 0x0 lc1__state = {l = 0x7fe09c391dd8, i = 0} save_jointype = JOIN_LEFT nestjoinOK = true useallclauses = false inner_cheapest_total = 0x7fe09c38fd28 matpath = 0x7fe09c393458 lc1 = 0x7fe09c391df0 __func__ = "match_unsorted_outer" #7 0x55ead3006ed3 in add_paths_to_joinrel (root=0x7fe09c5bd0a0, joinrel=0x7fe09c399d98, outerrel=0x7fe09c391868, innerrel=0x7fe09c4743d0, jointype=JOIN_LEFT, sjinfo=0x7fe09c4766a0, restrictlist=0x0) at joinpath.c:294 extra = {restrictlist = 0x0, mergeclause_list = 0x0, inner_unique = false, sjinfo = 0x7fe09c4766a0, semifactors = { outer_match_frac = 6.946694916976477e-310, match_count = 0}, param_source_rels = 0x7fe09c39a1f8} mergejoin_allowed = true lc = 0x0 joinrelids = 0x7fe09c399d78 #8 0x55ead300b9e9 in populate_joinrel_with_paths (root=0x7fe09c5bd0a0, rel1=0x7fe09c391868, rel2=0x7fe09c4743d0, joinrel=0x7fe09c399d98, sjinfo=0x7fe09c4766a0, restrictlist=0x0) at joinrels.c:942 __func__ = "populate_joinrel_with_paths" #9 0x55ead300b5b7 in make_join_rel (root=0x7fe09c5bd0a0, rel1=0x7fe09c391868, rel2=0x7fe09c4743d0) at joinrels.c:776 joinrelids = 0x7fe09c38fe58 sjinfo = 0x7fe09c4766a0 reversed = false pushed_down_joins = 0x0 sjinfo_data = {type = 2621925520, min_lefthand = 0x7fe09c474670, min_righthand = 0x7fe09c4737a0, syn_lefthand = 0x1, syn_righthand = 0x7fff6540d8c0, jointype = 3540362286, ojrelid = 2
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Mon, Jun 26, 2023 at 11:05:43PM -0500, Jaime Casanova wrote: > The attached query makes beta2 crash with attached backtrace. > Interestingly the index on ref_6 is needed to make it crash, without > it the query works fine. Issue reproduced here. I am adding an open item, whose owner should be Tom? -- Michael signature.asc Description: PGP signature
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Tue, Jun 27, 2023 at 1:35 PM Michael Paquier wrote: > On Mon, Jun 26, 2023 at 11:05:43PM -0500, Jaime Casanova wrote: > > The attached query makes beta2 crash with attached backtrace. > > Interestingly the index on ref_6 is needed to make it crash, without > > it the query works fine. > > Issue reproduced here. I am adding an open item, whose owner should > be Tom? That's right. This issue has something to do with the outer-join-aware-Var changes. I reduced the repro to the query below. create table t (a int); create index on t(a); explain (costs off) select 1 from t t1 join lateral (select t1.a from (select 1) foo offset 0) s1 on true join (select 1 from t t2 inner join t t3 left join t t4 left join t t5 on t4.a = 1 on t4.a = 1 on false where t3.a = coalesce(t5.a,1)) as s2 on true; When joining s1/t3 to t4, the relid of outer join t3/t4 appears both in the joinrel's relids and in the joinrel's required outer rels, which causes the Assert failure. I think it's reasonable for it to appear in the joinrel's relids, because we're forming this outer join. I doubt that it should appear in the joinrel's required outer rels. So I'm wondering if we can fix this issue by manually removing the outer join's relid from the joinrel's required_outer, something like: if (bms_is_member(extra->sjinfo->ojrelid, joinrel->relids)) required_outer = bms_del_member(required_outer, extra->sjinfo->ojrelid); This would be needed in try_nestloop_path, try_mergejoin_path and try_hashjoin_path after the required_outer set is computed for the join path. It seems quite hacky though, not sure if this is the right thing to do. Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
Richard Guo writes: > That's right. This issue has something to do with the > outer-join-aware-Var changes. I reduced the repro to the query below. Thanks for the simplified test case. > When joining s1/t3 to t4, the relid of outer join t3/t4 appears both in > the joinrel's relids and in the joinrel's required outer rels, which > causes the Assert failure. I think it's reasonable for it to appear in > the joinrel's relids, because we're forming this outer join. I doubt > that it should appear in the joinrel's required outer rels. It looks to me like we are trying to join (2 7), that is s1 and t3, to 8 (t4), which would necessitate forming the outer join with relid 11. That's fine as far as it goes, but the path we're trying to use for (2 7) is {NESTPATH :jpath.path.pathtype 335 :parent_relids (b 2 7) :required_outer (b 1 9 10 11) :jpath.outerjoinpath {SUBQUERYSCANPATH :path.pathtype 326 :parent_relids (b 2) :required_outer (b 1) :jpath.innerjoinpath {INDEXPATH :path.pathtype 321 :parent_relids (b 7) t3 :required_outer (b 9 10 11) t5 and both outer joins That is, the path involves an indexscan on t3 that evidently is using the "t3.a = coalesce(t5.a,1)" condition, so it needs a post-join value of t5.a. So it's completely not legit to use this path as an input for this join. (You could quibble about whether the path could be marked as needing only one of the two outer joins, but that doesn't really matter here. It certainly shouldn't be used when we've not yet formed either OJ.) So it looks to me like something further up should have rejected this path as not being usable here. Not sure what's dropping the ball. Another way to look at it is we should never have formed this index path at all, because it's not clear to me that it can have any valid use. We clearly cannot form OJ 11 (t3/t4) without having already scanned t3, so a path for t3 that requires 11 as an input is silly on its face. Even if you argue that the required_outer marking for the path could be reduced to (9 10) on the grounds of identity 3, I still don't see a valid join order that can use this path. So ideally the path wouldn't have been made in the first place, it's just a waste of planner cycles. That's a separate issue though. regards, tom lane
Re: Assert !bms_overlap(joinrel->relids, required_outer)
I wrote: > So it looks to me like something further up should have rejected this > path as not being usable here. Not sure what's dropping the ball. After further digging, I've concluded that what usually stops us from generating this bogus path for the t3/t4 join is the "param_source_rels" heuristic in joinpath.c. It does stop it in the simplified query select 1 from t t2 inner join (t t3 left join (t t4 left join t t5 on t4.a = 1) on t4.a = 1) on false where t3.a = coalesce(t5.a,1); However, once we add the lateral reference in s1, that heuristic uncritically lets the path go through, and then later we have trouble. Of course, that heuristic is only supposed to be a heuristic that helps winnow valid paths, not a defense against invalid paths, so it's not its fault that this goes wrong. (I think that the old delay_upper_joins mechanism is what prevented this error before v16.) For a real fix, I'm inclined to extend the loop that calculates param_source_rels (in add_paths_to_joinrel) so that it also tracks a set of incompatible relids that *must not* be present in the parameterization of a proposed path. This would basically include OJ relids of OJs that partially overlap the target joinrel; maybe we should also include the min RHS of such OJs. Then we could check that in try_nestloop_path. I've not tried to code this yet. There's also the question of why we generated the bogus indexscan in the first place; but it seems advisable to fix the join-level issue before touching that, else we'll have nothing to test with. regards, tom lane
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Tue, Jun 27, 2023 at 10:12 PM Tom Lane wrote: > Richard Guo writes: > > That's right. This issue has something to do with the > > outer-join-aware-Var changes. I reduced the repro to the query below. > > Thanks for the simplified test case. > > > When joining s1/t3 to t4, the relid of outer join t3/t4 appears both in > > the joinrel's relids and in the joinrel's required outer rels, which > > causes the Assert failure. I think it's reasonable for it to appear in > > the joinrel's relids, because we're forming this outer join. I doubt > > that it should appear in the joinrel's required outer rels. > > It looks to me like we are trying to join (2 7), that is s1 and t3, > to 8 (t4), which would necessitate forming the outer join with relid 11. > That's fine as far as it goes, but the path we're trying to use for > (2 7) is > >{NESTPATH >:jpath.path.pathtype 335 >:parent_relids (b 2 7) >:required_outer (b 1 9 10 11) >:jpath.outerjoinpath > {SUBQUERYSCANPATH > :path.pathtype 326 > :parent_relids (b 2) > :required_outer (b 1) >:jpath.innerjoinpath > {INDEXPATH > :path.pathtype 321 > :parent_relids (b 7) t3 > :required_outer (b 9 10 11) t5 and both outer joins > > That is, the path involves an indexscan on t3 that evidently is using > the "t3.a = coalesce(t5.a,1)" condition, so it needs a post-join value > of t5.a. So it's completely not legit to use this path as an input > for this join. (You could quibble about whether the path could be > marked as needing only one of the two outer joins, but that doesn't > really matter here. It certainly shouldn't be used when we've not > yet formed either OJ.) I tried this query on v15 and found that we'd also generate this bogus path for the t3/t4 join. {NESTPATH :pathtype 38 :parent_relids (b 2 7) :required_outer (b 1 9) :outerjoinpath {SUBQUERYSCANPATH :pathtype 28 :parent_relids (b 2) :required_outer (b 1) :innerjoinpath {INDEXPATH :pathtype 23 :parent_relids (b 7) t3 :required_outer (b 9) t5 The Assert failure is not seen on v15 because outer join relids are not included in joinrel's relids and required_outer sets. Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Wed, Jun 28, 2023 at 6:28 AM Tom Lane wrote: > For a real fix, I'm inclined to extend the loop that calculates > param_source_rels (in add_paths_to_joinrel) so that it also tracks > a set of incompatible relids that *must not* be present in the > parameterization of a proposed path. This would basically include > OJ relids of OJs that partially overlap the target joinrel; maybe > we should also include the min RHS of such OJs. Then we could > check that in try_nestloop_path. I've not tried to code this yet. I went ahead and drafted a patch based on this idea. A little differences include * You mentioned that the incompatible relids might need to also include the min_righthand of the OJs that are part of the target joinrel. It seems to me that we may need to also include the min_lefthand of such OJs, because the parameterization of any proposed join path for the target joinrel should not overlap anything in an OJ if the OJ is part of this joinrel. * I think we need to check the incompatible relids also in try_hashjoin_path and try_mergejoin_path besides try_nestloop_path. Thanks Richard v1-0001-Draft-Avoid-invalid-parameterized-path-for-joinrel.patch Description: Binary data
Re: Assert !bms_overlap(joinrel->relids, required_outer)
Richard Guo writes: > On Wed, Jun 28, 2023 at 6:28 AM Tom Lane wrote: >> For a real fix, I'm inclined to extend the loop that calculates >> param_source_rels (in add_paths_to_joinrel) so that it also tracks >> a set of incompatible relids that *must not* be present in the >> parameterization of a proposed path. This would basically include >> OJ relids of OJs that partially overlap the target joinrel; maybe >> we should also include the min RHS of such OJs. Then we could >> check that in try_nestloop_path. I've not tried to code this yet. > I went ahead and drafted a patch based on this idea. Hmm. This patch is the opposite of what I'd been imagining, because I was thinking we needed to add OJs to param_incompatible_relids if they were *not* already in the join, rather than if they were. However, I tried it like that and while it did stop the assertion failure, it also broke a bunch of other test cases that no longer found the parameterized-nestloop plans they were supposed to find. So clearly I just didn't have my head screwed on in the correct direction yesterday. However, given that what we need is to exclude parameterization that depends on the currently-formed OJ, it seems to me we can do it more simply and without any new JoinPathExtraData field, as attached. What do you think? > * I think we need to check the incompatible relids also in > try_hashjoin_path and try_mergejoin_path besides try_nestloop_path. I think this isn't necessary, at least in my formulation. Those cases will go through calc_non_nestloop_required_outer which has /* neither path can require rels from the other */ Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids)); Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids)); In order to have a dependency on an OJ, a path would have to have a dependency on at least one of the OJ's base relations too, so I think these assertions show that the case won't arise. (Of course, if someone can trip one of these assertions, I'm wrong.) regards, tom lane diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index c2f211a60d..4b6ed6e312 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -698,6 +698,17 @@ try_nestloop_path(PlannerInfo *root, Relids inner_paramrels = PATH_REQ_OUTER(inner_path); Relids outer_paramrels = PATH_REQ_OUTER(outer_path); + /* + * If we are forming an outer join at this join, it's nonsensical to use + * an input path that uses the outer join as part of its parameterization. + * (This can happen despite our join order restrictions, since those apply + * to what is in an input relation not what its parameters are.) + */ + if (extra->sjinfo && extra->sjinfo->ojrelid != 0 && + (bms_is_member(extra->sjinfo->ojrelid, outer_paramrels) || + bms_is_member(extra->sjinfo->ojrelid, inner_paramrels))) + return; + /* * Paths are parameterized by top-level parents, so run parameterization * tests on the parent relids. diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 6917faec14..12b828fae3 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -5063,6 +5063,37 @@ select 1 from -- (0 rows) +explain (costs off) +select 1 from tenk1 t1 + join lateral + (select t1.unique1 from (select 1) foo offset 0) s1 on true + join +(select 1 from tenk1 t2 +inner join tenk1 t3 + left join tenk1 t4 left join tenk1 t5 on t4.unique1 = 1 +on t4.unique1 = 1 on false + where t3.unique1 = coalesce(t5.unique1,1)) as s2 + on true; +QUERY PLAN +-- + Result + One-Time Filter: false +(2 rows) + +select 1 from tenk1 t1 + join lateral + (select t1.unique1 from (select 1) foo offset 0) s1 on true + join +(select 1 from tenk1 t2 +inner join tenk1 t3 + left join tenk1 t4 left join tenk1 t5 on t4.unique1 = 1 +on t4.unique1 = 1 on false + where t3.unique1 = coalesce(t5.unique1,1)) as s2 + on true; + ?column? +-- +(0 rows) + -- -- check a case in which a PlaceHolderVar forces join order -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 55080bec9a..38899ed3b9 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1751,6 +1751,29 @@ select 1 from on false, lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3; +explain (costs off) +select 1 from tenk1 t1 + join lateral + (select t1.unique1 from (select 1) foo offset 0) s1 on true + join +(select 1 from tenk1 t2 +inner join tenk1 t3 + left join tenk1 t4 left join tenk1 t5 on t4.unique
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Wed, Jun 28, 2023 at 10:09 PM Tom Lane wrote: > However, given that what we need is to exclude parameterization > that depends on the currently-formed OJ, it seems to me we can do > it more simply and without any new JoinPathExtraData field, > as attached. What do you think? I think it makes sense. At first I wondered if we should also exclude parameterization that depends on OJs that have already been formed as part of this joinrel. But it seems not possible that the input paths have parameterization dependency on these OJs. So it should be sufficient to only consider the currently-formed OJ. > > * I think we need to check the incompatible relids also in > > try_hashjoin_path and try_mergejoin_path besides try_nestloop_path. > > I think this isn't necessary, at least in my formulation. > Those cases will go through calc_non_nestloop_required_outer > which has > > /* neither path can require rels from the other */ > Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids)); > Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids)); > > In order to have a dependency on an OJ, a path would have to have > a dependency on at least one of the OJ's base relations too, so > I think these assertions show that the case won't arise. (Of > course, if someone can trip one of these assertions, I'm wrong.) Hmm, while this holds in most cases, it does not if the joins have been commuted according to identity 3. If we change the t3/t4 join's qual to 't3.a = t4.a' to make hashjoin possible, we'd see the same Assert failure through try_hashjoin_path. I think it's also possible for merge join. explain (costs off) select 1 from t t1 join lateral (select t1.a from (select 1) foo offset 0) s1 on true join (select 1 from t t2 inner join t t3 left join t t4 left join t t5 on t4.a = 1 on t3.a = t4.a on false where t3.a = coalesce(t5.a,1)) as s2 on true; server closed the connection unexpectedly Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Thu, Jun 29, 2023 at 10:39 AM Richard Guo wrote: > On Wed, Jun 28, 2023 at 10:09 PM Tom Lane wrote: > >> However, given that what we need is to exclude parameterization >> that depends on the currently-formed OJ, it seems to me we can do >> it more simply and without any new JoinPathExtraData field, >> as attached. What do you think? > > > I think it makes sense. At first I wondered if we should also exclude > parameterization that depends on OJs that have already been formed as > part of this joinrel. But it seems not possible that the input paths > have parameterization dependency on these OJs. So it should be > sufficient to only consider the currently-formed OJ. > BTW, it seems that extra->sjinfo would always have a valid value here. So maybe we do not need to check if it is not NULL explicitly? Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Wed, Jun 28, 2023 at 10:09 PM Tom Lane wrote: > Those cases will go through calc_non_nestloop_required_outer > which has > > /* neither path can require rels from the other */ > Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids)); > Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids)); Looking at these two assertions it occurred to me that shouldn't we check against top_parent_relids for an otherrel since paths are parameterized by top-level parents? We do that in try_nestloop_path. /* neither path can require rels from the other */ - Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids)); - Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids)); + Assert(!bms_overlap(outer_paramrels, + inner_path->parent->top_parent_relids ? + inner_path->parent->top_parent_relids : + inner_path->parent->relids)); + Assert(!bms_overlap(inner_paramrels, + outer_path->parent->top_parent_relids ? + outer_path->parent->top_parent_relids : + outer_path->parent->relids)); This is not related to the issue being discussed here. Maybe it should be a separate issue. Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
Richard Guo writes: > BTW, it seems that extra->sjinfo would always have a valid value here. > So maybe we do not need to check if it is not NULL explicitly? Right, I was being conservative but this module expects that to always be provided. Pushed with that and defenses added to try_mergejoin_path and try_hashjoin_path. It looks like the various try_partial_xxx_path functions already reject cases that could be problematic. (They will not accept inner parameterization that would lead to the result being parameterized differently from the outer path. By induction, that should be fine.) regards, tom lane
Re: Assert !bms_overlap(joinrel->relids, required_outer)
Richard Guo writes: > On Wed, Jun 28, 2023 at 10:09 PM Tom Lane wrote: >> Those cases will go through calc_non_nestloop_required_outer >> which has >> /* neither path can require rels from the other */ >> Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids)); >> Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids)); > Looking at these two assertions it occurred to me that shouldn't we > check against top_parent_relids for an otherrel since paths are > parameterized by top-level parents? We do that in try_nestloop_path. Yeah, while looking at this I was wondering why try_mergejoin_path and try_hashjoin_path don't do the same "Paths are parameterized by top-level parents, so run parameterization tests on the parent relids" dance that try_nestloop_path does. This omission is consistent with that, but it's not obvious why it'd be okay to skip it for non-nestloop joins. I guess we'd have noticed by now if it wasn't okay, but ... regards, tom lane
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Fri, Jun 30, 2023 at 12:16 AM Tom Lane wrote: > Pushed with that and defenses added to try_mergejoin_path and > try_hashjoin_path. It looks like the various try_partial_xxx_path > functions already reject cases that could be problematic. (They > will not accept inner parameterization that would lead to the > result being parameterized differently from the outer path. > By induction, that should be fine.) Thanks for pushing it! Yeah, I also checked that and there is no problem with partial join paths. However I found some opportunities for trivial revises there and created a new patch for those at [1]. [1] https://www.postgresql.org/message-id/flat/CAMbWs48mKJ6g_GnYNa7dnw04MHaMK-jnAEBrMVhTp2uUg3Ut4A%40mail.gmail.com Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Fri, Jun 30, 2023 at 12:20 AM Tom Lane wrote: > Richard Guo writes: > > On Wed, Jun 28, 2023 at 10:09 PM Tom Lane wrote: > >> Those cases will go through calc_non_nestloop_required_outer > >> which has > >> /* neither path can require rels from the other */ > >> Assert(!bms_overlap(outer_paramrels, inner_path->parent->relids)); > >> Assert(!bms_overlap(inner_paramrels, outer_path->parent->relids)); > > > Looking at these two assertions it occurred to me that shouldn't we > > check against top_parent_relids for an otherrel since paths are > > parameterized by top-level parents? We do that in try_nestloop_path. > > Yeah, while looking at this I was wondering why try_mergejoin_path and > try_hashjoin_path don't do the same "Paths are parameterized by > top-level parents, so run parameterization tests on the parent relids" > dance that try_nestloop_path does. This omission is consistent with > that, but it's not obvious why it'd be okay to skip it for > non-nestloop joins. I guess we'd have noticed by now if it wasn't > okay, but ... I think it just makes these two assertions meaningless to skip it for non-nestloop joins if the input paths are for otherrels, because paths would never be parameterized by the member relations. So these two assertions would always be true for otherrel paths. I think this is why we have not noticed any problem by now. However, this is not what we want. What we want is to verify that neither input path for the joinrel can require rels from the other, even for otherrel paths. So I think the current code is not right for that. We need to check against top_parent_relids for otherrel paths, and that would make these assertions meaningful. Thanks Richard
Re: Assert !bms_overlap(joinrel->relids, required_outer)
Richard Guo writes: > On Fri, Jun 30, 2023 at 12:20 AM Tom Lane wrote: >> Yeah, while looking at this I was wondering why try_mergejoin_path and >> try_hashjoin_path don't do the same "Paths are parameterized by >> top-level parents, so run parameterization tests on the parent relids" >> dance that try_nestloop_path does. This omission is consistent with >> that, but it's not obvious why it'd be okay to skip it for >> non-nestloop joins. I guess we'd have noticed by now if it wasn't >> okay, but ... > I think it just makes these two assertions meaningless to skip it for > non-nestloop joins if the input paths are for otherrels, because paths > would never be parameterized by the member relations. So these two > assertions would always be true for otherrel paths. I think this is why > we have not noticed any problem by now. After studying this some more, I think that maybe it's the "run parameterization tests on the parent relids" bit that is misguided. I believe the way it's really working is that all paths arriving here are parameterized by top parents, because that's the only thing we generate to start with. A path can only become parameterized by an otherrel when we apply reparameterize_path_by_child to it. But the only place that happens is in try_nestloop_path itself (or try_partial_nestloop_path), and then we immediately wrap it in a nestloop join node, which becomes a child of an append that's forming a partitionwise join. The partitionwise join as a whole won't be parameterized by any child rels. So I think that a path that's parameterized by a child rel can't exist "in the wild" in a way that would allow it to get fed to one of the try_xxx_path functions. This explains why the seeming oversights in the merge and hash cases aren't causing a problem. If this theory is correct, we could simplify try_nestloop_path a bit. I doubt the code savings would matter, but maybe it's worth changing for clarity's sake. regards, tom lane
Re: Assert !bms_overlap(joinrel->relids, required_outer)
On Fri, Jun 30, 2023 at 11:00 PM Tom Lane wrote: > Richard Guo writes: > > I think it just makes these two assertions meaningless to skip it for > > non-nestloop joins if the input paths are for otherrels, because paths > > would never be parameterized by the member relations. So these two > > assertions would always be true for otherrel paths. I think this is why > > we have not noticed any problem by now. > > After studying this some more, I think that maybe it's the "run > parameterization tests on the parent relids" bit that is misguided. > I believe the way it's really working is that all paths arriving > here are parameterized by top parents, because that's the only thing > we generate to start with. A path can only become parameterized > by an otherrel when we apply reparameterize_path_by_child to it. > But the only place that happens is in try_nestloop_path itself > (or try_partial_nestloop_path), and then we immediately wrap it in > a nestloop join node, which becomes a child of an append that's > forming a partitionwise join. The partitionwise join as a > whole won't be parameterized by any child rels. So I think that > a path that's parameterized by a child rel can't exist "in the wild" > in a way that would allow it to get fed to one of the try_xxx_path > functions. This explains why the seeming oversights in the merge > and hash cases aren't causing a problem. > > If this theory is correct, we could simplify try_nestloop_path a > bit. I doubt the code savings would matter, but maybe it's > worth changing for clarity's sake. Yeah, I think this theory is correct that all paths arriving at try_xxx_path are parameterized by top parents. But I do not get how to simplify try_nestloop_path on the basis of that. Would you please elaborate on that? Thanks Richard