Re: Single transaction in the tablesync worker?

On Mon, Jan 4, 2021 at 10:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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