Re: Proposal: Conflict log history table for Logical Replication

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.

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

======
[1] https://github.com/postgres/postgres/commit/a5918fddf10d297c70f7ec9067e9177e0be6d520

Kind Regards,
Peter Smith.
Fujitsu Australia