RE: Conflict detection for update_deleted in logical replication

On Tue, May 20, 2025 at 11:08 AM shveta malik wrote:
>
> Please find few more comments:

Thanks for the comments!

>
> 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?

I think each invocation of maybe_advance_nonremovable_xid() has a chance to
complete the final RCI_WAIT_FOR_LOCAL_FLUSH phase, as it could be waiting for
changes to be flushed. The function call was added with the intention to
enhance the likelihood of advancing the transaction ID, particularly when it is
waiting for flushed changes. Although we could check the same in other func
calls as well, but I think it's OK to keep the last check.

Best Regards,
Hou zj