Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip

Some review comments for the documentation patch v19-0003.

======
GENERAL

0.
FYI - The applied patch fails to build for me.

logical-replication.sgml:702: parser error : Opening and ending tag
mismatch: para line 292 and sect1
 </sect1>
         ^
logical-replication.sgml:3666: parser error : Opening and ending tag
mismatch: sect1 line 203 and chapter
</chapter>

~~~

AFAICT, it was caused by a missing </para> for the first paragraph of the patch.

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

(29.2. Subscription)

1.
+   Conflicts that occur during replication are, by default, logged as
plain text
+   in the server log, which can make automated monitoring and
analysis difficult.
+   The <command>CREATE SUBSCRIPTION</command> command provides the
+   <link linkend="sql-createsubscription-params-with-conflict-log-destination">
+   <literal>conflict_log_destination</literal></link> option to record detailed
+   conflict information in a structured, queryable format. When this parameter
+   is set to <literal>table</literal> or <literal>all</literal>, the system
+   automatically manages a dedicated conflict storage table, which is created
+   and dropped along with the subscription. This significantly
improves post-mortem
+   analysis and operational visibility of the replication setup.

"a dedicated conflict storage table"

Let's refer to this using consistent terminology like "a dedicated
conflict log table"

~~~

(29.8. Conflicts)

2.
+   in the following <firstterm>conflict</firstterm> cases. If the subscription
+   was created with the <literal>conflict_log_destination</literal> set to
+   <literal>table</literal> or <literal>all</literal>, detailed conflict
+   information is inserted into an internally managed table named
+   <literal>pg_conflict.pg_conflict_<replaceable>subscription_oid</replaceable>
+   </literal>, providing a structured record of all conflicts.

2a.
That "conflict_log_destination" should include a link back to the
CREATE SUBSCRIPTION page.

~

2b.
BUT... This information should all be removed from here to where the
next patch change is (see the following review comment). Otherwise, it
is just repeating more of the same information. [see the next review
comment]

~~~

3.
   <para>
-   The log format for logical replication conflicts is as follows:
+   When the <literal>conflict_log_destination</literal> 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 schema of this table is detailed in
+   <xref linkend="logical-replication-conflict-log-schema"/>.
+  </para>

IMO, that earlier information (see review comment #2) about the table
name can just be included here. Something like this:

SUGGESTION (markup/links are missing in my example)

When the conflict_log_destination is set to table or all, the system
automatically creates a new table to log conflict details. This table
is created in the dedicated pg_conflict namespace. The name of the
conflict log table is
pg_conflict_<replaceable>subscription_oid</replaceable>. The schema of
this table is detailed in Table 29.3.

~~~

4.
There was some other unchanged sentence:
"Note that there are other conflict scenarios, such as exclusion
constraint violations. Currently, we do not provide additional details
for them in the log."

I think the wording of the 2nd sentence does not account for the new
CLT, as "in the log" sounds like referring to the system log file to
me.

It can be changed to be generic so that it can also apply to the CLT. e.g.:
"Currently, we do not log additional details for them."

~~~

5.
+  <para>
+   The conflicting row data, including the original local tuple and
+   the remote tuple, is stored in <type>JSON</type> columns
(<literal>local_tuple</literal>
+   and <literal>remote_tuple</literal>) for flexible querying and analysis.
+  </para>

But that table schema did not have anything called 'local_tuple' (??).

~~~

6.
IMO, all the details about the format of the CLT Schema and the Log
File were running into each other, making it harder to read. It would
be better to introduce a couple of subsections to keep those apart.
e.g.

Section 29.8.1 "Logging conflicts in a Conflict Log Table"
Section 29.8.2 "Logging conflicts in the System Log"

~~~

(29.9. Restrictions)

7.
+   <listitem>
+    <para>
+     The internal table automatically created when
<literal>conflict_log_destination</literal>
+     is set to <literal>table</literal> or <literal>all</literal> is
excluded from
+     logical replication. It will not be published, even if a
publication on the
+     subscriber is defined using <literal>FOR ALL TABLES</literal>.
+    </para>
+   </listitem>

That "conflict_log_destination" should include a link back to the
CREATE SUBSCRIPTION page.

======
doc/src/sgml/ref/alter_subscription.sgml

ALTER SET

8.
+     <para>
+      When the <literal>conflict_log_destination</literal> parameter is set to
+      <literal>table</literal> or <literal>all</literal>, the system
+      automatically creates the internal logging table if it does not already
+      exist. Conversely, if the destination is changed to
+      <literal>log</literal>, logging to the table stops and the internal
+      table is automatically dropped.
+     </para>

8a.
That 'conflict_log_destination' should include the link to get back to
the parameter definition on the CREATE SUBSCRIPTION page (e.g. same as
all the other parameters do in the preceding paragraph)

~

8b.
"internal logging table"

Maybe use the consistent terminology and call this the "internal
conflict log table"

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

conflict_log_destination:

9.
It wasn't clear to me if this was intended to be in alphabetical order
or not; in case it was, note that 'conflict' comes before  'copy'.

~~~

10.
+          The supported values are <literal>log</literal>,
<literal>table</literal>,
+          and <literal>all</literal>. The default is <literal>log</literal>.

These sentences are not necessary because they are just the same
information that is repeated in the itemized list.

~~~

11.
+             <literal>table</literal>: The system automatically
creates a structured table
+             named
<literal>pg_conflict.conflict_log_table_<subid></literal>.

I felt that the schema name isn't really part of the table name.

SUGGESTION
The system automatically creates a structured table named
<literal>conflict_log_table_<subid></literal> in the
<literal>pg_conflict</literal> schema.

~~~

12.
+            <para>
+             <literal>all</literal>: Records the conflict information
to both the server log
+             and the dedicated conflict table.
+            </para>
+           </listitem>

IMO it's better to just describe these in terms of those other enums,
instead of trying to explain again what they do.

SUGGESTION
all: Same as a combination of log and table.

~~~

13.
I felt that this section should also include links "See xxx for more
details" to the appropriate logic-replication.sgml sections for the
System log formats and the CLT conflict formats.

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