Re: Proposal: Conflict log history table for Logical Replication

On Tue, Oct 27, 2026 at 5:03 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> Please find the updated patches v85. I also incorporated Shveta's doc
> suggestion[1] into the v85-0004 patch.

Thanks for the patches. You did not post v85-0004, which you are
referring to in the above comment.

I reviewed 001 mainly. A few comments:

1)

+ <entry><literal>apply_origin</literal></entry>
+ <entry><type>text</type></entry>
+ <entry>The origin of the remote transaction.</entry>
+ </row>

Should we change it to:
The name of the replication origin the apply worker used when
recording the conflict.

2)
ReportApplyConflict:
+ else
+ {
+ appendStringInfo(&err_detail,
+ _("Conflict details are logged to the conflict log table: %s.%s"),
+ get_namespace_name(RelationGetNamespace(conflictlogrel)),
+ RelationGetRelationName(conflictlogrel));
+ }

We should have an Assert(conflictlogrel) in this else-block, specially
because for further table-insertion we are using condition as 'if
(conflictlogrel)' rather that 'if (CONFLICTS_LOGGED_TO_TABLE)'.

3)
+ * Take the column name from the table, not the index descriptor. The
+ * index keeps the name it had at creation, which goes stale after a
+ * column rename and can be wrong for repeated columns such as UNIQUE
+ * (a, a). This matches the names in the server log.

It is not clear what 'This' refers to in last sentence, generally it
is meant to be referring to neareset preceding thing which is not what
we inted here.

Suggestion:
Using the table's names keeps these consistent with the server log.

4)
build_local_conflicts_json_array() still uses row_to_json() and is
safe unlike build_index_key_json() which mentions why to not use
row_to_json. I think it is worth a brief comment in
build_local_conflicts_json_array() as in why row_to_json is safe in
this case.

Shall we add this comment atop row_to_json() call:

Unlike build_index_key_json(), using row_to_json() is safe here: the
object's fields are all built-in types (xid, timestamptz, text) below
FirstNormalObjectId, so json_categorize_type() never invokes a user
CREATE CAST (t AS json), and the values are small and bounded.

5)
Similar to how we mention against remote_commit_ts in docs that 'It is
also <literal>NULL</literal> if the conflict occurs in a transaction
that is prepared on the subscriber', shouldn't we also mention against
'remote_commit_lsn', that it is prepare-lsn for prepared-txns and not
commit-prepared lsn?

6)
+###############################################################################
+# Check that a user-defined CAST (t AS json) is not consulted when recording
+# the replica identity.
+#
+# Each key value is rendered with its type's output function, as the server log
+# does, so a cast cannot substitute its own representation. This
matters because
+# such a cast is a function no other part of apply calls, and its
output bears no
+# relation to the size of the value received, so a tiny key could otherwise
+# render a json value past the 1GB limit and error out the apply worker.
+###############################################################################

I find this comment very hard to follow, can we please simplify it?

Suggestion: (take it if you agree)

# Check that a user-defined CAST (t AS json) is not used when recording the
# replica identity.
#
# Each key value is formatted with its data type's output function, as in the
# server log; a user-defined cast to json is never consulted. Calling it would
# execute user code that the apply worker otherwise never calls, and its output
# could be arbitrarily large, unrelated to the size of the original value. Even
# a tiny key could thus produce a json value exceeding the 1GB column limit and
# cause the apply worker to fail.

thanks
Shveta