RE: Conflict detection for update_deleted in logical replication

On Wednesday, November 20, 2024 8:05 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Thu, Nov 14, 2024 at 8:24 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)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