On Mon, Jan 4, 2021 at 10:48 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > 7. > @@ -905,7 +905,7 @@ replorigin_advance(RepOriginId node, > LWLockAcquire(&replication_state->lock, LW_EXCLUSIVE); > > /* Make sure it's not used by somebody else */ > - if (replication_state->acquired_by != 0) > + if (replication_state->acquired_by != 0 && > replication_state->acquired_by != MyProcPid) > { > > I think you won't need this change if you do replorigin_advance before > replorigin_session_setup in your patch. >
As you know the replorigin_session_setup sets the replication_state->acquired_by to be the current PID. So without this change the replorigin_advance rejects that same slot state thinking that it is already active for a different process. Root problem is that the same process/PID calling both functions would hang. So this patch change allows replorigin_advance code to be called by self. IIUC that acquired_by check condition is like a sanity check for the originid passed. The patched code only does just like what the comment says: "/* Make sure it's not used by somebody else */" Doesn't "somebody else" means "anyone but me" (i.e. anyone but MyProcPid). Also, “setup” of a thing generally comes before usage of that thing, so won't it seem strange to do (like the suggestion) and deliberately call the "setup" function 2nd instead of 1st? Can you please explain why is it better to do it the suggested way (switch the calls around) than keep the patch code? Probably there is a good reason but I am just not understanding it. ---- Kind Regards, Peter Smith. Fujitsu Australia