Re: Proposal: Conflict log history table for Logical Replication

On Tue, Apr 28, 2026 at 9:45 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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:
> ====================

Thanks for the review, I will send the updated patch along with other
open comments and bug fixes.

> 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?

+1

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

I will analyze this and respond by tomorrow.

> 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?

BlessTupleDesc are used in other cases as well other then SRF as well,
I will add a one liner comment.

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

Will fix

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

I will check and fix this.

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

I prefer pg_conflict_log_<subid> over pg_subconflict_<subid>

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

Make sense

--
Regards,
Dilip Kumar
Google