On Sat, Jul 4, 2026 at 11:07 PM Dilip Kumar <[email protected]> wrote: > > On Fri, Jul 3, 2026 at 11:03 AM Peter Smith <[email protected]> wrote: > > > > Some review comments for v62-0001. > > > > I was midway through reviewing patch v62-0001 when I saw it had > > already been pushed [1]. > > I am posting my review comments here anyway. > > > > ====== > > doc/src/sgml/ddl.sgml > > > > 1. > > + <para> > > + The <literal>pg_conflict</literal> schema (sometimes referred to as the > > + <emphasis>conflict schema</emphasis>) contains system-managed conflict > > + log tables used for logical replication conflict tracking. > > + These tables are created and maintained by the system and are not > > intended for > > + direct user manipulation. Unlike <literal>pg_catalog</literal>, the > > + <literal>pg_conflict</literal> schema is not implicitly included in the > > + search path, so objects within it must be referenced explicitly or by > > + adjusting the search path. > > + </para> > > > > 1a. > > Why say "sometimes"? > > > > ~ > > > > 1b. > > I thought the markup should be <firstterm> instead of <emphasis>. > > > > ~ > > > > 1c. > > > > We just said this is referred to as the "conflict schema" so let's do that! > > > > /the <literal>pg_conflict</literal> schema is not/the conflict schema is > > not/ > > > > ====== > > doc/src/sgml/ref/create_subscription.sgml > > > > 2. > > + <para> > > + The system automatically creates a structured table named > > + <literal>pg_conflict_log_<subid></literal> in the > > + <literal>pg_conflict</literal> schema. This allows for easy > > + querying and analysis of conflicts. > > + </para> > > > > Don't say "pg_conflict schema". Instead, use the term "conflict > > schema" with the appropriate <link>. > > > > ====== > > doc/src/sgml/ref/drop_subscription.sgml > > > > 3. > > + <para> > > + If a conflict log table exists for the subscription (that is, when > > + <link > > linkend="sql-createsubscription-params-with-conflict-log-destination"> > > + <literal>conflict_log_destination</literal></link> is set to > > <literal>table</literal> > > + or <literal>all</literal>), <command>DROP SUBSCRIPTION</command> > > automatically drops > > + the associated conflict log table. > > + </para> > > > > This seems a bit awkwardly worded: > > > > SUGGESTION > > When <link > > linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link> > > is set to <literal>table</literal> or <literal>all</literal>, > > <command>DROP SUBSCRIPTION</command> automatically drops the > > associated conflict log table. > > > > ====== > > GENERAL > > > > 4. > > > > policy.c: > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not be modified directly, as it could > > + * disrupt conflict logging. > > + */ > > > > tablecmds.c: > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not be modified directly, as it could > > + * disrupt conflict logging. > > + */ > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not be altered directly, as it could > > + * disrupt conflict logging. Direct ALTER commands are already rejected > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not be referenced by foreign keys, as it > > + * could disrupt conflict logging. > > + */ > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not be modified directly, as it could > > + * disrupt conflict logging. > > + */ > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not be altered directly, as it could > > + * disrupt conflict logging. > > + */ > > > > trigger.c: > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not have triggers, as it could disrupt > > + * conflict logging. > > + */ > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not have triggers, as it could disrupt > > + * conflict logging. > > + */ > > > > rewriteDefine.c: > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not have rules, as it could disrupt > > + * conflict logging. > > + */ > > > > + /* > > + * Conflict log tables are used internally for logical replication > > + * conflict logging and should not have rules, as it could disrupt > > + * conflict logging. > > + */ > > > > ~~~ > > > > The wording of the comments above is extremely repetitive. That may > > not be noteworthy if the comment was just in one place, but it's > > everywhere. > > > > How about like: > > > > BEFORE > > Conflict log tables are used internally for logical replication > > conflict logging and should not have <xxx>, as it could disrupt > > conflict logging. > > > > SUGGESTION > > Conflict log tables are internal to logical replication and must not > > have <xxx>, since it could interfere with their operation. > > > > I agree with most of Amit's comments. Regarding this one, I really > don't think the suggested comment adds much value over what we > currently have. However, if anyone feels this must be changed, please > let me know, I will provide a patch. >
Of course, changing or not changing is your call, but just so you know, whenever I post suggestions for rewording docs or code comments, those are always first validated using AI language tools. If I suggest one phrasing is better than another it is not purely my personal opinion — feel free to cut/paste both versions into your favourite AI assistant to compare for yourself. ====== Kind Regards, Peter Smith. Fujitsu Australia
