Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip/Vignesh.

Some review comments for v33-0004 (docs).

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

(29.2. Subscription)

1.
Perhaps the "conflict log table" should be using <firstterm> SGML
markup the first time it gets mentioned?

~~~

(29.8. Conflicts)

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

2a.
It's not really correct to say that it "automatically creates a
dedicated conflict log table.", because that sounds like it will
always happen.

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

~

2b.
Also it seems overkill to say the word "dedicated" multiple times.
Maybe remove the 2nd one.

~~~

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

Comma typo: /formats, for/formats for/

~~~

(29.9. Restrictions)

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 FOR ALL TABLES in a publication.
+ </para>
+ </listitem>

The "FOR ALL TABLES" should have SGML <literal> markup.

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

(conflict_log_destination (enum))

5.
+ <para>
+ If post-mortem analysis may be needed, back up the
conflict log table before
+ removing the subscription.
+ </para>

5a.
My AI tool says that the "post-mortem analysis" wording is a bit
overkill for online documentation:

SUGGESTION
If conflict history may be needed later, back up...

~

5b.
That note only says about "removing the subscription", but AFAIK the
user will also need to do backup if changing from "table/all" to
"log". Should that also be mentioned? It might make this caution a bit
repetitive -- Maybe it is simply easier to reword this sentence like:

SUGGESTION
If conflict history may be needed later, be sure to back up the
conflict log table before it gets removed.

======
GENERAL -- add new subsections

6.
Apart from those minor review comments above, I felt that the current
single "29.8. Conflicts" section should be broken into subsections for
readability and for easier referencing.

I propose that it should look like this:

29.8. Conflicts
29.8.1. Conflict logging
29.8.2. Table-based logging
29.8.3. File-based logging
29.8.4. Notes

PSA a POC patch where I've done this restructuring. It looks much
better to me. See what you think.

Most of the original patch wording is unchanged.
Some xrefs are added on the CREATE SUBSCRIPTION page.

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