Re: Proposal: Conflict log history table for Logical Replication

On Thu, Apr 23, 2026 at 7:36 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 21 Apr 2026 at 15:21, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
Some comments on 0001:
====================
1.
postgres=# create subscription sub1 connection 'dbname=postgres'
publication pub1 WITH (conflict_log_destination='all');
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION

Should there be a NOTICE for internally created table similar to slot creation?

2.
+typedef enum ConflictLogDest
+{
+ /* Log conflicts to the server logs */
+ CONFLICT_LOG_DEST_LOG = 1 << 0, /* 0x01 */
+
+ /* Log conflicts to an internally managed conflict log table */
+ CONFLICT_LOG_DEST_TABLE = 1 << 1, /* 0x02 */
+
+ /* Convenience bitmask for all supported destinations */
+ CONFLICT_LOG_DEST_ALL = (CONFLICT_LOG_DEST_LOG | CONFLICT_LOG_DEST_TABLE)
+} ConflictLogDest;
+
+/*
+ * Array mapping for converting internal enum to string.
+ */
+static const char *const ConflictLogDestNames[] = {
+ [CONFLICT_LOG_DEST_LOG] = "log",
+ [CONFLICT_LOG_DEST_TABLE] = "table",
+ [CONFLICT_LOG_DEST_ALL] = "all"
+};

Defining an array this way could be an Array size issue. Actually the
array has just three elements so the last element should be at
ConflictLogDestNames[2] but if we go by the above definition, it will
be ConflictLogDestNames[3]. Can we define by referring the following
existing way:

typedef enum
{
XLOG_FROM_ANY = 0, /* request to read WAL from any source */
XLOG_FROM_ARCHIVE, /* restored using restore_command */
XLOG_FROM_PG_WAL, /* existing file in pg_wal */
XLOG_FROM_STREAM, /* streamed from primary */
} XLogSource;

/* human-readable names for XLogSources, for debugging output */
static const char *const xlogSourceNames[] = {"any", "archive",
"pg_wal", "stream"};

In this case, _ANY is similar to your 'all' case and won't have the
array size issue.

3. create_conflict_log_table_tupdesc() calls BlessTupleDesc, which is
intended for set-returning functions as indicated by a comment atop it
(BlessTupleDesc - make a completed tuple descriptor useful for SRFs).
Is there a reason that the patch uses it for
heap_create_with_catalog()? If so, can we write a comment for the
same?

4. ConflictLogDestNames[] and ConflictLogSchema[] are static const
arrays defined in conflict.h, which is included by conflict.c,
worker.c, and execReplication.c. Each translation unit gets its own
copy. It is better to define them in .c and the declared extern in the
header. See how forkNames is used in the code.

5.
We allow DELETE to permit users to manually
+ * prune or truncate these logs, but manual data insertion or modification
+ * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of the
+ * system-generated logs.
+ */
+ if (IsConflictNamespace(RelationGetNamespace(resultRel)) &&
+ operation != CMD_DELETE)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("cannot modify or insert data into conflict log table \"%s\"",
+ RelationGetRelationName(resultRel)),
+ errdetail("Conflict log tables are system-managed and only support
cleanup via DELETE or TRUNCATE.")));

The comment and code doesn't match completely because errdetail has
reference to TRUNCATE but comment says only DELETE probably because
this function doesn't accept TRUNCATE as operation type. I think we
can slightly update the comments to reflect it.

6. Commit message states:
"If the user chooses to enable logging to a table (by selecting
'table' or 'all'),
an internal logging table named conflict_log_table_<subid> is automatically
created .."

I see that the patch creates a table with the name
pg_conflict_<subid>. Apart from updating the commit message, how about
naming the table as pg_conflict_log_<subid> or pg_subconflict_<subid>.
In the first alternative pg_conflict_log_<subid>, the table name in
isolation without schema name describes its purpose, the second
alternative breaks the repetitiveness of pg_conflict when used with
schema like pg_conflict.pg_conflict_16394.

7. The commit message can contain why we didn't go for a single global
table to store conflicts for each subscription.

--
With Regards,
Amit Kapila.