Re: Proposal: Conflict log history table for Logical Replication

Hi. Here are some review comments for v88-0001 (DOCS part only)

======
Commit Message

1.
SELECT remote_xid, relname, apply_origin, replica_identity,
local_conflicts[1] ->> 'xid' AS local_xid
FROM pg_conflict.pg_conflict_log_16396;

remote_xid | relname | apply_origin | replica_identity | lo
------------+----------+---------------+------------------+-----------
760 | test | pg_16406 | {"a":"1"} | 77
765 | conf_tab | pg_16406 | {"a":"2"} | 775

Typo: column "lo"?

======
doc/src/sgml/logical-replication.sgml

2.
+ <para>
+ <replaceable class="parameter">column_value</replaceable>
is the column value.
+ The large column values are truncated to 64 bytes.
+ </para>

IMO don't need to say "The large", and anyway "large" is an unknown
quantity until you read to the end of the sentence.

Consider to reword last sentence:
Values longer than 64 bytes are truncated.

~~~

3.
+ <para>
+ The conflict log table serves as an audit trail for resolved conflicts
+ where replication continues and the transaction successfully commits
+ (such as <literal>update_origin_differs</literal>,
+ <literal>update_missing</literal>, <literal>update_deleted</literal>,
+ <literal>delete_origin_differs</literal>, and
+ <literal>delete_missing</literal>). In these cases, local data may diverge
+ or be overwritten, making a queryable audit log valuable for DBAs to
+ understand post-hoc why a row changed or was skipped.
+ </para>

My AI identified (and I concur) a few unusual things about the above paragraph.

e.g. "resolved conflicts" -- nothing really gets "resolved", they are
just applied or skipped...
e.g. "successfully commits" -- just say "commits"
e.g. "post-hoc" -- latin but PG Docs prefer plain English
e.g. "audit trail" -- It suggeststhe CLT is more like a diagnostic
record, not an audit log in the security sense, and some readers may
expect more from the term.
etc.

The suggested rewording is like:

The conflict log table records conflicts that do not stop replication,
that is, those where the apply transaction continues and commits
(<literal>update_origin_differs</literal>,
<literal>update_missing</literal>, <literal>update_deleted</literal>,
<literal>delete_origin_differs</literal>, and
<literal>delete_missing</literal>). In these cases local data can be
overwritten, or the remote change skipped, so the table lets
administrators determine afterwards why a row changed or was skipped.

~

4.
+ <listitem>
+ <para>
+ Conflict log tables (see <link
linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
parameter)
+ are never published, even when using <literal>FOR ALL
TABLES</literal> in a
+ publication.
+ </para>
+ </listitem>

I wonder if it makes more sense for this Restrictions note to refer
direct to the CLT section (29.8.1.2. Table-based logging #) rather
than the conflict_log_destination parameter.

======
doc/src/sgml/ref/create_subscription.sgml

5.
<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.
+ <literal>pg_conflict</literal> schema to record resolved
+ conflicts. Conflicts raised at <literal>ERROR</literal> level
+ (which halt replication and abort the transaction) are not
+ recorded in the table; they are always reported to the
server log.
+ See <xref
linkend="logical-replication-conflict-table-based-logging"/>
+ for details.
</para>

IMO that "See xxx for details." sentence should immediately follow the
sentence "... to record resolved conflicts.".

Currently, "See xxx for details." appears to refer to the server log,
not the CLT, because that is the topic of the sentence preceding it.

======
doc/src/sgml/ref/drop_subscription.sgml

6.
<para>
- If a conflict log table exists for the subscription (that is, when
- <link linkend="sql-createsubscription-params-with-conflict-log-destination">
+ If <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.
+ or <literal>all</literal>, the subscription has an associated conflict log
+ table, which <command>DROP SUBSCRIPTION</command> drops along with the
+ subscription. However, <command>DROP SUBSCRIPTION</command> cannot be
+ executed while the subscription has prepared transactions (see
+ <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>),
+ because such a transaction may hold a lock on the conflict log table that is
+ released only when the apply worker processes the corresponding
+ <command>COMMIT PREPARED</command> or <command>ROLLBACK PREPARED</command>.
+ Resolve these transactions first (see
+ <link linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>)
+ before dropping the subscription.
</para>

The "However, DROP SUBSCRIPTION</command> cannot be executed" reads
too much like a general limitation. The pre-condition (a CLT exists)
was assumed, but the "However" wording doesn't tie back to that.

Consider rewording more like:

If <literal>conflict_log_destination</literal> is set to
<literal>table</literal> or <literal>all</literal>, <command>DROP
SUBSCRIPTION</command> also drops the subscription's conflict log
table. In that case, the command cannot complete while the
subscription has prepared transactions (see
<literal>two_phase</literal>), because such a transaction may hold a
lock on the conflict log table until the apply worker processes the
corresponding <command>COMMIT PREPARED</command> or <command>ROLLBACK
PREPARED</command>. Resolve these transactions first (see
<link linkend="view-pg-prepared-xacts"><structname>pg_prepared_xacts</structname></link>).

======
Kind Regards,
Peter Smith.
Fujitsu Australia