Hi,
On Tue, Nov 3, 2026 at 09:44 AM Peter Smith wrote:
Hi. Here are some review comments for v88-0001 (DOCS part only)
======
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.
This is pre-existing text on HEAD that only appeared in the diff due to
re-indentation under <sect3>, so I don't think we should change it in
this patch.
~~~
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...
The <note> right above this paragraph already defines what "resolved
conflicts" means here ("logged at LOG level where the apply transaction
continues"), and the same term is used consistently in
create_subscription.sgml and the code comments. And applied or skipped
is also form of 'resolved'. So the current version is fine.
e.g. "successfully commits" -- just say "commits"
Removing it does not look better to me, I think we want to highlight the
commit succeed.
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.
The content of the CLT is clearly documented, so it should be clear to
user what info can be found there. So not sure about this comment.
~
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.
conflict_log_destination is the parameter that creates the table, and
its entry already links to the table-based logging section. So I think
current version is good as is.
======
doc/src/sgml/ref/create_subscription.sgml
5.
<para>
The system automatically creates a structured table named
<literal>pg_conflict_log_<subid></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.
The later part for conflicts at ERROR level is also explained at that
section, so the current position should be fine.
======
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.
The whole <para> is exclusively about the conflict log table, and the
very same sentence explicitly says "because such a transaction may hold
a lock on the conflict log table", so in context it is already clear. I
the current version is fine.
--
Best Regards,
Zhijie Hou
