Re: Conflict detection for update_deleted in logical replication

Please find few more comments:

1)
ProcessStandbyPSRequestMessage:
+ /*
+ * This shouldn't happen because we don't support getting primary status
+ * message from standby.
+ */
+ if (RecoveryInProgress())
+ elog(ERROR, "the primary status is unavailable during recovery");

a) This error is not clear. Is it supposed to be user oriented error
or internal error? As a user, it is difficult to interpret this error
and take some action.

b) What I understood is that there is no user of enabling
'retain_conflict_info' for a subscription which is subscribing to a
publisher which is physical standby too. So shall we emit such an
ERROR during 'Create Sub(retain_conflict_info=on)' itself? But that
would need checking whether the publisher is physical standby or not
during CREATE-SUB. Is that a possibility?

2)

----------
send_feedback(last_received, requestReply, requestReply);

+ maybe_advance_nonremovable_xid(&data, false);
+
/*
* Force reporting to ensure long idle periods don't lead to
* arbitrarily delayed stats. Stats can only be reported outside
----------

Why do we need this call to 'maybe_advance_nonremovable_xid' towards
end of LogicalRepApplyLoop() i.e. the last call? Can it make any
further 'data.phase' change here? IIUC, there are 2 triggers for
'data.phase' change through LogicalRepApplyLoop(). First one is for
the very first time when we start this loop, it will set data.phase to
0 (RCI_GET_CANDIDATE_XID) and will call
maybe_advance_nonremovable_xid(). After that, LogicalRepApplyLoop()
function can trigger a 'data.phase' change only when it receives a
response from the publisher. Shouldn't the first 4 calls
to maybe_advance_nonremovable_xid() from LogicalRepApplyLoop() suffice?

3)
Code is almost the same for GetOldestActiveTransactionId() and
GetOldestTransactionIdInCommit(). I think we can merge these two.
GetOldestActiveTransactionId() can take new arg "getTxnInCommit".
GetOldestTransactionIdInCommit() can call
GetOldestActiveTransactionId() with that arg as true, whereas other 2
callers can pass it as false.

thanks
Shveta