Re: Proposal: Conflict log history table for Logical Replication

On Fri, Jul 3, 2026 at 11:03 AM Peter Smith <smithpb2250(at)gmail(dot)com> 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_&lt;subid&gt;</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.