Re: Conflict detection for update_deleted in logical replication

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:
>
> 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.
>
> 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..
>

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.

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.

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.

thanks
Shveta