On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Oct-19, Alvaro Herrera wrote:
>
Thank you for the comment.
> > Hmm, I think this should happen before the transaction snapshot is
> > established in the worker; perhaps immediately after calling
> > StartParallelWorkerTransaction(), or anyway not after
> > SetTransactionSnapshot. In fact, since SetTransactionSnapshot receives
> > a 'sourceproc' argument, why not do it exactly there? ISTM that
> > ProcArrayInstallRestoredXmin() is where this should happen.
>
> ... and there is a question about the lock strength used for
> ProcArrayLock. The current routine uses LW_SHARED, but there's no
> clarity that we can modify proc->statusFlags and ProcGlobal->statusFlags
> without LW_EXCLUSIVE.
>
> Maybe we can change ProcArrayInstallRestoredXmin so that if it sees that
> proc->statusFlags is not zero, then it grabs LW_EXCLUSIVE (and copies),
> otherwise it keeps using LW_SHARED as it does now (and does not copy.)
Initially, I've considered copying statusFlags in
ProcArrayInstallRestoredXmin() but I hesitated to do that because
statusFlags is not relevant with xmin and snapshot stuff. But I agree
that copying statusFlags should happen before restoring the snapshot.
If we copy statusFlags in ProcArrayInstallRestoredXmin() there is
still little window that the restored snapshot holds back the oldest
xmin? If so it would be better to call ProcArrayCopyStatusFlags()
right after StartParallelWorker().
> (This also suggests that using LW_EXCLUSIVE inconditionally for all
> cases as your patch does is not great. OTOH it's just once at every
> bgworker start, so it's not *that* frequent.)
Agreed.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/