On Fri, Nov 12, 2021 at 6:44 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > 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. >
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.
