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>. >
I don't know why this is better than the current one. This means saying sometimes in the first point is also okay. > ====== > 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. > I don't know if this suggestion is any better than the current one. The current one matches the previous sentence where we say: "If a subscription is associated with a replication slot, then DROP SUBSCRIPTION cannot be executed inside a transaction block." > ====== > GENERAL > ... ... > > ====== > src/include/catalog/pg_subscription.h > > 5. > + /* > + * Strategy for logging replication conflicts: 'log' - server log only, > + * 'table' - conflict log table only, 'all' - both log and table. > + */ > + text subconflictlogdest BKI_FORCE_NOT_NULL; > > IIUC, this comment was deliberately aligned for readability for ~50 > versions. But then pg_indent wrapped everything. Consider putting it > back how it was and using "/**" style comment so that pg_indent will > leave it alone. > Hmm, I don't see a reason to protect pgindent do its work here. And, even if we want we should use /* --- as that is more established style but here I don't such a need. -- With Regards, Amit Kapila.
