Re: Proposal: Conflict log history table for Logical Replication

Some review comments for the v25* patches:

//////////
Patch v25-0001
//////////

1.
+ /*
+ * Conflict log tables are managed by the system to record logical
+ * replication conflicts. We do not allow locking rows in CONFLICT
+ * relations.
+ */
+ if (IsConflictNamespace(RelationGetNamespace(rel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot lock rows in conflict log table \"%s\"",
+ RelationGetRelationName(rel))));

I felt the code comment should be changed, too.

e.g. That comment sentence "We do not allow locking rows in CONFLICT
relations" seemed redundant because you already said the CLT is
"managed by the system", and also it is basically just repeating the
same info as the error message.

Suggest either:
a) remove it, or
b) change /CONFLICT/pg_conflict namespace/

//////////
Patch v25-0002
//////////

No review comments.

//////////
Patch v25-0003 (docs)
//////////

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

(29.8 Conflicts)

1.
There is an earlier sentence on this page:
"Note that there are other conflict scenarios, such as exclusion
constraint violations. Currently, we do not provide additional details
for them in the log."

~

That "in the log part" wording maybe needs to be changed/removed now
because the destination might be a CLT, not a log.

SUGGESTION:
Currently, we do not provide additional details for them.

~~~

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 can 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_&lt;subid&gt;</literal>. The predefined
schema of this table is
+ detailed in
+ <xref linkend="logical-replication-conflict-log-schema"/>.
+ </para>
+

2a.
Typo: /can automatically creates/can automatically create/

~

2b.
It sounds a bit strange to say "a dedicated" 2x in 2 sentences. Maybe
omit the first one:

SUGGESTION
... can automatically create a conflict log table.

~

2c.
Perhaps the "conflict log table" should be using <firstterm> SGML markup here.

~~~

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>

/formats, for/format for/

~~~

4.
+ <para>
+ If <link linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
+ is set to log conflicts to the server log, the following format is used:

I'm not sure you need that link because the same parameter was already
linked a bit earlier on this same page.

~~~

(29.9 Restrictions)

5.
+ <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>

That FOR ALL TABLES ought to have <literal> SGML markup.

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

6.
I still feel that somewhere in here, there ought to be some link/s
back to the "29.8 Conflicts" details about the CLT schema and LOG
formats.

SUGGESTION
conflict_log_destination / log: ... See <link> for the format used to
log conflict details.

conflict_log_destination / table: ... See <link> for the predefined
schema of the conflict log table.

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