RE: Conflict detection for update_deleted in logical replication

On Monday, August 4, 2025 2:16 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Aug 1, 2025 at 9:16 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> >
> > Thanks for confirming. Here is V56 patch set which addressed all the
> > comments including the comments from Amit[1] and Shveta[2].
> >
> > I have merged V55-0002 into 0001 and updated the list of author and
> > reviewers based on my knowledge.
> >
>
> Thank You Hou-San for the patches. Please find a few initial comments on 002:
>
>
> 1)
> src/sgml/system-views.sgml:
> + <para>
> + <literal>conflict_retention_exceeds_max_duration</literal>
> means that
> + the duration for retaining conflict information, which is used
> + in logical replication conflict detection, has exceeded the
> maximum
> + allowable limit. It is set only for the slot
> + <literal>pg_conflict_detection</literal>, which is created when
> + <link
> linkend="sql-createsubscription-params-with-retain-dead-tuples"><literal
> >retain_dead_tuples</literal></link>
> + is enabled.
> + </para>
>
> We can mention 'max_conflict_retention_duration' here i.e:
> ...has exceeded the maximum allowable limit of
> max_conflict_retention_duration.

Added.

>
>
> 2)
> Shall we rename 'conflict_retention_exceeds_max_duration' as
> 'conflict_info_retention_exceeds_limit'? It is better to incorporate 'info' keyword,
> but then 'conflict_info_retention_exceeds_max_duration' becomes too long
> and thus I suggest 'conflict_info_retention_exceeds_limit'. Thoughts?

I will think on this.

>
> 3)
> src/sgml/monitoring.sgml:
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>retain_dead_tuples</structfield>
> <type>boolean</type>
> + </para>
> + <para>
> + True if <link
> linkend="sql-createsubscription-params-with-retain-dead-tuples"><literal
> >retain_dead_tuples</literal></link>
> + is enabled and the duration for which conflict information is
> + retained for conflict detection by this apply worker does not exceed
> + <link
> + linkend="guc-max-conflict-retention-duration"><literal>max_conflict_re
> + tention_duration</literal></link>;
> NULL for
> + parallel apply workers and table synchronization workers.
> + </para></entry>
> + </row>
>
> a)
> In the html file, the link does not take me to 'max_conflict_retention_duration'
> GUC. It takes to that page but to some other location.
>
> b)
> + the duration for which conflict information is
> + retained for conflict detection by this apply worker
>
> Shall this be better: 'the duration for which information useful for conflict
> detection is retained by this apply worker'

Changed.

>
> 4)
> src/sgml/config.sgml:
>
> a)
> + Maximum duration for which each apply worker can request to retain
> the
> + information useful for conflict detection when
> + <literal>retain_dead_tuples</literal> is enabled for the associated
> + subscriptions.
>
> Shall it be :
> "Maximum duration for which each apply worker is allowed to retain.."
> or "can retain"?

Changed to "is allowed to"

>
> b)
> src/sgml/config.sgml
> + subscriptions. The default value is <literal>0</literal>, indicating
> + that conflict information is retained until it is no longer needed for
> + detection purposes. If this value is specified without units, it is
> + taken as milliseconds.
>
> 'that conflict information is retained' --> 'that information useful for conflict
> detection is retained'

I changed to "the information" because the nearby texts have already
mentioned the usage of this information.

>
> c)
> src/sgml/config.sgml
> + The replication slot
> + <quote><literal>pg_conflict_detection</literal></quote> that
> used to
> + retain conflict information will be invalidated if all apply workers
> + associated with the subscriptions, where
>
> 'that used' --> 'that is used'

Fixed.

>
> 5)
> ApplyLauncherMain():
> + /*
> + * Stop the conflict information retention only if all workers
> + * for subscriptions with retain_dead_tuples enabled have
> + * requested it.
> + */
> + stop_retention &= sub->enabled;
>
> This comment is not clear. By enabling or disabling subscription, how can it
> request for 'stop or continue conflict info retention'?
>
> Do you mean we can not 'invalidate the slot' and thus stop retention if a sub
> with rdt=ON is disabled?

Yes, there is no apply worker for disabled
subscription, thus no way to request that.

> If so, we can pair it up with the previous comment
> itself where we have mentioned that we can not advance xmin when sub is
> disabled, as that comment indicates a clear reason too.

Changed.

>
> 6)
> Above brings me to a point that in doc, shall we mention that if a sub with
> rdt=on is disabled, even 'max_conflict_retention_duration' is not considered
> for other subs which have rdt=ON.

I think the documentation specifies that only active apply workers can make such
requests, which appears sufficient to me.

>
> 7)
> Shall we rename 'max_conflict_retention_duration' to
> 'max_conflict_info_retention_duration' as the latter one is more clear?

I will think on it.

>
> 8)
> + * nonremovable xid. Similarly, stop the conflict information
> + * retention only if all workers for subscriptions with
> + * retain_dead_tuples enabled have requested it.
>
> Shall we rephrase to:
> Similarly, can't stop the conflict information retention unless all such workers
> are running.

Changed.

Here is V57 patch set which addressed most of comments.

In this version, I also fixed a bug that the apply worker continued to
find dead tuples even if it has already stop retaining dead tuples.

Best Regards,
Hou zj