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: Nisha Moond <nisha.moond412@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2024-11-21T09:33:04Z
Lists: pgsql-hackers

Attachments

On Wednesday, November 20, 2024 8:05 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> 
> On Thu, Nov 14, 2024 at 8:24 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Attach the V9 patch set which addressed above comments.
> >
> 
> Reviewed v9 patch-set and here are my comments for below changes:

Thanks for the comments.

> 
> @@ -1175,10 +1189,29 @@ ApplyLauncherMain(Datum main_arg)
...
> + if (can_advance_xmin && w != NULL)
> + {
> + FullTransactionId nonremovable_xid;
> +
> + SpinLockAcquire(&w->relmutex);
> + nonremovable_xid = w->oldest_nonremovable_xid;
> + SpinLockRelease(&w->relmutex);
> +
> + if (!FullTransactionIdIsValid(xmin) ||
> + !FullTransactionIdIsValid(nonremovable_xid) ||
> + FullTransactionIdPrecedes(nonremovable_xid, xmin))
> + xmin = nonremovable_xid;
> + }
> +
> 
> 1) In Patch-0002, could you please add a comment above "+ if
> (can_advance_xmin && w != NULL)" to briefly explain the purpose of
> finding the minimum XID at this point?

Sure, added.

> 
> 2) In Patch-0004, with the addition of the 'detect_update_deleted'
> option, I see the following two issues in the above code:
> 2a) Currently, all enabled subscriptions are considered when comparing
> and finding the minimum XID, even if detect_update_deleted is disabled
> for some subscriptions.
> I suggest excluding the oldest_nonremovable_xid of subscriptions where
> detect_update_deleted=false by updating the check as follows:
> 
>     if (sub->detectupdatedeleted && can_advance_xmin && w != NULL)

Right, that's a miss. Fixed in V10 patch set.

> 
> 2b) I understand why advancing xmin is not allowed when a subscription
> is disabled. However, the current check allows a disabled subscription
> with detect_update_deleted=false to block xmin advancement, which
> seems incorrect. Should the check also account for
> detect_update_deleted?, like :
>   if (sub->detectupdatedeleted &&  !sub->enabled)
> + {
> + can_advance_xmin = false;
> + xmin = InvalidFullTransactionId;
>   continue;
> + }

Yes, I think we should add this check.

> 
> However, I'm not sure if this is the right fix, as it could lead to
> inconsistencies if the detect_update_deleted is set to false after
> disabling the sub.
> Thoughts?

I think it's OK as it doesn't affect the functionality. 


Attach the V10 patch set which addressed above comments
and fixed a CFbot warning due to un-initialized variable.

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.