On Sat, Nov 13, 2021 at 2:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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().
> >
Thank you for the comment!
> > 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.
I agree that it's simpler.
I thought statusFlags and xmin are conceptually separate things since
PROC_VACUUM_FOR_WRAPAROUND is not related to xid at all for example.
But given that the use case of copying statusFlags from someone is
only parallel worker startup for now, copying statusFlags while
setting xmin seems convenient and simple. If we want to only copy
statusFlags in some use cases in the future, we can have a separate
function for that.
> >
> > 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.
For the idea of is_vacuum flag, we don't know if a parallel worker is
launched for parallel vacuum at the time of ParallelWorkerMain().
> >
>
> 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.
Agreed.
I've updated the patch so that ProcArrayInstallRestoredXmin() sets
both xmin and statusFlags only when the source proc is still running
and xmin doesn't go backwards. IOW it doesn't happen that only one of
them is set by this function, which seems more understandable
behavior.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
