Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

Thanks Tels for reviewing the patch.

On Fri, Dec 8, 2017 at 2:54 PM, Tels <nospam-pg-abuse(at)bloodgate(dot)com> wrote:

> Hello Rushabh,
>
> On Fri, December 8, 2017 2:28 am, Rushabh Lathia wrote:
> > Thanks for review.
> >
> > On Fri, Dec 8, 2017 at 6:27 AM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> >
> >> On Thu, Dec 7, 2017 at 12:25 AM, Rushabh Lathia
> >> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
> >> > 0001-Add-parallel-B-tree-index-build-sorting_v14.patch
>
> I've looked only at patch 0002, here are some comments.
>
> > + * leaderasworker indicates whether leader going to participate as
> worker or
> > + * not.
>
> The grammar is a bit off, and the "or not" seems obvious. IMHO this could
> be:
>
> + * leaderasworker indicates whether the leader is going to participate as
> worker
>
>
Sure.

> The argument leaderasworker is only used once and for one temp. variable
> that is only used once, too. So the temp. variable could maybe go.
>
> And not sure what the verdict was from the const-discussion threads, I did
> not follow it through. If "const" is what should be done generally, then
> the argument could be consted, as to not create more "unconsted" code.
>
> E.g. so:
>
> +plan_create_index_workers(Oid tableOid, Oid indexOid, const bool
> leaderasworker)
>
>
Make sense.

> and later:
>
> - sort_mem_blocks / (parallel_workers + 1) <
> min_sort_mem_blocks)
> + sort_mem_blocks / (parallel_workers + (leaderasworker
> ? 1 : 0)) < min_sort_mem_blocks)
>
>
Even I didn't liked to take a extra variable, but then code looks bit
unreadable - so rather then making difficult to read the code - I thought
of adding new variable.

> Thank you for working on this patch!
>
>
I will address review comments in the next set of patches.

Regards,
Rushabh Lathia
www.EnterpriseDB.com