MemoizePath fails to work for partitionwise join
I happened to notice that currently MemoizePath cannot be generated for
partitionwise join. I traced that and have found the problem. One
condition we need to meet to generate MemoizePath is that inner path's
ppi_clauses should have the form "outer op inner" or "inner op outer",
as checked by clause_sides_match_join in paraminfo_get_equal_hashops.
Note that when are at this check, the inner path has not got the chance
to be re-parameterized (that is done later in try_nestloop_path), so if
it is parameterized, it is parameterized by the topmost parent of the
outer rel, not the outer rel itself. Thus this check performed by
clause_sides_match_join could not succeed if we are joining two child
rels.
The fix is straightforward, just to use outerrel->top_parent if it is
not null and leave the reparameterization work to try_nestloop_path. In
addition, I believe when reparameterizing MemoizePath we need to adjust
its param_exprs too.
Attach the patch for fix.
Thanks
Richard
Attachments:
v1-0001-Fix-MemoizePath-for-partitionwise-join.patchapplication/octet-stream; name=v1-0001-Fix-MemoizePath-for-partitionwise-join.patchDownload+58-2
Richard Guo <guofenglinux@gmail.com> writes:
The fix is straightforward, just to use outerrel->top_parent if it is
not null and leave the reparameterization work to try_nestloop_path. In
addition, I believe when reparameterizing MemoizePath we need to adjust
its param_exprs too.
Right you are. I'd noticed the apparent omission in
reparameterize_path_by_child, but figured that we'd need a test case to
find any other holes before it'd be worth fixing. Thanks for finding
a usable test case.
One small problem is that top_parent doesn't exist in the back branches,
so I had to substitute a much uglier lookup in order to make this work
there. I'm surprised that we got away without top_parent for this long
TBH, but anyway this fix validates the wisdom of 2f17b5701.
So, pushed with some cosmetic adjustments and the modified back-branch
code.
regards, tom lane
On Tue, Dec 6, 2022 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
One small problem is that top_parent doesn't exist in the back branches,
so I had to substitute a much uglier lookup in order to make this work
there. I'm surprised that we got away without top_parent for this long
TBH, but anyway this fix validates the wisdom of 2f17b5701.So, pushed with some cosmetic adjustments and the modified back-branch
code.
Thanks for the modifying and pushing and the back-patching. I didn't
realize how the fix should look like if without top_parent. Thanks to
the work in 2f17b5701, it makes life much easier.
Thanks
Richard