Re: Parallel vacuum workers prevent the oldest xmin from advancing

On Fri, Nov 5, 2021 at 8:16 PM Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Fri, 22 Oct 2021 at 07:38, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Oct 20, 2021 at 9:27 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Oct 20, 2021 at 3:07 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.