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

From: Peter Smith <[email-masked]>
Date: Fri, 9 Jan 2026 14:00:20 +1100

Hi Dilip.

Here are some review comments for the v19-0002 (code only, not tests).

======
src/backend/replication/logical/conflict.c

1.
+/* Schema for the elements within the 'local_conflicts' JSON array */
+static const ConflictLogColumnDef LocalConflictSchema[] =
+{
+ { .attname = "xid",       .atttypid = XIDOID },
+ { .attname = "commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "origin",    .atttypid = TEXTOID },
+ { .attname = "key",       .atttypid = JSONOID },
+ { .attname = "tuple",     .atttypid = JSONOID }
+};

Maybe this only needs to be used in this module, but I still thought
LocalConflictSchema[] (and the MAX_LOCAL_CONFLICT_INFO_ATTRS) belongs
more naturally alongside the other ConflictLogSchema[] (e.g. in
conflict.h)

~~~

2.
+
+#define MAX_LOCAL_CONFLICT_INFO_ATTRS \
+ (sizeof(LocalConflictSchema) / sizeof(LocalConflictSchema[0]))
+

how about:

#define MAX_LOCAL_CONFLICT_INFO_ATTRS lengthof(LocalConflictSchema)

~~~

ReportApplyConflict:

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

I think it is unnecessary to even mention 'all'. The bitmasks tell
everything you need to know.

SUGGESTION
Insert to table if requested.

~

4.
There's a slightly convoluted if;if/else with these bitmasks here, but
I think Shveta already suggested the same change as what I was
thinking.

~~~

GetConflictLogTableInfo:

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

I think checking and mentioning 'log' here is not future-proof for
when there might be more kinds of destinations. We just want to return
when the user doesn't want a 'table'. Use the bitmasks to do that.

SUGGESTION
/* Quick exit if a conflict log table was not requested. */
if ((*logdest & CONFLICT_LOG_DEST_TABLE) == 0)
  return NULL;

~~~

build_conflict_tupledesc:

6.
+static TupleDesc
+build_conflict_tupledesc(void)
+{

Missing function comment. There used to be one (??).

~~~

build_local_conflicts_json_array:

7.
+ json_datum_array = (Datum *) palloc(num_conflicts * sizeof(Datum));
+ json_null_array = (bool *) palloc0(num_conflicts * sizeof(bool));

I may have already asked this same question in a previous review, but
shouldn't these be using those new palloc_array and palloc0_array
macros?

======
src/include/replication/conflict.h

8.
-/* Define the count using the array size */
 #define MAX_CONFLICT_ATTR_NUM (sizeof(ConflictLogSchema) /
sizeof(ConflictLogSchema[0]))

This change appears to be in the wrong patch. AFAICT it belonged in 0001.

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

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 10 Jan 2026 12:19:46 +0530

> more naturally alongside the other ConflictLogSchema[] (e.g. in

Since this is used only in conflict.c, my personal preference to keep
it in the .c file

> #define MAX_LOCAL_CONFLICT_INFO_ATTRS lengthof(LocalConflictSchema)

Make sense
> Insert to table if requested.

Done

> I think Shveta already suggested the same change as what I was

So now we have this, which is first handling the table case and
handling the LOG case separately, because irrespective of whether we
want to insert into the table or not we need to check what we need to
log?  What's your suggestion for this?

if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
{
 <Insert into table>
}

if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
{
<write full log>
}
else
{
<write minimal log>
}

> if ((*logdest & CONFLICT_LOG_DEST_TABLE) == 0)

Make sense

> Missing function comment. There used to be one (??).

Yeah it got lost in some of the refactoring.

> shouldn't these be using those new palloc_array and palloc0_array

Done.

> This change appears to be in the wrong patch. AFAICT it belonged in 0001.

Fixed.

Will shared the updated patch after fixing other comments from Shveta
as well, thanks for the review.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 10 Jan 2026 18:45:28 +0530

> as well, thanks for the review.

Here is the updated patch

1. fixes all the agreed comments from Peter and Shveta.

2. also for dependency between the clt and subscription I have
improvised the comments, I mean now we don't need that dependency to
protect the clt from getting dropped as that is taken care by internal
schema, we still maintain the dependency considering the same thing is
done for the toast table and these tables are internally
created/dropped along with subscription so conceptually it makes sense
to maintain the internal dependency and its also looks fine to drop it
explicitly using dependency dependency drop function i.e.
performDeletion() as we are doing similar things in other cases like
"Identity Columns".

3. I tried to merge the test cases in the existing 035_conflict.pl, I
modified the create subscription to log into both table and log and
then extended some of the existing test cases to validate the table
logging as well.  If we think this looks fine then we can update a few
more test cases to validate from the table or maybe all the test
cases?

--
Regards,
Dilip Kumar
Google

---

From: Peter Smith <[email-masked]>
Date: Mon, 12 Jan 2026 16:26:23 +1100

Some review comments for the v20-0001 patch.

======
0. General

Applying the patch gives whitespace warnings:

$ git apply ../patches_misc/v20-0001-Add-configurable-conflict-log-table-for-Logical-.patch
../patches_misc/v20-0001-Add-configurable-conflict-log-table-for-Logical-.patch:1543:
trailing whitespace.
CREATE SUBSCRIPTION regress_conflict_protection_test CONNECTION
'dbname=regress_doesnotexist'
../patches_misc/v20-0001-Add-configurable-conflict-log-table-for-Logical-.patch:610:
new blank line at EOF.
+
warning: 2 lines add whitespace errors.

======
src/backend/catalog/pg_publication.c

1.
+
+ /* Can't be conflict log table */
+ if (IsConflictNamespace(RelationGetNamespace(targetrel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot add relation \"%s.%s\" to publication",
+ get_namespace_name(RelationGetNamespace(targetrel)),
+ RelationGetRelationName(targetrel)),
+ errdetail("This operation is not supported for conflict log tables.")));

I felt it may be better to still keep the function
IsConflictLogTable(). You can just put this namespace logic inside
that function. Then one day, if other tables are added to that special
schema, at least the impact for checking CLT is contained.

~~~

is_publishable_relation:

2.
 /*
  * Another variant of is_publishable_class(), taking a Relation.
+ *
+ * Note: Conflict log tables are not publishable.  However, we intentionally
+ * skip this check here because this function is called for every change and
+ * performing this check during every change publication is costly.  To ensure
+ * unpublishable entries are ignored without incurring performance overhead,
+ * tuples inserted into the conflict log table uses the HEAP_INSERT_NO_LOGICAL
+ * flag.  This allows the decoding layer to bypass these entries automatically.

Is that comment still relevant, now that is_publishable_class() has
been changed to use IsConflictClass()?

~~~

pg_relation_is_publishable:

3.
+
+ /* Subscription conflict log tables are not published */
  result = is_publishable_class(relid, (Form_pg_class) GETSTRUCT(tuple));

The comment seems unnecessary/misplaced because there is no special
code anymore in this function for CLT.

~~~

GetAllPublicationRelations:

4.
+ /* Subscription conflict log tables are not published */
  if (is_publishable_class(relid, relForm) &&

The comment seems unnecessary/misplaced because there is no special
code anymore in this function for CLT.

======

5.
+extern Relation GetConflictLogTableInfo(ConflictLogDest *log_dest);
+extern void InsertConflictLogTuple(Relation conflictlogrel);

This change seems to be in the wrong patch. afaict these functions are
not implemented in patch 0001.

======
src/test/regress/sql/subscription.sql

6.
+-- verify the physical table exists, its OID matches subconflictlogrelid,
+-- and it is located in the 'pg_conflict' namespace.
+SELECT count(*)
+FROM pg_class c
+JOIN pg_subscription s ON c.relname = 'pg_conflict_' || s.oid
+JOIN pg_namespace n ON c.relnamespace = n.oid
+WHERE s.subname = 'regress_conflict_test1'
+  AND c.oid = s.subconflictlogrelid
+  AND n.nspname = 'pg_conflict';
+ count
+-------
+     1
+(1 row)
+

For this kind of test, I wondered if it would be better to make the
SQL read more like the comment says. You can put some of the WHERE
conditions directly in the SELECT column list and just let the
regression comparisons with 'expected' results take care of validating
them.

e.g.
SUGGESTION
-- verify the physical table exists, its OID matches subconflictlogrelid,
-- and it is located in the 'pg_conflict' namespace.
SELECT n.nspname, (c.oid = s.subconflictlogrelid) AS "Oid matches"
FROM pg_class c
JOIN pg_subscription s ON c.relname = 'pg_conflict_' || s.oid
JOIN pg_namespace n ON c.relnamespace = n.oid
WHERE s.subname = 'regress_conflict_test1';
   nspname   | Oid matches
-------------+-------------
 pg_conflict | t
(1 row)

~~~

7.
Similarly, for the CLT metadata test, instead of only expecting 11
rows, why not just SELECT/compare all the attributes more generically?

e.g.
SUGGESTION
-- check if the internal table has the correct schema
SELECT a.attnum, a.attname
FROM pg_attribute a
JOIN pg_class c ON a.attrelid = c.oid
JOIN pg_subscription s ON c.relname = 'pg_conflict_' || s.oid
WHERE s.subname = 'regress_conflict_test1' AND a.attnum > 0
ORDER BY a.attnum;
 attnum |      attname
--------+-------------------
      1 | relid
      2 | schemaname
      3 | relname
      4 | conflict_type
      5 | remote_xid
      6 | remote_commit_lsn
      7 | remote_commit_ts
      8 | remote_origin
      9 | replica_identity
     10 | remote_tuple
     11 | local_conflicts
(11 rows)

~~~

8.
+EXCEPTION WHEN insufficient_privilege THEN
+    RAISE NOTICE 'captured expected error: insufficient_privilege
during ALTER';

IIUC, the test was only written that way because the CLT name is
dynamically based on the <subid>, so you cannot know it up-front to
include that name in the 'expected' error message. Maybe there could
be a comment to explain that?

~~~

9.
+EXCEPTION WHEN insufficient_privilege THEN
+    RAISE NOTICE 'captured expected error: insufficient_privilege
during TRUNCATE';

Ditto previous review comment.

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

---

From: Peter Smith <[email-masked]>
Date: Mon, 12 Jan 2026 16:29:18 +1100

Some review comments for patch v20-0003 (docs)

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

(29.8. Conflicts)

1.
+   When the <link
linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
+   is set to <literal>table</literal> or <literal>all</literal>, the system
+   automatically creates a new table with a predefined schema to log conflict
+   details. This table is created in the dedicated
+   <literal>pg_conflict</literal> namespace.  The name of the
conflict log table
+   is pg_conflict_<replaceable>subscription_oid</replaceable>. The
schema of this
+   table is detailed in
+   <xref linkend="logical-replication-conflict-log-schema"/>.
+  </para>

1a.
Instead of saying "When the conflict_log_destination is ...", maybe it
should say "When the conflict_log_destination parameter is ...".

That should be more consistent with the wording used elsewhere in this patch.

~

1b.
But, on the CREATE SUBSCRIPTION page, the table name is called:
<literal>pg_conflict_&lt;subid&gt;</literal>

Both places should refer to the name using the same format -- the
CREATE SUBSCRIPTION way looked good to me.

~~~

2.
+   The conflicting row data, including the original local tuple and
+   the remote tuple, is stored in <type>JSON</type> columns
(<literal>local_tuple</literal>
+   and <literal>remote_tuple</literal>) for flexible querying and analysis.
+  </para>

As reported previously [1, comment #5], I didn't see any column called
'local_tuple' --  did you mean to refer to the elements of the
'local_conflicts' array here?

~~~

3.
A previous review comment [1, comment #6 ] suggesting making some
Chapter 29 supsection/s about "Logging conflicts" was not addressed.
You disagreed?

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

4.
A previous review comment [1, comment #13 ] suggesting adding some
links to refer to Chapter 29 for the logging details was not
addressed. You disagreed?

======
[1] my v19-0003 review -
https://www.postgresql.org/message-id/CAHut%2BPtu9-R6x5t%3D2aXdVUR-cjopGxYFEgOjHpUY1jsAfG1drA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Mon, 12 Jan 2026 18:09:35 +1100

Some review comments for v20-0002 (code)

======

ReportApplyConflict:

1.
+ /* Insert to table if requested. */
+ if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
+ {
...
+ }

+ /* Decide what detail to show in server logs. */
+ if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
+ {
...
+ /* Standard reporting with full internal details. */
+ ereport(elevel,
+ errcode_apply_conflict(type),
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
+    get_namespace_name(RelationGetNamespace(localrel)),
+    RelationGetRelationName(localrel),
+    ConflictTypeNames[type]),
+ errdetail_internal("%s", err_detail.data));
+ }
+ else
+ {
+ /*
+ * 'table' only: Report the error msg but omit raw tuple data from
+ * server logs since it's already captured in the internal table.
+ */
+ ereport(elevel,
+ errcode_apply_conflict(type),
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
+    get_namespace_name(RelationGetNamespace(localrel)),
+    RelationGetRelationName(localrel),
+    ConflictTypeNames[type]),
+ errdetail("Conflict details logged to internal table with OID %u.",
+   MySubscription->conflictlogrelid));
+ }

OK, I understand better now what you are trying to do with the logging
-- e.g. I see the brief log is referring to the CLT. But, it seems a
bit strange still that the 'else' of the LOG is asserting there *must*
be a CLT. Perhaps that is a valid assumption today, but if another
kind of destination is supported in the future, then I doubt this
logic is correct.

How about using some variables, like this:

SUGGESTION

bool log_dest_clt = (dest & CONFLICT_LOG_DEST_TABLE) != 0);
bool log_dest_logfile = (dest & CONFLICT_LOG_DEST_LOG) != 0);

if (log_dest_clt)
{
  /* Logging to the CLT */
  ...

  if (!log_dest_logfile)
  {
    /* Not logging conflict details to the server log; instead, write
a brief message referencing this CLT. */
    ereport(elevel, ...
      errdetail("Conflict details logged to internal table with OID %u.", ...);
  }
}

if (log_dest_logfile)
{
  /* Logging to the Server Log */
  ...
}

~~~

2.
+ json_datum_array = (Datum *) palloc_array(Datum, num_conflicts);
+ json_null_array = (bool *) palloc0_array(bool, num_conflicts);

The casts are redundant. The macros are taking care of that.

======

3.
-extern Relation GetConflictLogTableInfo(ConflictLogDest *log_dest);
+extern Relation GetConflictLogDestAndTable(ConflictLogDest *log_dest);
+extern void InsertConflictLogTuple(Relation conflictlogrel);
+extern bool ValidateConflictLogTable(Relation rel);

3a.
AFAICT GetConflictLogTableInfo() should not have existed anyway.

~

3b.
AFAICT ValidateConflictLogTable() is not used anymore. This seems
leftover from some old patch version.

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

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 12 Jan 2026 20:02:49 +0530

> warning: 2 lines add whitespace errors.

Fixed

> schema, at least the impact for checking CLT is contained.

I prefer not to keep that additional function, anyway all the checks
are done using isConflictClass and friends, and in future if we need
to change where something else can come in this conflict namespace it
wouldn't be hard to change the logic.

> been changed to use IsConflictClass()?

Yeah this is not relevant anymore.

> code anymore in this function for CLT.

Right
> code anymore in this function for CLT.

Fixed

> not implemented in patch 0001.

Fixed

Make sense
> be a comment to explain that?

Right that's the reason, added comments for the same

Done

Will send updated patch after checking comments in other emails.

-- 
Regards,
Dilip Kumar
Google

---

From: Peter Smith <[email-masked]>
Date: Tue, 13 Jan 2026 08:54:34 +1100

Hi Dilip.

Some comments about the v20-0002 (test code only)

======
src/test/subscription/t/035_conflicts.pl

1.
+my $subid = $node_subscriber->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'sub_tab';");
+my $conflict_table = "pg_conflict.pg_conflict_$subid";

1a.
Maybe there should be a comment somewhere near here to describe that
the remainder of this test part is for verifying the contents of the
CLT.

~

1b.
Consider renaming the variable '$conflict_table' as '$clt', so it
cannot be confused with "the table that has the conflict".

~~~

2.
+# Wait for the conflict to be logged
+my $log_check = $node_subscriber->poll_query_until(
+    'postgres',
+    "SELECT count(*) > 0 FROM $conflict_table;"
+);

/to be logged/to be logged in the CLT/

~~~

3.
+# Verify that '2' is present in the resulting string
+like($all_keys, qr/2/, 'Verified that key 2 exists in the
local_conflicts log');

The check just for '2' seemed vague. IIUC, this would also match 12,
25, 102, or anything else with a 2 in it. Is it possible for this test
to compare a longer string so it is guaranteed to be a more unique
match for what you are expecting?

~~~

4.
+# Wait for the conflict to be logged
+$log_check = $node_subscriber->poll_query_until(
+    'postgres',
+    "SELECT count(*) > 0 FROM $conflict_table;"
+);
+
+$conflict_check = $node_subscriber->safe_psql('postgres',
+    "SELECT count(*) FROM $conflict_table WHERE conflict_type =
'multiple_unique_conflicts';");
+is($conflict_check, 1, 'Verified multiple_unique_conflicts logged
into internal table');
+
+$json_query = qq[
+    SELECT string_agg((unnested.j::json)->'key'->>'a', ',')
+    FROM (
+        SELECT unnest(local_conflicts) AS j
+        FROM $conflict_table
+    ) AS unnested;
+];
+
+$all_keys = $node_subscriber->safe_psql('postgres', $json_query);
+
+# Verify that '6' is present in the resulting string
+like($all_keys, qr/6/, 'Verified that key 6 exists in the
local_conflicts log');
+

All the same review comments above apply here, too.

- Comment to say all this is for verifying the CLT
- /to be logged/to be logged in the CLT/
- The check just for '6' seemed vague.

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

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 13 Jan 2026 15:58:15 +0530

Done

Done

Done

> match for what you are expecting?

How about matching specifically 2 using like($all_keys, qr/\b2\b/,
'Verified that key 2 exists in the local_conflicts log'); ?
> - The check just for '6' seemed vague.

Done same treatment as above.

I am not sure does it makes sense to check the tuple from clt for all
the test cases in 035_conflict.pl or just checking in a few of them is
sufficient?  Thoughts?

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Tue, 13 Jan 2026 15:58:54 +0530

> Here is the updated patch

Thanks, I will review the code. Few concerns while experimenting with 001 alone:

1)
I am able to insert and update rows in pg_conflict.pg_conflict_16394.
It should be restricted.

2)
I think we need to block 'SELECT for UPDATE' and similar operations on
CLT. Currently it is allowed on CLT.
See this:

postgres=# SELECT * FROM  pg_toast.pg_toast_3466 for UPDATE;
ERROR:  cannot lock rows in TOAST relation "pg_toast_3466"
postgres=# SELECT * FROM pg_conflict.pg_conflict_16394 for UPDATE;
....
(1 row)

3)
We currently skip ANALYZE on TOAST tables, but I’m not sure whether
the same restriction should apply to CLT. Since users are expected to
query CLT frequently, collecting statistics could be beneficial. Even
though there are currently no indexes or other structures to enable
more optimal plans, having statistics should not harm. Thoughts?

postgres=# analyze pg_toast.pg_toast_16399;
WARNING:  skipping "pg_toast_16399" --- cannot analyze non-tables or
special system tables

postgres=# analyze pg_conflict.pg_conflict_16394;
ANALYZE

4)
It will be good to show 'Conflict Log Table:' in \dRs command.

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 13 Jan 2026 16:09:30 +0530

> postgres=# SELECT * FROM pg_conflict.pg_conflict_16394 for UPDATE;

I sent an analysis on this a few days ago but realized it only went to
Amit. Resending to the full list:

So the summary is, currently, all INSERT/UPDATE/DELETE operations are
blocked for TOAST tables for safety. However, I have allowed these
operations for the pg-conflict table. Unlike TOAST, the system does
not internally reference conflict data, so user manipulation doesn't
pose a safety risk to the system. If a user modifies or deletes this
data, they simply lose their logs without impacting system stability.
What are your thoughts on this approach?

>> I  Wrote
> I have been exploring the enforcement of these restrictions.
> Currently, DML is validated via CheckValidResultRel(). If we do not
> explicitly check for the conflict log table in that function, DML
> operations, including DELETE, will be permitted. While I am not overly
> concerned about users attempting manual INSERT or UPDATE operations.

> Now for TRUNCATE, the truncate_check_rel() currently relies on
> IsSystemClass() to restrict truncations. Since the conflict namespace
> is included in IsSystemClass() to prevent unauthorized DDL, TRUNCATE
> is blocked by default. We could allow it by adding an exception in
> truncate_check_rel() (e.g., IsSystemClass() &&
> !IsConflictNamespace()), but I am unsure if this is necessary. Do we
> really need to permit TRUNCATE, or is allowing DELETE sufficient for
> user-driven cleanup?

> postgres=# analyze pg_conflict.pg_conflict_16394;

I think its good to ANALYZE CLT data because user logically never need
to query the toast data, but they would have to query the conflict
data.

> It will be good to show 'Conflict Log Table:' in \dRs command.

Yeah, you raised this point earlier as well, somehow I am missing this
point from the last few versions. I will analyze this and change this
as a first comment in the next version.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Tue, 13 Jan 2026 17:02:39 +0530

> What are your thoughts on this approach?

I don’t see a strong reason for a user to manually perform INSERT or
UPDATE here. But on rethinking, I also agree that allowing it does no
harm. It simply gives the user flexibility to modify data they “own”,
i.e., data that the system does not internally reference. Personally,
I’m okay with leaving it unrestricted, but it will be good to document
it as a NOTE/CAUTION, stating that these DML operations are allowed
and it is the user’s responsibility to manage any changes they make
manually.

> > really need to permit TRUNCATE, or is allowing DELETE sufficient for

I am okay if we allow DELETE alone.

> to query the toast data, but they would have to query the conflict

Right. Do you think we shall allow index creation as well on this
table? It may help if the table is huge. As an example, a user can
create an index on relid to query it faster. Currently it is not
allowed.

> as a first comment in the next version.

Okay.

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 13 Jan 2026 18:15:17 +0530

> and it is the user’s responsibility to manage any changes they make

To clarify, I’m allowing INSERT and UPDATE alongside DELETE not
because I anticipate a specific use case, but to avoid adding
unnecessary code complexity. Since restricting these operations isn't
a safety requirement, I felt it was better to keep the implementation
simple rather than adding extra checks that don't provide real value.

So let's get consensus on this decision and then we can change accordingly.

> I am okay if we allow DELETE alone.

Thanks for your opinion.

> create an index on relid to query it faster. Currently it is not

That’s an interesting point. I initially considered creating internal
indexes to simplify management and avoid requiring users to have DDL
permissions on the pg_conflict schema. However, I realized that
predefined indexes might be too restrictive for diverse search
requirements.  Perhaps we should omit indexes from the initial
version? We can then decide whether to add predefined indexes or allow
external ones based on actual usage patterns and feedback.  Thoughts?

-- 
Regards,
Dilip Kumar
Google

---

From: Peter Smith <[email-masked]>
Date: Wed, 14 Jan 2026 13:05:21 +1100

Some review comments for patch v21-0001.

======

1.
+ /* Subscription conflict log tables are not published */
  if (is_publishable_class(relid, relForm) &&
  !(relForm->relispartition && pubviaroot))
  result = lappend_oid(result, relid);
You replied that this (redundant) comment had been removed, but it is
still in the patch.

======
src/backend/commands/subscriptioncmds.c

2.
+ ConflictLogDest logdest;
  XLogRecPtr lsn;
 } SubOpts;

Should you call this member 'conflictlogdest', in case the future
supports other kinds of subscription logging?

======

3.
+static const ConflictLogColumnDef ConflictLogSchema[] =
+{
+ { .attname = "relid",            .atttypid = OIDOID },
+ { .attname = "schemaname",       .atttypid = TEXTOID },
+ { .attname = "relname",          .atttypid = TEXTOID },
+ { .attname = "conflict_type",    .atttypid = TEXTOID },
+ { .attname = "remote_xid",       .atttypid = XIDOID },
+ { .attname = "remote_commit_lsn",.atttypid = LSNOID },
+ { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "remote_origin",    .atttypid = TEXTOID },
+ { .attname = "replica_identity", .atttypid = JSONOID },
+ { .attname = "remote_tuple",     .atttypid = JSONOID },
+ { .attname = "local_conflicts",  .atttypid = JSONARRAYOID }
+};

Is "local_conflicts" meant to be a JSON array or a JSONB array? I
think there are lots of comments in the patch 0002 describing it as
"JSONB", but I don't know if those comments are accurate or not. If
those comments are correct, then shouldn't it be saying JSONBARRAYOID
here?

TBH, I think the JSONOID/JSONARRAYOID is correct, but something is
fishy and clashing with patch 0002.

~~~

4.
+ /* Convenience flag for all supported destinations */
+ CONFLICT_LOG_DEST_ALL   = (CONFLICT_LOG_DEST_LOG | CONFLICT_LOG_DEST_TABLE)
+} ConflictLogDest;

Is this better described as a "bitmask", instead of a "flag"?

======

5.
+DROP SUBSCRIPTION regress_conflict_protection_test;
+
+
 RESET SESSION AUTHORIZATION;

Double blank lines.

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

---

From: Peter Smith <[email-masked]>
Date: Wed, 14 Jan 2026 13:09:53 +1100

On Tue, Jan 13, 2026 at 9:28 PM Dilip Kumar <[email-masked]> wrote:
...
> the test cases in 035_conflict.pl or just checking in a few of them is

I felt just having some was OK, not everything.

//////////

Some review comments for patch v21-0002.

======
Commit message

1.
Handling Multi-row Conflicts: A single remote tuple may conflict with multiple
local tuples (e.g., in the case of multiple_unique_conflicts). To handle this,
the infrastructure creates a single row in the conflict log table for each
remote tuple. The details of all conflicting local rows are aggregated into a
single JSON array in the local_conflicts column.

The JSON array uses the following structured format:
[ { "xid": "1001", "commit_ts": "2025-12-25 10:00:00+05:30", "origin": "node_1",
"key": {"id": 1}, "tuple": {"id": 1, "val": "old_data"} }, ... ]

~

As mentioned in the patch 0001 review comments, there is some
ambiguity regarding JSON versus JSONB. Here, the commit message is
calling it a "JSON array"; later in the patch, the same field is
called a "JSONB array".

Also, the code refers to type JSONOID in places where I thought maybe
it should be JSONBOID (assuming the JSONB array comment was right).
So, which one is it meant to be? All the code/comments need to be
consistent.

Meanwhile, according to my AI... "Only use JSON if you absolutely need
to preserve exact formatting or if your workload is heavily
write-biased with almost no reads."
Since the CLT is basically write-only, and human-readable would be
nice too, maybe it's JSON that we want here.

======

2.
+ /*
+ * Get both the conflict log destination and the opened conflict log
+ * relation for insertion.
+ */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);

