On Fri, Nov 5, 2021 at 8:16 PM Matthias van de Meent <boekewurm+postg...@gmail.com> wrote: > > On Fri, 22 Oct 2021 at 07:38, Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvhe...@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? > > > > That's wrong, I'd misunderstood. > > > > I agree to copy statusFlags in ProcArrayInstallRestoredXmin(). I've > > updated the patch accordingly. > > I've tested this patch, and it correctly fixes the issue of blocking > xmin from advancing, and also fixes an issue of retreating the > observed *_oldest_nonremovable in XidHorizons through parallel > workers. > > There are still some other soundness issues with xmin handling (see > [0]), but that should not prevent this patch from landing in the > relevant branches. >
AFAICU, in the thread referred by you, it seems that the main reported issue will be resolved by this patch but there is a discussion about xmin moving backward which seems to be the case with the current code as per code comments mentioned by Andres. Is my understanding correct? -- With Regards, Amit Kapila.
