Re: Conflict detection for update_deleted in logical replication

On Mon, May 26, 2025 at 12:46 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Attaching the V32 patch set which addressed comments in [1]~[5].

Thanks for the patch, I am still reviewing the patches, please find
few trivial comments for patch001:

1)

+ FullTransactionId last_phase_at; /* publisher transaction ID that must
+ * be awaited to complete before
+ * entering the final phase
+ * (RCI_WAIT_FOR_LOCAL_FLUSH) */

'last_phase_at' seems like we are talking about the phase in the past.
(similar to 'last' in last_recv_time).
Perhaps we should name it as 'final_phase_at'

2)
RetainConflictInfoData data = {0};

We can change this name as well to rci_data.

3)
+ /*
+ * Compute FullTransactionId for the oldest running transaction ID. This
+ * handles the case where transaction ID wraparound has occurred.
+ */
+ full_xid = FullTransactionIdFromAllowableAt(next_full_xid,
oldest_running_xid);

Shall we name it to full_oldest_xid for better clarity?

4)
+ /*
+ * Update and check the remote flush position if we are applying changes
+ * in a loop. This is done at most once per WalWriterDelay to avoid
+ * performing costy operations in get_flush_position() too frequently
+ * during change application.
+ */
+ if (last_flushpos < rci_data->remote_lsn && rci_data->last_recv_time &&
+ TimestampDifferenceExceeds(rci_data->flushpos_update_time,
+ rci_data->last_recv_time, WalWriterDelay))

a) costy --> costly

thanks
Shveta