Re: Conflict detection for update_deleted in logical replication

On Fri, Nov 29, 2024 at 7:54 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> It is possible to reach here if user creates a subscription with
> (connect=false,detect_update_deleted=true), in which case we could not check it
> during creation. But I agree that it would be better to report an ERROR here,
> so changed as suggested. After this change, there is no need to check the
> invalid remote lsn in apply worker and thus the error can also be removed.
>

1.
if (XLogRecPtrIsInvalid(data.remote_lsn))
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot get the latest WAL position from the publisher"),
+ errdetail("The connected publisher is also a standby server."));
+

Instead of removing this message from the patch, we should change it
to elog(ERROR, category of ERROR.

2.
+ Timestamp xid_advance_attempt_time; /* when the candidate_xid is
+ * decided */

How about naming this variable as candidate_xid_time /* time when the
next candidate_xid is computed */?

3.
+ /* Return if the new transaction ID is unchanged */
+ if (FullTransactionIdFollowsOrEquals(MyLogicalRepWorker->oldest_nonremovable_xid,
+ full_xid))
+ return;

This comment is unclear. Can we change it to: "Return if the
oldest_nonremovable_xid can't be advanced" or something like that?

4.
+request_publisher_status(RetainConflictInfoData *data)
+{
+ if (!reply_message)
+ {
+ MemoryContext oldctx = MemoryContextSwitchTo(ApplyContext);
+
+ reply_message = makeStringInfo();
+ MemoryContextSwitchTo(oldctx);
+ }
+ else
+ resetStringInfo(reply_message);
+
+ pq_sendbyte(reply_message, 'S');
+ pq_sendint64(reply_message, GetCurrentTimestamp());
+
+ elog(DEBUG2, "sending publisher status request message");

The name 'reply_message' sounds confusing as this is a request
message. Can we change it to request_message? Also, let's avoid
reusing the same variable among different messages as it makes the
code unclear.

Apart from the above, I have modified a few comments after applying
0001 and 0002 in the attached.

--
With Regards,
Amit Kapila.