Re: Conflict detection for update_deleted in logical replication

Thanks you for v31 patch-set. Please find few comments on patch001:

1)

wait_for_local_flush:

+ if (data->last_recv_time &&
+ TimestampDifferenceExceeds(data->flushpos_update_time,
+ data->last_recv_time, WalWriterDelay))
+ {
+ XLogRecPtr writepos;
+ XLogRecPtr flushpos;
+ bool have_pending_txes;
+
+ /* Fetch the latest remote flush position */
+ get_flush_position(&writepos, &flushpos, &have_pending_txes);
+
+ if (flushpos > last_flushpos)
+ last_flushpos = flushpos;
+
+ data->flushpos_update_time = data->last_recv_time;
+ }

We should only get new flush-position, if 'last_flushpos' is still
lesser than 'data->remote_lsn'. Since 'last_flushpos' is also updated
by 'send_feedback' and we do not update 'data->flushpos_update_time'
there, it is possible that we have latest flush position but still
TimestampDifferenceExceeds gives 'true', making it re-read the flush
position unnecessarily.

Having said that, I think the correct way will be to move
'flushpos_update_time' out of RetainConflictInfoData() similar to
last_flushpos. Let it be a static variable, then we can update it in
send_feedback as well.

2)

get_candidate_xid:

+ /* Return if the oldest_nonremovable_xid cannot be advanced */
+ if (FullTransactionIdFollowsOrEquals(MyLogicalRepWorker->oldest_nonremovable_xid,
+ full_xid))
+ {
+ adjust_xid_advance_interval(data, false);
+ return;
+ }

I fail to think of a scenario where oldest_nonremovable_xid can be
greater than current oldest txn-id. I think we should only check for
'equal' in the above condition and assert if oldest_nonremovable_xid
is greater.

3)

+ elog(DEBUG2, "confirmed remote flush up to %X/%X: new
oldest_nonremovable_xid %u",
+ LSN_FORMAT_ARGS(data->remote_lsn),
+ XidFromFullTransactionId(data->candidate_xid));

This message is confusing as we have not confirmed remote flush,
instead it is local flush upton remote-lsn. We can say:
confirmed flush up to remote lsn %X/%X: new oldest_nonremovable_xid %u",

4)
Everywhere we are using variable name as 'data', it is a very generic
name. Shall we change it to 'conflict_info_data' or
'retain_conf_info_data'?

5)
+ * RecordTransactionCommit uses DELAY_CHKPT_IN_COMMIT for; see notes
+ * there.). Note that DELAY_CHKPT_IN_COMMIT is used to find transactions
+ * in the critical commit section. We need to know about such transactions
+ * for conflict detection in logical replication. See
+ * GetOldestTransactionIdInCommit and its use.

Please update the comment as 'GetOldestTransactionIdInCommit' is no
longer there.

6)
+ * RecordTransactionCommit uses DELAY_CHKPT_IN_COMMIT for; see notes
+ * there.). Note that DELAY_CHKPT_IN_COMMIT is used to find transactions

there.). --> there.) (another full stop not needed)

thanks
Shveta