On 2021-Oct-19, Alvaro Herrera wrote:
> 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.)
(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.)
Initially, I was a bit nervous about copying flags willy-nilly. Do we
need to be more careful? I mean, have a way for the code to specify
flags to copy, maybe something like
MyProc->statusFlags |= proc->statusFlags & copyableFlags;
ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
with this coding,
1. we do not unset flags that the bgworker already has for whatever
reason
2. we do not copy flags that may be unrelated to the effect we desire.
The problem, and it's something I don't have an answer for, is how to
specify copyableFlags. This code is the generic ParallelWorkerMain()
and there's little-to-no chance to pass stuff from the process that
requested the bgworker. So maybe Sawada-san's original coding of just
copying everything is okay.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/