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

From: Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
To: Peter Geoghegan <pg(at)bowt(dot)ie>
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
Subject: Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
Date: 2017-12-07 08:25:06
Message-ID: CAGPqQf0YrYxi_Y=VJ6RexjMw3TE7Y9jHO6aDmrorUW3vTTF8kw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Sorry for the delay in the another version of patch.

On Tue, Nov 14, 2017 at 11:31 PM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:

> On Tue, Nov 14, 2017 at 1:41 AM, Rushabh Lathia
> <rushabh(dot)lathia(at)gmail(dot)com> wrote:
> > Thanks Peter and Thomas for the review comments.
>
> No problem. More feedback:
>
> * I don't really see much need for this:
>
> + elog(LOG, "Worker for create index %d", parallel_workers);
>
> You can just use trace_sort, and observe the actual behavior of the
> sort that way.
>
>
Right, that was just added for the testing purposed. Removed in the
latest version of the patch.

> * As I said before, you should remove the header comments within nbtsort.c.
>
>
Done.

> * This should just say "write routines":
>
> + * This is why write/recycle routines don't need to know about offsets at
> + * all.
>
>
Okay, done.

> * You didn't point out the randomAccess restriction in tuplesort.h.
>
>
I did, it's there in the file header comments.

> * I can't remember why I added the Valgrind suppression at this point.
> I'd remove it until the reason becomes clear, which may never happen.
> The regression tests should still pass without Valgrind warnings.
>
>
Make sense.

> * You can add back comments removed from above LogicalTapeTell(). I
> made these changes because it looked like we should close out the
> possibility of doing a tell during the write phase, as unified tapes
> actually would make that hard (no one does what it describes anyway).
> But now, unified tapes are a distinct case to frozen tapes in a way
> that they weren't before, so there is no need to make it impossible.
>
> I also think you should replace "Assert(lt->frozen)" with
> "Assert(lt->offsetBlockNumber == 0L)", for the same reason.
>
>
Yep, done.

I see that Robert just committed support for a
> parallel_leader_participation GUC. Parallel tuplesort should use this,
> too.
>
> It will be easy to adopt the patch to make this work. Just change the
> code within nbtsort.c to respect parallel_leader_participation, rather
> than leaving that as a compile-time switch. Remove the
> force_single_worker variable, and use !parallel_leader_participation
> in its place.
>
>
Added handling for parallel_leader_participation as well as deleted
compile time option force_single_worker.

> The parallel_leader_participation docs will also need to be updated.
>
>
Done.

Also performed more testing with the patch, with
parallel_leader_participation
ON and OFF. Found one issue, where earlier we always used to call
_bt_leader_sort_as_worker() but now need to skip the call if
parallel_leader_participation
is OFF.

Also fixed the documentation and the compilation error for the
documentation.

PFA v14 patch.

...
Thanks,
Rushabh Lathia
www.EnterpriseDB.com

Attachment Content-Type Size
0001-Add-parallel-B-tree-index-build-sorting_v14.patch text/x-patch 147.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Kukushkin 2017-12-07 09:31:10 Re: Speeding up pg_upgrade
Previous Message David Rowley 2017-12-07 08:14:29 Out of date comment in cached_plan_cost