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>.
>
> ======
> 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