Re: Proposal: Conflict log history table for Logical Replication

Some review comments for v23-0003 (docs).

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

1. GENERAL.

Mentioning those specific enum values of coflict_log_dest every time
seemed overkill. In most cases you do really need to mention 'all' or
'log' ot 'table'. IMO the user should use the link to the
'conflict_log_destination' parameter to see that level of detail.

Some examples what I am referring to:

(29.2 Subscription)

BEFORE
The CREATE SUBSCRIPTION command provides the conflict_log_destination
option to record detailed conflict information in a structured,
queryable format. When this parameter is set to table or all, the
system automatically manages a dedicated conflict log table, which is
created and dropped along with the subscription.

SUGGESTION
The CREATE SUBSCRIPTION conflict_log_destination parameter can be set
to create a dedicated conflict log table that is automatically
managed with the subscription.

~

(29.8)

BEFORE
When the conflict_log_destination parameter is set to table or all,
the system automatically creates a new table with a predefined schema
to log conflict details.

SUGGESTION
The conflict_log_destination parameter can automatically create a
dedicated conflict log table.

~

(29.8 Conflicts)

BEFORE
If conflict_log_destination is left at the default setting or
explicitly configured as log or all, logical replication conflicts are
logged in the following format:

SUGGESTION
When conflict_log_destination is set to log conflicts to the server
log, the following format is used:

~

(29.9 Restrictions)

BEFORE:
The internal table automatically created when conflict_log_destination
is set to table or all is excluded from logical replication. It will
not be published, even if a publication on the subscriber is defined
using FOR ALL TABLES.

SUGGESTION
Conflict log tables (see conflict_log_destination parameter) are never
published, even when using FOR ALL TABLES in a publication.

~~~

(29.8 conflicts)

2.
<para>
- The log format for logical replication conflicts is as follows:
+ When the <link
linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
+ parameter is set to <literal>table</literal> or
<literal>all</literal>, the system
+ automatically creates a new table with a predefined schema to log conflict
+ details. This table is created in the dedicated
+ <literal>pg_conflict</literal> namespace. The name of the
conflict log table
+ is <literal>pg_conflict_&lt;subid&gt;</literal>. The schema of this
+ table is detailed in
+ <xref linkend="logical-replication-conflict-log-schema"/>.
+ </para>

Maybe instead of mentioned "schema" twice you can remove that part
that says "with a predefined schema", and then also change:

/The schema of this table is detailed in.../The predefined schema of
this table is detailed in.../

~~~

(29.8 Conflicts)

3.
+ <para>
+ The conflicting row data, including the incoming remote row and
the associated
+ local conflict details, is stored in <type>JSON</type> formats
(<literal>remote_tuple</literal>
+ and <literal>local_conflicts</literal>) for flexible querying and analysis.
+ </para>

Would this be better rearranged slightly to name those columns where
they are mentioned?

SUGGESTION
The conflicting row data, including the incoming remote row
(remote_tuple) and the associated local conflict details
(local_conflicts), is stored in JSON format, for flexible querying and
analysis.

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

(value: table)

4.
+ <caution>
+ <para>
+ The internal conflict log table is strictly tied to the
lifecycle of the
+ subscription or the
<literal>conflict_log_destination</literal> setting. If
+ the subscription is dropped, or if the destination is changed to
+ <literal>log</literal>, the table and all its recorded
conflict data are
+ <emphasis>permanently deleted</emphasis>. To perform a
post-mortem analysis
+ after removing a subscription, users must manually back
up the conflict log
+ table before the deletion occurs.
+ </para>
+ </caution>

4a.
That last sentence seems a bit awkward. You already explained that the
CLT is removed when a subscription is dropped. Here is one simpler
alternative:

SUGGESTION
If post-mortem analysis may be needed, back up the conflict log table
before removing the subscription.

~

4b.
I also thought this is more readable when that last caution sentence
was in a new <para>, but maybe just my opinion.

~~~

(value: all)

5.
+ <listitem>
+ <para>
+ <literal>all</literal>: Records to both the server log
and the table.
+ </para>
+ </listitem>

I felt that the description for 'all' should refer to those other
destinations explicitly.

SUGGESTION
<literal>all</literal>: Records conflict details to both destinations
<literal>log</literal> and <literal>table</literal>.

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