RE: Conflict detection for update_deleted in logical replication

On Fri, Jun 13, 2025 at 2:52 PM shveta malik wrote:

>
> On Thu, Jun 12, 2025 at 4:22 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> wrote:
> >
> > On Thu, Jun 12, 2025 at 11:34 AM Zhijie Hou (Fujitsu)
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > >
> > > Here is the V35 patch set which includes the following changes:
> > >
> >
> > Thank You for the patches. Few comments:

Thanks for the comments!

> >
> > 1)
> > Since now we create slot for rci enabled subscription, it will require
> > wal_level >= replica even on subscribers. We shall mention this in the
> > docs.

Added.

> >
> > 2)
> > postgres=# alter subscription sub3 set (retain_conflict_info=true);
> > NOTICE: deleted rows will continue to accumulate for detecting
> > conflicts until the subscription is enabled ALTER SUBSCRIPTION
> >
> > postgres=# alter subscription sub3 disable;
> > WARNING: deleted rows to detect conflicts would not be removed until
> > the subscription is enabled
> > HINT: Consider setting retain_conflict_info to false.
> >
> > I feel we shall have the same message in both the cases as we are
> > trying to convey the exact same thing concerning deleted rows
> > accumulation..

Changed.

> >
>
> Few more comments:
>
> 3)
>
> +static void
> +create_conflict_slot_if_not_exists(void)
> +{
> - /* Exit early if the replication slot is already created and acquired */
> + /* Exit early, if the replication slot is already created and acquired
> + */
> if (MyReplicationSlot)
> return;
>
> - /* If the replication slot exists, acquire it and exit */
> + /* If the replication slot exists, acquire it, and exit */
> if (acquire_conflict_slot_if_exists())
> return;
>
> We never release the slot explicitly in the launcher and thus do not need 'If the
> replication slot exists, acquire it' code part here. For the cases 1) when slot is
> dropped and recreated 2) slot is acquired by launcher on restart; 'if
> (MyReplicationSlot)' check is enough.

Agreed. I have removed this check.

>
> 4)
> /*
> * Common checks for altering failover and two_phase options.
> */
> @@ -1051,7 +1075,8 @@ CheckAlterSubOption(Subscription *sub, const char
> *option,
> * two_phase options.
> */
> Assert(strcmp(option, "failover") == 0 ||
> - strcmp(option, "two_phase") == 0);
> + strcmp(option, "two_phase") == 0 ||
> + strcmp(option, "retain_conflict_info") == 0);
>
> Please update the comment atop CheckAlterSubOption to mention
> retain_conflict_info as well.

Added.

>
> 5)
> In CheckAlterSubOption(), we need to ensure that when 'slot_needs_update'
> is true, it is either failover or two_phase but not rci. Such Assert can be added.

Added an Assert().

0001:
* Removed the slot acquisition as suggested above.

0002:
* Addressed the comments above.

0003:
Rebased

0004:
Rebased

0005:
Rebased

0006:
Rebased

0007:
Rebased

Best Regards,
Hou zj