Re: Proposal: Conflict log history table for Logical Replication

On Sat, Jul 4, 2026 at 11:07 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> 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.
>

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