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/
