On Thu, Mar 7, 2019 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > This all looks good to me. I'm wondering about this chunk though:
>
> > + bool rel_is_partitioned = (rel->part_scheme && rel->part_rels);
>
> > IIUC it' safe for now (according to f069c91a579), but should we use
> > IS_PARTITIONED_REL macro instead? If yes, probably
> > create_ordinary_grouping_paths() should be updated too.
>
> Hmm, I was just copying the existing test in that function, but I agree
> that it seems pretty random to not be using the macro.
>
> Also, given that IS_PARTITIONED_REL is explicitly testing IS_DUMMY_REL,
> it doesn't really seem like we need the hack about forcing nparts etc
> to 0. I'd transferred that out of apply_scanjoin_target_to_paths into
> set_dummy_rel_pathlist and mark_dummy_rel, but that doesn't make it any
> less of an ugly kluge. In particular, that's not consistent with the
> idea that an appendrel automatically becomes dummy if we generate a
> zero-child path for it. So I'm now thinking we should drop that bit
> and instead make sure that everyplace that's testing for partitioned-ness
> is using this macro.
I did look for other usage yesterday, and AFAICT the only remaining
one is in create_ordinary_grouping_paths(), though it's already
checking for !IS_DUMMY_REL:
if (extra->patype != PARTITIONWISE_AGGREGATE_NONE &&
input_rel->part_scheme && input_rel->part_rels &&
!IS_DUMMY_REL(input_rel))
> One more thing --- after sleeping in it, I'm questioning my earlier
> feeling that apply_scanjoin_target_to_paths should flush existing paths
> for partitioned rels. Maybe it was done like that with the idea of
> letting paths with tlist computations done above the Append compete
> with paths doing the tlist computations below? That would be a fine
> idea if we had any costing factors that would produce a meaningful
> choice between the two; but I'm afraid that what we're probably getting
> right now is a quasi-random choice between paths whose costs differ
> only by rounding errors.
I don't know and the comments surely didn't mention that. But since
we're trying hard to speed up everything for high number of partitions
it seems like a good idea to avoid wasting cycles here if there's no
clear benefit.