Thanks Tels for reviewing the patch. On Fri, Dec 8, 2017 at 2:54 PM, Tels <nospam-pg-ab...@bloodgate.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 <p...@bowt.ie> wrote: > > > >> On Thu, Dec 7, 2017 at 12:25 AM, Rushabh Lathia > >> <rushabh.lat...@gmail.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
