Thread: pgsql: Further fixes for degenerate outer join clauses.
Further fixes for degenerate outer join clauses. Further testing revealed that commit f69b4b9495269cc4 was still a few bricks shy of a load: minor tweaking of the previous test cases resulted in the same wrong-outer-join-order problem coming back. After study I concluded that my previous changes in make_outerjoininfo() were just accidentally masking the problem, and should be reverted in favor of forcing syntactic join order whenever an upper outer join's predicate doesn't mention a lower outer join's LHS. This still allows the chained-outer-joins style that is the normally optimizable case. I also tightened things up some more in join_is_legal(). It seems to me on review that what's really happening in the exception case where we ignore a mismatched special join is that we're allowing the proposed join to associate into the RHS of the outer join we're comparing it to. As such, we should *always* insist that the proposed join be a left join, which eliminates a bunch of rather dubious argumentation. The case where we weren't enforcing that was the one that was already known buggy anyway (it had a violatable Assert before the aforesaid commit) so it hardly deserves a lot of deference. Back-patch to all active branches, like the previous patch. The added regression test case failed in all branches back to 9.1, and I think it's only an unrelated change in costing calculations that kept 9.0 from choosing a broken plan. Branch ------ REL9_1_STABLE Details ------- http://git.postgresql.org/pg/commitdiff/656b1e8cf358990b7700448d3b9e85202105cde0 Modified Files -------------- src/backend/optimizer/README | 23 +++++++--- src/backend/optimizer/path/joinrels.c | 73 ++++++++++++-------------------- src/backend/optimizer/plan/initsplan.c | 28 ++++++------ src/test/regress/expected/join.out | 72 ++++++++++++++++++++++++++++++- src/test/regress/sql/join.sql | 25 +++++++++++ 5 files changed, 153 insertions(+), 68 deletions(-)
On 08/06/2015 03:36 PM, Tom Lane wrote: > Further fixes for degenerate outer join clauses. > > Further testing revealed that commit f69b4b9495269cc4 was still a few > bricks shy of a load: minor tweaking of the previous test cases resulted > in the same wrong-outer-join-order problem coming back. After study > I concluded that my previous changes in make_outerjoininfo() were just > accidentally masking the problem, and should be reverted in favor of > forcing syntactic join order whenever an upper outer join's predicate > doesn't mention a lower outer join's LHS. This still allows the > chained-outer-joins style that is the normally optimizable case. > > I also tightened things up some more in join_is_legal(). It seems to me > on review that what's really happening in the exception case where we > ignore a mismatched special join is that we're allowing the proposed join > to associate into the RHS of the outer join we're comparing it to. As > such, we should *always* insist that the proposed join be a left join, > which eliminates a bunch of rather dubious argumentation. The case where > we weren't enforcing that was the one that was already known buggy anyway > (it had a violatable Assert before the aforesaid commit) so it hardly > deserves a lot of deference. > > Back-patch to all active branches, like the previous patch. The added > regression test case failed in all branches back to 9.1, and I think it's > only an unrelated change in costing calculations that kept 9.0 from > choosing a broken plan. Looks like this might have upset brolga on 9.0 and 9.1 - it's coming up with a different plan from what's expected. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 08/06/2015 03:36 PM, Tom Lane wrote:
>> Further fixes for degenerate outer join clauses.
> Looks like this might have upset brolga on 9.0 and 9.1 - it's coming up
> with a different plan from what's expected.
Yeah, I saw that, but haven't had time yet to investigate what's up.
Odd that it's just the one critter though ...
regards, tom lane