Horton

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

Rushabh Lathia <rushabh.lathia@gmail.com>

View thread View on postgr.es
From: Rushabh Lathia <rushabh.lathia@gmail.com>
To: Tels <nospam-pg-abuse@bloodgate.com>
Cc: Peter Geoghegan <pg@bowt.ie>, Thomas Munro <thomas.munro@enterprisedb.com>, Robert Haas <robertmhaas@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Pg Hackers <pgsql-hackers@postgresql.org>, Corey Huinker <corey.huinker@gmail.com>
Date: 2017-12-11T04:49:06Z
Lists: pgsql-hackers 
Thanks Tels for reviewing the patch.

On Fri, Dec 8, 2017 at 2:54 PM, Tels <nospam-pg-abuse@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 <pg@bowt.ie> wrote:
> >
> >> On Thu, Dec 7, 2017 at 12:25 AM, Rushabh Lathia
> >> <rushabh.lathia@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

Commits

  1. Support parallel btree index builds.

  2. Report an ERROR if a parallel worker fails to start properly.

  3. Transfer state pertaining to pending REINDEX operations to workers.

  4. Add a barrier primitive for synchronizing backends.

  5. Allow DML commands that create tables to use parallel query.

  6. Refactor GetOldestXmin() to use flags

  7. Fix regression in parallel planning against inheritance tables.

  8. Don't create "holes" in BufFiles, in the new logtape code.

  9. Simplify the code for logical tape read buffers.

  10. Fix excessive memory consumption in the new sort pre-reading code.

  11. Implement binary heap replace-top operation in a smarter way.

  12. Cosmetic code cleanup in commands/extension.c.

  13. Speed up planner's scanning for parallel-query hazards.

  14. Read from the same worker repeatedly until it returns no tuple.

  15. Improve tuplesort.c to support variable merge order. The original coding