It' not obvious from that comment that the conflictlogrel could be NULL here.

SUGGESTION
Get the conflict log destination. Also, (if there is one) return the
CLT relation already opened and ready for insertion.

~~~

3.

Instead of "internal table", it should be called the "conflict log table".

Also, why not give the real name of the CLT instead of making the user
have to figure it out from the relid?

SUGGESTION
"Conflict details are logged to the conflict log table: %s"

(It might be nicer if you have a common util function for returning
the CLT name "pg_conflict_%u" given the subid)

~~~

4.
+/*
+ * GetConflictLogDestAndTable
+ *
+ * Fetches conflict logging metadata from the cached MySubscription pointer.
+ * Sets the destination enum in *log_dest and, if applicable, opens and
+ * returns the relation handle for the internal log table.
+ */
+Relation
+GetConflictLogDestAndTable(ConflictLogDest *log_dest)

/for the internal log table./for the conflict log table./

======

5.
+my $json_query = qq[
+    SELECT string_agg((unnested.j::json)->'key'->>'a', ',')
+    FROM (
+        SELECT unnest(local_conflicts) AS j
+        FROM $clt
+    ) AS unnested;
+];
+
+my $all_keys = $node_subscriber->safe_psql('postgres', $json_query);
+
+# Verify that '2' is present in the resulting string
+like($all_keys, qr/\b2\b/, 'Verified that key 2 exists in the
local_conflicts log');

I noticed you are jumping through some hoops to extract fields from
the JSON before checking their content. If the 'local_conflicts'
really is stored as human-readable JSON (not JSONB), then maybe that
extraction work is not necessary. e.g. consider just matching against
a portion of the JSON string, instead of having to unnest/extract
anything at all.

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

---

From: Peter Smith <[email-masked]>
Date: Wed, 14 Jan 2026 13:17:45 +1100

Some review comments for patch v21-0003.

======

(v21-0003 is identical to v20-0003. None of my v20-0003 review
comments have been addressed yet.)

~~~

Anyway, here is one more review comment:

1.
The DOCS for CREATE SUBSCRIPTION say:
+             <literal>pg_conflict</literal> schema. This allows for
easy querying and
+             analysis of conflicts. This table is automatically
dropped when the
+             subscription is removed.

The DOCS for ALTER SUBSCRIPTION also say:
+      exist. Conversely, if the destination is changed to
+      <literal>log</literal>, logging to the table stops and the internal
+      table is automatically dropped.

~

I felt these topics about the CLT auto-removal need to be more
prominent, like in a <caution> note or something similar, to recommend
that the user must make a *backup* of the CLT first, if they intend to
do post-mortem analysis of the conflict. Maybe the best place to put
this <caution> is in the CREATE SUBSCRIPTION / Parameters /
conflict_log_desttination = table section.

IMO "post-mortem" implies first dropping the subscription (e.g.
because it might be in some kind of apply retry-loop conflicting over
and over and over), and then studying the logs/CLT to figure out where
it went wrong. Therefore, it would be bad if the user wants to exactly
that kind of study, only to find that they've accidentally also
deleted all the CLT evidence when they dropped the subscription.

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

---

From: Amit Kapila <[email-masked]>
Date: Wed, 14 Jan 2026 15:59:31 +0530

> simple rather than adding extra checks that don't provide real value.

What kind of complexity you are anticipating, can you show by top-up
patch? I think allowing just DELETE and TRUNCATE to table owners would
be ideal.

> external ones based on actual usage patterns and feedback.  Thoughts?

I agree that the indexes could be considered later unless required by
internal code to search the conflict table.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Wed, 14 Jan 2026 17:09:26 +0530

> patch? I think allowing just DELETE and TRUNCATE to table owners would

I mean by code complexity i.e. additional check for DELETE/TRUNCATE,
but if we think that's ideal, I can update it.

> internal code to search the conflict table.

Thanks.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 19 Jan 2026 11:38:31 +0530

> That should be more consistent with the wording used elsewhere in this patch.

Done in v23-0003

> CREATE SUBSCRIPTION way looked good to me.

Done as suggested

> 'local_conflicts' array here?

Fixed and adjusted comments accordingly
> Chapter 29 supsection/s about "Logging conflicts" was not addressed.

I am not sure if we need separate sections for this, so does anyone
else have an opinion on this?

> addressed. You disagreed?

I have changed this but it is not included in v23, so will be sent
along with the next version.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Tue, 20 Jan 2026 16:08:33 +0530

> > postgres=# SELECT * FROM pg_conflict.pg_conflict_16394 for UPDATE;

I had a quick look at v23-001, I see that it has INSERT and UPDATE
blocked but SELECt for UPDATE is still working. I do not see any
strong need to provide this flexibility for user-level concurrency
control on conflict-tables. Shall this be blocked as well?

Also, the error I get for INSERT and UPDATE is:

postgres=# update pg_conflict.pg_conflict_16390 set relid=1;
ERROR:  cannot modify or insert data for conflict log table "pg_conflict_16390"
DETAIL:  Conflict log tables are system-managed and only support
cleanup via DELETE or TRUNCATE.

I feel "cannot modify or insert data into conflict log table ..."
seems more natural.

thanks
Shveta