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.
... 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.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/