Re: Parallel vacuum workers prevent the oldest xmin from advancing - Mailing list pgsql-hackers

On Thu, Nov 11, 2021 at 10:40 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Hi,
> > >
> > > On 2021-11-11 12:22:42 +0900, Masahiko Sawada wrote:
> > > > > 2.
> > > > >   LWLockAcquire(ProcArrayLock, LW_SHARED);
> > > > >
> > > > > + flags = proc->statusFlags;
> > > > > +
> > > > > + /*
> > > > > + * If the source xact has any statusFlags, we re-grab ProcArrayLock
> > > > > + * on exclusive mode so we can copy it to MyProc->statusFlags.
> > > > > + */
> > > > > + if (flags != 0)
> > > > > + {
> > > > > + LWLockRelease(ProcArrayLock);
> > > > > + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
> > > > > + }
> > > > >
> > > > >
> > > > > This looks a bit odd to me. It would have been better if we know when
> > > > > to acquire an exclusive lock without first acquiring the shared lock.
> > > >
> > > > I think we should acquire an exclusive lock only if status flags are
> > > > not empty. But to check the status flags we need to acquire a shared
> > > > lock. No?
> > >
> > > This seems like an unnecessary optimization. ProcArrayInstallRestoredXmin()
> > > only happens in the context of much more expensive operations.
> > >
> >
> > Fair point. I think that will also make the change in
> > ProcArrayInstallRestoredXmin() appear neat.
>
> Agreed.
>
> This makes me think that it'd be better to copy status flags in a
> separate function rather than ProcArrayInstallRestoredXmin(). The
> current patch makes use of the fact that ProcArrayInstallRestoedXmin()
> acquires a shared lock in order to check the source's status flags.
> But if we can acquire an exclusive lock unconditionally in this
> context, it’s clearer to do in a separate function.
>

Do you mean to say that do it in a separate function and call
immediately after StartParallelWorkerTransaction or do you mean to do
it in a separate function and invoke it from
ProcArrayInstallRestoedXmin()? I think the disadvantage I see by not
doing in ProcArrayInstallRestoedXmin is that we need to take procarray
lock twice (once in exclusive mode and then in shared mode) so doing
it in ProcArrayInstallRestoedXmin is beneficial from that angle. The
main reason why I was not very happy with the last patch was due to
releasing and reacquiring the lock but if we directly acquire it in
exclusive mode then that shouldn't be a problem. OTOH, doing it via a
separate function is also not that bad.

> >
> > > I think it might be worth asserting that the set of flags we're copying is a
> > > known subset of the flags that are valid to copy from the source.
> > >
> >
> > Sounds reasonable.
>
> +1
>
> Regards,
>
> --
> Masahiko Sawada
> EDB:  https://www.enterprisedb.com/



--
With Regards,
Amit Kapila.