On Sat, Jan 6, 2018 at 3:47 AM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Tue, Jan 2, 2018 at 8:43 PM, Rushabh Lathia <rushabh.lat...@gmail.com>
> wrote:
>> I agree that plan_create_index_workers() needs to count the leader as a
>> normal worker for the CREATE INDEX. So what you proposing is - when
>> parallel_leader_participation is true launch (return value of
>> compute_parallel_worker() - 1)
>> workers. true ?
>
> Almost. We need to not subtract one when only one worker is indicated
> by compute_parallel_worker(). I also added some new stuff there, to
> consider edge cases with the parallel_leader_participation GUC.
>
>>> I'm working on fixing up what you posted. I'm probably not more than a
>>> week away from posting a patch that I'm going to mark "ready for
>>> committer". I've already made the change above, and once I spend time
>>> on trying to break the few small changes needed within buffile.c I'll
>>> have taken it as far as I can, most likely.
>>>
>>
>> Okay, once you submit the patch with changes - I will do one round of
>> review for the changes.
>
> I've attached my revision. Changes include:
>
Few observations while skimming through the patch:
1.
+ if (!IsBootstrapProcessingMode() && !indexInfo->ii_Concurrent)
{
- snapshot = RegisterSnapshot(GetTransactionSnapshot());
- OldestXmin = InvalidTransactionId; /* not used */
+ OldestXmin = GetOldestXmin(heapRelation, true);
I think leader and workers should have the same idea of oldestXmin for
the purpose of deciding the visibility of tuples. I think this is
ensured in all form of parallel query as we do share the snapshot,
however, same doesn't seem to be true for Parallel Index builds.
2.
+
+ /* Wait on worker processes to finish (should be almost instant) */
+ reltuples = _bt_leader_wait_for_workers(buildstate);
Can't we use WaitForParallelWorkersToFinish for this purpose? The
reason is that if we use a different mechanism here then we might need
a different way to solve the problem related to fork failure. See
thread [1]. Basically, what if postmaster fails to launch workers due
to fork failure, the leader backend might wait indefinitely.
[1] - https://commitfest.postgresql.org/16/1341/
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
