Subject: Re: Proposal: Conflict log history table for Logical Replication

From: shveta malik <[email-masked]>
Date: Wed, 7 Jan 2026 12:12:20 +0530

Hi Dilip,
Please find a few comments on v19-001:

1)
We can replace IsConflictLogTable(relid)  with
IsConflictClass(reltuple). Wherever we call IsConflictLogTable(), we
have already fetched reltuple from pg_class for that relid, we can
simply use that to see if it belongs to pg_conflict namespace. That
will avoid the need of
patch004 as well to 'Introduce a dedicated shared unique index on
pg_subscription.subconflictlogrelid'.

2)
+ if (opts.logdest == CONFLICT_LOG_DEST_TABLE ||
+ opts.logdest == CONFLICT_LOG_DEST_ALL)

We can replace with:

IsSet(opts.logdest, CONFLICT_LOG_DEST_TABLE);

3)
+-- this should generate an internal table named pg_conflict_$subid$
+CREATE SUBSCRIPTION regress_conflict_test1 CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, conflict_log_destination = 'table');
+

I think we shall verify 2 things here:
a) Table is created in pg_conflict namespace.
b) Table has name pg_conflict_<subid>

4)
We can add these 3 simple tests as well:
a) Trying to alter and truncate pg_conflict_subid table.
b) Trying to create a new table in pg_conflict namespace.
c) Moving a table into pg_conflict namespace.

~~

Overall, I’m concerned about how users will manage this table as it
grows. There is currently no way to purge old data, truncation is
disallowed, and the table must be sub-ID–tied, which also prevents
users from attaching a different table as a CLT (if needed).
Additionally, we do not offer any form of partitioning.
Do you think we should provide users with a basic purge mechanism? At
the very least, should we allow truncation so users can take a backup
and truncate a sub-ID–tied CLT to start afresh? Thoughts?

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 8 Jan 2026 12:29:47 +0530

> Please find a few comments on v19-001:

Please find a few comments for v19-002's part I have reviewed so far:

1)
ReportApplyConflict:
+ if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
+ if ((dest & CONFLICT_LOG_DEST_LOG) != 0)

We can use IsSet everywhere

2)
GetConflictLogTableInfo
This function gets dest and opens table, shall we rename to :
GetConflictLogDestAndTable

3)
GetConflictLogTableInfo:
+ *log_dest = GetLogDestination(MySubscription->conflictlogdest);
+ conflictlogrelid = MySubscription->conflictlogrelid;
+
+ /* If destination is 'log' only, no table to open. */
+ if (*log_dest == CONFLICT_LOG_DEST_LOG)
+ return NULL;
+
+ Assert(OidIsValid(conflictlogrelid));

We don't need to fetch conflictlogrelid until after 'if (*log_dest ==
CONFLICT_LOG_DEST_LOG)' check. We shall move it after the 'if' check.

4)
GetConflictLogTableInfo:
+ /* Conflict log table is dropped or not accessible. */
+ if (conflictlogrel == NULL)
+ ereport(WARNING,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("conflict log table with OID %u does not exist",
+ conflictlogrelid)));

Shall we replace it with elog(ERROR)? IMO, it should never happen and
if it happens, we should raise it as an internal error as we do for
various other cases.

5)
ReportApplyConflict():

Currently the function structure is:

/* Insert to table if destination is 'table' or 'all' */
if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)

  /* Decide what detail to show in server logs. */
if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
else <table-only: put reduced info in log>

It will be good to make it:

/*
 * Insert to table if destination is 'table' or 'all' and
 * also log the error msg to serverlog
 */
if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
{...}
else <CONFLICT_LOG_DEST_LOG case>
{log complete detail}

6)
tuple_table_slot_to_indextup_json:
+ tuple = heap_form_tuple(tupdesc, values, isnull);

Do we need to do: heap_freetuple at the end?

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 8 Jan 2026 14:51:17 +0530

> Do we need to do: heap_freetuple at the end?

Dilip, few more comments on 002:

7)
+$node_subscriber->safe_psql(
+ 'postgres', qq[
+ CREATE TABLE conf_tab_2 (a int PRIMARY KEY, b int, c int,
unique(a,b)) PARTITION BY RANGE (a);
+ CREATE TABLE conf_tab_2_p1 PARTITION OF conf_tab_2 FOR VALUES FROM
(MINVALUE) TO (100);
+]);
+
I don't see conf_tab_2 being used in  tests.

8)
+##################################################
+# INSERT data on Pub and Sub
+##################################################
+
+# Insert data in the publisher table
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO conf_tab VALUES (1,1,1);");
+
+# Insert data in the subscriber table
+$node_subscriber->safe_psql('postgres',
+ "INSERT INTO conf_tab VALUES (2,2,2), (3,3,3), (4,4,4);");

Do we really need this? IIUC, we are not using the data inserted here
for any tests.

9)
+# Test conflict insertion into the internal conflict log table
Shall we mention insert_exists test like you have mentioned for subsequent tests

10)
+# CASE 3: Switching Destination to 'log' (Server Log Verification)
No CASE 1 and CASE 2 anywhere above. So term 'CASE 3' can be removed.

11)
+# Final cleanup for subsequent bidirectional tests in the script
+$node_subscriber->safe_psql('postgres', "TRUNCATE conf_tab;");

There are no bidirectional tests.

12)
Do we even need a new file, or can we adjust these in
t/035_conflicts.pl? What do you think?
~~

Overall, tests can be reviewed and structured better.  When you get a
chance, please review it once yourself too.

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 9 Jan 2026 16:48:52 +0530

> We can use IsSet everywhere

IMHO in subscriptioncmd.c we very frequently use the bitwise operation
so it has IsSet declared, I am not really sure do we want to define
IsSet in this function as we are using it just 2 places.

> GetConflictLogDestAndTable

Yeah make sense.

> CONFLICT_LOG_DEST_LOG)' check. We shall move it after the 'if' check.

Done

> if it happens, we should raise it as an internal error as we do for

Right, now its internal table so we should make it elog(ERROR)

> else <CONFLICT_LOG_DEST_LOG case>

I did not understand this, I mean we can not put the "log complete
detail" case in the else part, because we might have to log the
complete detail along with the table if the destination is "all", are
you suggesting something else?

> Do we need to do: heap_freetuple at the end?

Yeah better we do that, instead of assuming short lived memory context.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Mon, 12 Jan 2026 12:04:04 +0530

> you suggesting something else?

Okay, I see it now. I missed the 'all' part earlier. Thanks for
pointing it out. The current implementation is good.