On Fri, 22 Oct 2021 at 07:38, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > 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?
>
> 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.
Kind regards,
Matthias van de Meent
[0] https://www.postgresql.org/message-id/flat/17257-1e46de26bec11433%40postgresql.org