On Monday, August 4, 2025 2:16 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Fri, Aug 1, 2025 at 9:16 PM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.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
v57-0002-Re-create-the-replication-slot-if-the-conflict-r.patch
Description: v57-0002-Re-create-the-replication-slot-if-the-conflict-r.patch
v57-0001-Introduce-a-new-GUC-max_conflict_retention_durat.patch
Description: v57-0001-Introduce-a-new-GUC-max_conflict_retention_durat.patch
