Re: Conflict detection for update_deleted in logical replication

Hi Hou-san, here are my review comments for patch v5-0001.

======
General

1.
Sometimes in the commit message and code comments the patch refers to
"transaction id" and other times to "transaction ID". The patch should
use the same wording everywhere.

======
Commit message.

2.
"While for concurrent remote transactions with earlier timestamps,..."

I think this means:
"But, for concurrent remote transactions with earlier timestamps than
the DELETE,..."

Maybe expressed this way is clearer.

~~~

3.
... the resolution would be to convert the update to an insert.

Change this to uppercase like done elsewhere:
"... the resolution would be to convert the UPDATE to an INSERT.

======
doc/src/sgml/protocol.sgml

4.
+ <varlistentry
id="protocol-replication-primary-wal-status-update">
+ <term>Primary WAL status update (B)</term>
+ <listitem>
+ <variablelist>
+ <varlistentry>
+ <term>Byte1('s')</term>
+ <listitem>
+ <para>
+ Identifies the message as a primary WAL status update.
+ </para>
+ </listitem>
+ </varlistentry>

I felt it would be better if this is described as just a "Primary
status update" instead of a "Primary WAL status update". Doing this
makes it more flexible in case there is a future requirement to put
more status values in here which may not be strictly WAL related.

~~~

5.
+ <varlistentry id="protocol-replication-standby-wal-status-request">
+ <term>Standby WAL status request (F)</term>
+ <listitem>
+ <variablelist>
+ <varlistentry>
+ <term>Byte1('W')</term>
+ <listitem>
+ <para>
+ Identifies the message as a request for the WAL status
on the primary.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </listitem>
+ </varlistentry>

5a.
Ditto the previous comment #4. Perhaps you should just call this a
"Primary status request".

~

5b.
Also, The letter 'W' also seems chosen because of WAL. But it might be
more flexible if those identifiers are more generic.

e.g.
's' = the request for primary status update
'S' = the primary status update

======
src/backend/replication/logical/worker.c

6.
+ else if (c == 's')
+ {
+ TimestampTz timestamp;
+
+ remote_lsn = pq_getmsgint64(&s);
+ timestamp = pq_getmsgint64(&s);
+
+ maybe_advance_nonremovable_xid(&remote_lsn, &phase);
+ UpdateWorkerStats(last_received, timestamp, false);
+ }

Since there's no equivalent #define or enum value, IMO it is too hard
to know the intent of this code without already knowing the meaning of
the magic letter 's'. At least there could be a comment here to
explain that this is for handling an incoming "Primary status update"
message.

~~~

maybe_advance_nonremovable_xid:

7.
+ * The oldest_nonremovable_xid is maintained in shared memory to prevent dead
+ * rows from being removed prematurely when the apply worker still needs them
+ * to detect update-delete conflicts.

/update-delete/update_deleted/

~

8.
+ * applied and flushed locally. The process involves:
+ *
+ * DTR_REQUEST_WALSENDER_WAL_POS - Call GetOldestActiveTransactionId() to get
+ * the candidate xmin and send a message to request the remote WAL position
+ * from the walsender.
+ *
+ * DTR_WAIT_FOR_WALSENDER_WAL_POS - Wait for receiving the WAL position from
+ * the walsender.
+ *
+ * DTR_WAIT_FOR_LOCAL_FLUSH - Advance the non-removable transaction ID if the
+ * current flush location has reached or surpassed the received WAL position.

8a.
This part would be easier to read if those 3 phases were indented from
the rest of this function comment.

~

8b.
/Wait for receiving/Wait to receive/

~

9.
+ * Retaining the dead tuples for this period is sufficient for ensuring
+ * eventual consistency using last-update-wins strategy, which involves
+ * converting an UPDATE to an INSERT and applying it if remote transactions

The commit message referred to a "latest_timestamp_wins". I suppose
that is the same as what this function comment called
"last-update-wins". The patch should use consistent terminology.

It would be better if the commit message and (parts of) this function
comment were just cut/pasted to be identical. Currently, they seem to
be saying the same thing, but using slightly different wording.

~

10.
+ static TimestampTz xid_advance_attemp_time = 0;
+ static FullTransactionId candidate_xid;

typo in var name - "attemp"

~

11.
+ *phase = DTR_WAIT_FOR_LOCAL_FLUSH;
+
+ /*
+ * Do not return here because the apply worker might have already
+ * applied all changes up to remote_wal_pos. Proceeding to the next
+ * phase to check if we can immediately advance the transaction ID.
+ */

11a.
IMO this comment should be above the *phase assignment.

11b.
/Proceeding to the next phase to check.../Instead, proceed to the next
phase to check.../

~

12.
+ /*
+ * Advance the non-removable transaction id if the remote wal position
+ * has been received, and all transactions up to that position on the
+ * publisher have been applied and flushed locally.
+ */

Some minor re-wording would help clarify this comment.

SUGGESTION
Reaching here means the remote wal position has been received, and all
transactions up to that position on the
publisher have been applied and flushed locally. So, now we can
advance the non-removable transaction id.

~

13.
+ *phase = DTR_REQUEST_WALSENDER_WAL_POS;
+
+ /*
+ * Do not return here as enough time might have passed since the last
+ * wal position request. Proceeding to the next phase to determine if
+ * we can send the next request.
+ */

13a.
IMO this comment should be above the *phase assignment.

~

13b.
This comment should have the same wording here as in the previous
if-block (see #11b).

/Proceeding to the next phase to determine.../Instead, proceed to the
next phase to check.../

~

14.
+ FullTransactionId next_full_xix;
+ FullTransactionId full_xid;

You probably mean 'next_full_xid' (not xix)

~

15.
+ /*
+ * Exit early if the user has disabled sending messages to the
+ * publisher.
+ */
+ if (wal_receiver_status_interval <= 0)
+ return;

What are the implications of this early exit? If the update request is
not possible, then I guess the update status is never received, but
then I suppose that means none of this update_deleted logic is
possible. If that is correct, then will there be some documented
warning/caution about conflict-handling implications by disabling that
GUC?

======
src/backend/replication/walsender.c

16.
+/*
+ * Process the standby message requesting the latest WAL write position.
+ */
+static void
+ProcessStandbyWalPosRequestMessage(void)

Ideally, this function comment should be referring to this message we
are creating by the same name that it was called in the documentation.
For example something like:

"Process the request for a primary status update message."

======
Kind Regards,
Peter Smith.
Fujitsu Australia