Horton

RE: Conflict detection for update_deleted in logical replication

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

View thread View on postgr.es
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Nisha Moond <nisha.moond412@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-01-07T12:49:55Z
Lists: pgsql-hackers 
On Thursday, January 2, 2025 2:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> Sounds reasonable but OTOH, all other places that create physical
> slots (which we are doing here) don't use this trick. So, don't they
> need similar reliability?

I have not figured the reason for existing physical slots' handling,
but will think more.

> Also, add some comments as to why we are
> initially creating the RS_EPHEMERAL slot as we have at other places.

Added.

> 
> Few other comments on 0003
> =======================
> 1.
> + if (sublist)
> + {
> + bool updated;
> +
> + if (!can_advance_xmin)
> + xmin = InvalidFullTransactionId;
> +
> + updated = advance_conflict_slot_xmin(xmin);
> 
> How will it help to try advancing slot_xmin when xmin is invalid?

It was intended to create the slot without updating the xmin in this case,
but the function name seems misleading. So, I will think more on this and
modify it in next version because it may also be affected by the discussion
in [1].

> 
> 2.
> @@ -1167,14 +1181,43 @@ ApplyLauncherMain(Datum main_arg)
>   long elapsed;
> 
>   if (!sub->enabled)
> + {
> + can_advance_xmin = false;
> 
> In ApplyLauncherMain(), if one of the subscriptions is disabled (say
> the last one in sublist), then can_advance_xmin will become false in
> the above code. Now, later, as quoted in comment-1, the patch
> overrides xmin to InvalidFullTransactionId if can_advance_xmin is
> false. Won't that lead to the wrong computation of xmin?

advance_conflict_slot_xmin() would skip updating the slot.xmin
if the input value is invalid. But I will think how to improve this
in next version.

> 
> 3.
> + slot_maybe_exist = true;
> + }
> +
> + /*
> + * Drop the slot if we're no longer retaining dead tuples.
> + */
> + else if (slot_maybe_exist)
> + {
> + drop_conflict_slot_if_exists();
> + slot_maybe_exist = false;
> 
> Can't we use MyReplicationSlot instead of introducing a new boolean
> slot_maybe_exist?
> 
> In any case, how does the above code deal with the case where the
> launcher is restarted for some reason and there is no subscription
> after that? Will it be possible to drop the slot in that case?

Since the initial value of slot_maybe_exist is true, so I think the launcher would
always check the slot once and drop the slot if not needed even if the
launcher restarted.

[1] https://www.postgresql.org/message-id/CAA4eK1Li8XLJ5f-pYvPJ8pXxyA3G-QsyBLNzHY940amF7jm%3D3A%40mail.gmail.com

Best Regards,
Hou zj

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.