Horton

Re: Conflict detection for update_deleted in logical replication

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Nisha Moond <nisha.moond412@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-11-29T10:08:53Z
Lists: pgsql-hackers

Attachments

On Fri, Nov 29, 2024 at 7:54 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.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.

Commits

  1. Fix intermittent BF failures in 035_conflicts.

  2. Resume conflict-relevant data retention automatically.

  3. Fix intermittent test failure introduced in 6456c6e2c4.

  4. Fix Coverity issue reported in commit a850be2fe.

  5. Add test to prevent premature removal of conflict-relevant data.

  6. Post-commit review fixes for 228c370868.

  7. Add max_retention_duration option to subscriptions.

  8. Detect and report update_deleted conflicts.

  9. Preserve conflict-relevant data during logical replication.

  10. Integrate FullTransactionIds deeper into two-phase code

  11. Improve checks for GUC recovery_target_timeline

  12. Prevent excessive delays before launching new logrep workers.

  13. Keep WAL segments by slot's last saved restart LSN

  14. Rework some code handling pg_subscription data in psql and pg_dump

  15. Use generateClonedIndexStmt to propagate CREATE INDEX to partitions.