Re: Parallel vacuum workers prevent the oldest xmin from advancing

On Fri, Nov 12, 2021 at 6:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Nov-11, Masahiko Sawada 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:
>
> > > > 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.
> >
> > This makes me think that it'd be better to copy status flags in a
> > separate function rather than ProcArrayInstallRestoredXmin().
>
> To me, and this is perhaps just personal opinion, it seems conceptually
> simpler to have ProcArrayInstallRestoredXmin acquire exclusive and do
> both things.  Why?  Because if you have two functions, you have to be
> careful not to call the new function after ProcArrayInstallRestoredXmin;
> otherwise you would create an instant during which you make an
> Xmin-without-flag visible to other procs; this causes the computed xmin
> go backwards, which is verboten.
>
> If I understand Amit correctly, his point is about the callers of
> RestoreTransactionSnapshot, which are two: CreateReplicationSlot and
> ParallelWorkerMain.  He wants you hypothetical new function called from
> the latter but not the former.  Looking at both, it seems a bit strange
> to make them responsible for a detail such as "copy ->statusFlags from
> source proc to mine".  It seems more reasonable to add a third flag to
>   RestoreTransactionSnapshot(Snapshot snapshot, void *source_proc, bool is_vacuum)
> and if that is true, tell SetTransactionSnapshot to copy flags,
> otherwise not.
>

If we decide to go this way then I suggest adding a comment to convey
why we choose to copy status flags in ProcArrayInstallRestoredXmin()
as the function name doesn't indicate it.

>
> ... unrelated to this, and looking at CreateReplicationSlot, I wonder
> why does it pass MyProc as the source_pgproc parameter.  What good is
> that doing?  I mean, if the only thing we do with source_pgproc is to
> copy stuff from source_pgproc to MyProc, then if source_pgproc is
> MyProc, we're effectively doing nothing at all.  (You can't "fix" this
> by merely passing NULL, because what that would do is change the calling
> of ProcArrayInstallRestoredXmin into a call of
> ProcArrayInstallImportedXmin and that would presumably have different
> behavior.)  I may be misreading the code of course, but it sounds like
> the intention of CreateReplicationSlot is to "do nothing" with the
> transaction snapshot in a complicated way.
>

It ensures that the source transaction is still running, otherwise, it
won't allow the import to be successful. It also seems to help by
updating the state for GlobalVis* stuff. I think in the current form
it seems to help in not moving MyProc-xmin and TransactionXmin
backward due to checks in ProcArrayInstallRestoredXmin() and also
change them to the value in source snapshot.

-- 
With Regards,
Amit Kapila.