Sorry for the delay in the another version of patch.
On Tue, Nov 14, 2017 at 11:31 PM, Peter Geoghegan <pg@bowt.ie> wrote:
On Tue, Nov 14, 2017 at 1:41 AM, Rushabh Lathia <rushabh.lathia@gmail.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.