Re: Parallel vacuum workers prevent the oldest xmin from advancing

On Fri, Nov 12, 2021 at 6:44 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2021-Nov-11, Masahiko Sawada wrote:
>
> > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <andres(at)anarazel(dot)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.