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. -- Regards, Dilip Kumar Google
