On 2021-Nov-11, Masahiko Sawada wrote: > On Thu, Nov 11, 2021 at 12:53 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Thu, Nov 11, 2021 at 9:11 AM Andres Freund <and...@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/
