On Wed, Jan 13, 2021 at 9:18 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 11:18 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > 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. > > > > I think the hang happens only if we call unchanged replorigin_advance > after session_setup API, right? > > > 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. > > > > Because there is no requirement for origin_advance API to be called > after session setup. Session setup is required to mark the node as > replaying from a remote node, see [1] whereas origin_advance is used > for setting up the initial location or setting a new location, see [2] > (pg_replication_origin_advance). > > Now here, after creating the origin, we need to set up the initial > location and it seems fine to call origin_advance before > session_setup. In short, as such, I don't see any problem with your > change in replorigin_advance but OTOH, I don't see the need for the > same as well. So, let's try to avoid that change unless we can't do > without it. > > Also, another thing is we need to take RowExclusiveLock on > pg_replication_origin as written in comments atop replorigin_advance > before calling it. See its usage in pg_replication_origin_advance. > Also, write comments on why we need to use replorigin_advance here > (... something, like we need to WAL log this for the purpose of > recovery...). >
Modified in latest patch [v15]. ---- [v15] = https://www.postgresql.org/message-id/CAHut%2BPu3he2rOWjbXcNUO6z3aH2LYzW03KV%2BfiMWim49qW9etQ%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia