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: shveta malik <shveta.malik@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-06-16T03:59:21Z
Lists: pgsql-hackers

Attachments

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

> 
> On Thu, Jun 12, 2025 at 4:22 PM shveta malik <shveta.malik@gmail.com>
> wrote:
> >
> > On Thu, Jun 12, 2025 at 11:34 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.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

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.