Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
Thomas Munro <thomas.munro@enterprisedb.com>
On Fri, Jan 26, 2018 at 7:30 PM, Peter Geoghegan <pg@bowt.ie> wrote: > On Thu, Jan 25, 2018 at 10:00 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> However, now I see that you and Thomas are trying to find a different >> way to overcome this problem differently, so not sure if I should go >> ahead or not. I have seen that you told you wanted to look at >> Thomas's proposed stuff carefully tomorrow, so I will wait for you >> guys to decide which way is appropriate. > > I suspect that the overhead of Thomas' experimental approach is going > to causes problems in certain cases. Cases that are hard to foresee. > That patch makes HandleParallelMessages() set ParallelMessagePending > artificially, pending confirmation of having launched all workers. > > It was an interesting experiment, but I think that your > WaitForParallelWorkersToAttach() idea has a better chance of working > out. Thanks for looking into this. Yeah. I think you're right that it could add a bit of overhead in some cases (ie if you receive a lot of signals that AREN'T caused by fork failure, then you'll enter HandleParallelMessage() every time unnecessarily), and it does feel a bit kludgy. The best idea I have to fix that so far is like this: (1) add a member fork_failure_count to struct BackgroundWorkerArray, (2) in do_start_bgworker() whenever fork fails, do ++BackgroundWorkerData->fork_failure_count (ie before a signal is sent to the leader), (3) in procsignal_sigusr1_handler where we normally do a bunch of CheckProcSignal(PROCSIG_XXX) stuff, if (BackgroundWorkerData->fork_failure_count != last_observed_fork_failure_count) HandleParallelMessageInterrupt(). As far as I know, as long as fork_failure_count is (say) int32 (ie not prone to tearing) then no locking is required due to the barriers implicit in the syscalls involved there. This is still slightly more pessimistic than it needs to be (the failed fork may be for someone else's ParallelContext), but only in rare cases so it would be practically as good as precise PROCSIG delivery. It's just that we aren't allowed to deliver PROCSIGs from the postmaster. We are allowed to communicate through BackgroundWorkerData, and there is a precedent for cluster-visible event counters in there already. I think you should proceed with Amit's plan. If we ever make a plan like the above work in future, it'd render that redundant by turning every CFI() into a cancellation point for fork failure, but I'm not planning to investigate further given the muted response to my scheming in this area so far. -- Thomas Munro http://www.enterprisedb.com
Commits
-
Support parallel btree index builds.
- 9da0cc35284b 11.0 landed
-
Report an ERROR if a parallel worker fails to start properly.
- 2badb5afb89c 11.0 cited
-
Transfer state pertaining to pending REINDEX operations to workers.
- 29d58fd3adae 11.0 landed
-
Add a barrier primitive for synchronizing backends.
- 1145acc70deb 11.0 cited
-
Allow DML commands that create tables to use parallel query.
- e9baa5e9fa14 11.0 cited
-
Refactor GetOldestXmin() to use flags
- af4b1a0869bd 10.0 cited
-
Fix regression in parallel planning against inheritance tables.
- 2609e91fcf9d 10.0 cited
-
Don't create "holes" in BufFiles, in the new logtape code.
- 7ac4a389a7db 10.0 cited
-
Simplify the code for logical tape read buffers.
- b75f467b6eec 10.0 cited
-
Fix excessive memory consumption in the new sort pre-reading code.
- b56fb691b003 10.0 cited
-
Implement binary heap replace-top operation in a smarter way.
- 24598337c8d2 10.0 cited
-
Cosmetic code cleanup in commands/extension.c.
- 25794e841e5b 10.0 cited
-
Speed up planner's scanning for parallel-query hazards.
- da1c91631e35 10.0 cited
-
Read from the same worker repeatedly until it returns no tuple.
- bc7fcab5e36b 9.6.0 cited
-
Improve tuplesort.c to support variable merge order. The original coding
- df700e6b4019 8.2.0 cited