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

From: Dilip Kumar <[email-masked]>
Date: Tue, 27 Jan 2026 09:23:43 +0530

On Tue, Jan 27, 2026 at 8:39 AM Peter Smith <[email-masked]> wrote:
>
> Hi Dilip.
>
> Some comments for the first 2 patches:
>
> //////////
> v24-00001
> //////////
>
> 1.
> + /*
> + * Conflict log tables are managed by the system to record logical
> + * replication conflicts.  We do not allow locking rows in CONFLICT
> + * relations.
> + */
> + if (IsConflictNamespace(RelationGetNamespace(rel)))
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("cannot lock rows in CONFLICT relation \"%s\"",
> + RelationGetRelationName(rel))));
>
> AFAIK, this "CONFLICT relation" terminology is not used anywhere else.
>
> Why not just call it what it is:
>
> e.g.
> cannot lock rows in conflict log table \"%s\"
>
> ~
>
> OTOH, if you were attempting to future-proof the message for different
> kinds of relations in the 'pg_conflict' namespace, I still felt it
> might be better to refer to 'pg_conflict' instead of CONFLICT:
>
> e.g.
> cannot lock rows in 'pg_conflict' relation \"%s\"

I prefer conflict log tables for consistency with other places.

> //////////
> v24-0002
> //////////
>
> 1.
> +static char *build_index_value_desc(EState *estate, Relation localrel,
> + TupleTableSlot *slot, Oid indexoid);
>
> Declared twice?

Removed duplicate.

Also fixed all pending doc comments.

-- 
Regards,
Dilip Kumar
Google

---

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

Some review comments for the v25* patches:

//////////
Patch v25-0001
//////////

1.
+ /*
+ errmsg("cannot lock rows in conflict log table \"%s\"",

I felt the code comment should be changed, too.

e.g. That comment sentence "We do not allow locking rows in CONFLICT
relations" seemed redundant because you already said the CLT is
"managed by the system", and also it is basically just repeating the
same info as the error message.

Suggest either:
a) remove it, or
b) change /CONFLICT/pg_conflict namespace/

//////////
Patch v25-0002
//////////

No review comments.

//////////
Patch v25-0003 (docs)
//////////

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

(29.8 Conflicts)

1.
There is an earlier sentence on this page:
"Note that there are other conflict scenarios, such as exclusion
constraint violations. Currently, we do not provide additional details
for them in the log."

~

That "in the log part" wording maybe needs to be changed/removed now
because the destination might be a CLT, not a log.

SUGGESTION:
Currently, we do not provide additional details for them.

~~~

2.
   <para>
-   The log format for logical replication conflicts is as follows:
+   The <link linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
+   parameter can automatically creates a dedicated conflict log
table.  This table is created in the dedicated
+   <literal>pg_conflict</literal> namespace.  The name of the
conflict log table
+   is <literal>pg_conflict_&lt;subid&gt;</literal>. The predefined
schema of this table is
+   detailed in
+   <xref linkend="logical-replication-conflict-log-schema"/>.
+  </para>
+

2a.
Typo: /can automatically creates/can automatically create/

~

2b.
It sounds a bit strange to say "a dedicated" 2x in 2 sentences. Maybe
omit the first one:

SUGGESTION
... can automatically create a conflict log table.

~

2c.
Perhaps the "conflict log table" should be using <firstterm> SGML markup here.

~~~

3.
+  <para>
+   The conflicting row data, including the incoming remote row
(<literal>remote_tuple</literal>)
+   and the associated local conflict details
(<literal>local_conflicts</literal>), is stored in
+   <type>JSON</type> formats, for flexible querying and analysis.
+  </para>

/formats, for/format for/

~~~

4.
+  <para>
+   If <link linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
+   is set to log conflicts to the server log, the following format is used:

I'm not sure you need that link because the same parameter was already
linked a bit earlier on this same page.

~~~

(29.9 Restrictions)

5.
+   <listitem>
+    <para>
+     Conflict log tables (see <link
linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
parameter)
+     are never published, even when using FOR ALL TABLES in a publication.
+    </para>
+   </listitem>

That FOR ALL TABLES ought to have <literal> SGML markup.

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

6.
I still feel that somewhere in here, there ought to be some link/s
back to the "29.8 Conflicts" details about the CLT schema and LOG
formats.

SUGGESTION
conflict_log_destination / log: ... See <link> for the format used to
log conflict details.

conflict_log_destination / table: ... See <link> for the predefined
schema of the conflict log table.

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

---

From: shveta malik <[email-masked]>
Date: Wed, 28 Jan 2026 17:33:48 +0530

> Also fixed all pending doc comments.

Thanks for the patch, Please find a few comments:

patch0001:

1)
+/*
+ * GetLogDestination
+ *
+ * Convert string to enum by comparing against standardized labels.
+ */
+ConflictLogDest
+GetLogDestination(const char *dest)

IMO the name of the function should be GetConflictLogDest, otherwise
the current name might suggest it applies to all the logs related to
subscription, which is misleading.

2)
+ { .attname = "schemaname",       .atttypid = TEXTOID },

I checked the catalog tables known to me which refer to namespace, all
use namespace or nsp in their names, none use 'schema'. Examples:
pg_namespace, pg_class, pg_type. pg_constraint.  Shall we use nspname
instead?

<No issues found in my local testing for 0001>

~~

patch0002:

1)
conflict.c compiles without these:

+#include "utils/fmgroids.h"
+#include "utils/json.h"

2)
+ /*
+ * Get the conflict log destination. Also, (if there is one) return the
+ * CLT relation already opened and ready for insertion.
+ */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);

'CLT relation' means conflict log table relation. Shall we correct it
to mention either table alone or relation alone?

3)
This is defined in conflict.h:

+/* The single source of truth for the conflict log table schema */
+static const ConflictLogColumnDef ConflictLogSchema[] =
+{
....
+ { .attname = "local_conflicts",  .atttypid = JSONARRAYOID }
+};

while its element 'local_conflicts' is defined in conflict.c:

+/* Schema for the elements within the 'local_conflicts' JSON array */
+static const ConflictLogColumnDef LocalConflictSchema[] =
+{
...
+};

It takes some time to figure this part out as a reader of code. I
think we shall define LocalConflictSchema schema immediately after
ConflictLogSchema for anyone to understand it better, unless there is
something blocking it?

4)
+# Verify that '2' is present inside the JSON structure using a regex
+# This matches the key/value pattern for "a": 2
+like($raw_json, qr/\\"a\\":2/, 'Verified that key 2 exists in the
local_conflicts');
+

Reviewing further. Testing of 0002 yet to be finished.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Wed, 28 Jan 2026 17:36:56 +0530

> +like($raw_json, qr/\\"a\\":2/, 'Verified that key 2 exists in the

Oops, I missed adding my feedback for pt4 earlier, here it is:

To properly validate correctness, given that local_conflicts is an
array of multiple local tuple details here, we should also check that
it contains the keys b:3 and c:4.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Thu, 29 Jan 2026 07:59:45 +1100

On Wed, Jan 28, 2026 at 11:04 PM shveta malik <[email-masked]> wrote:

...
> ConflictLogSchema for anyone to understand it better, unless there is

+1.

I had asked for this same change last year [1 - #17]

======
[1] https://www.postgresql.org/message-id/CAHut%2BPtSggpJH36YOwdfmY5gU6yr7Wa-%3Dreht4c2v%2Bn8FYUKJg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Thu, 29 Jan 2026 11:03:05 +0530

> it contains the keys b:3 and c:4.

Few more comments on 002:

5)
+ bool log_dest_clt;
+ bool log_dest_logfile;

Do we need 'clt' in the variable name? It would be clearer to name it
log_dest_table. Having said that, neither of these names clearly
indicate a boolean value. Will it be better to name these as
logToTable and logToLogFile?

6)
+/*
+ * Helper function to extract the "raw" index key Datums and their null flags
+ * from a TupleTableSlot, given an already open index descriptor.
+ * This is the reusable core logic.
+ */
+static void
+build_index_datums_from_slot(EState *estate, Relation localrel,
+ TupleTableSlot *slot,
+ Relation indexDesc, Datum *values,
+ bool *isnull)

We should mention which ones are output variables here. Something like:

Helper function to extract...to values[] and isnull[] arrays.
<or any other way>

7)
In build_index_datums_from_slot(), shall we check/assert that 'values'
and 'isnull' are non-null before passing it to FormIndexDatum.

8)
+static TupleDesc
+build_conflict_tupledesc(void)

The name gives an impression that we are building a full
tuple-descriptor for CLT, while it is only for local_conflicts.  Shall
we name it to build_tupledesc_for_local_conflicts or
build_local_conflicts_tupledesc. It aligns with its caller as well
'build_local_conflicts_json_array'.

9)
build_local_conflicts_json_array():
+ int i;

+ i = 0;
+ foreach(lc, json_datums)
+ {
+ json_datum_array[i] = (Datum) lfirst(lc);
+ i++;
+ }

I think variable 'i' and its usage is leftover from some debugging.

10)
There are few new functions such as tuple_table_slot_to_json_datum,
tuple_table_slot_to_indextup_json, build_conflict_tupledesc,
prepare_conflict_log_tuple which mention function name in
header-comment section as well, while other functions do not. We can
keep header-comment section of all functions in the same format. I
think we generally do not mention function names explicitly in the
header.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 29 Jan 2026 11:37:42 +0530

Few comments on 0003:

1)
Section '29.2. Subscription' has this:

~~
Conflicts that occur during replication are, by default, logged as
plain text in the server log, which can make automated monitoring and
analysis difficult. The CREATE SUBSCRIPTION command provides the
conflict_log_destination option to record detailed conflict
information in a structured, queryable format. When this parameter is
set to table or all, the system automatically manages a dedicated
conflict log table, which is created and dropped along with the
subscription. This significantly improves post-mortem analysis and
operational visibility of the replication setup.
~~

This new section begins by discussing log-destination without
providing much background on conflicts, which makes it feel somewhat
out of place. It could be made more general by first referring the
reader to the 'Conflicts' page. How about this:

Conflicts can occur during logical replication when changes from the
publisher cannot be applied on the subscriber, for example due to
constraint violations or concurrent data modifications. An overview of
possible conflict types is provided in Section 29.4, Logical
Replication Conflicts. By default, conflict information is written to
the server log. The CREATE SUBSCRIPTION command provides the
conflict_log_destination parameter, which allows conflict details to
be recorded in a dedicated table, making post-mortem analysis and
operational monitoring easier.

2)
 '29.8. Conflicts' section has this:
~~
Note that there are other conflict scenarios, such as exclusion
constraint violations. Currently, we do not provide additional details
for them in the log.

The conflict_log_destination parameter can automatically creates a
dedicated conflict log table. This table is created in the dedicated
pg_conflict namespace. The name of the conflict log table is
pg_conflict_<subid>. The predefined schema of this table is detailed
in Table 29.3.
~~

This phrasing feels somewhat abrupt and lacks context. A clearer
version could be:

By default, conflict information is written to the server log. The
CREATE SUBSCRIPTION command also provides the conflict_log_destination
parameter to record detailed conflict information in a structured,
queryable format. When this parameter is set to table or all, the
system automatically
manages a dedicated conflict log table, which is created and dropped
along with the subscription. The table is created in the pg_conflict
schema and is named pg_conflict_<subid>. The schema of this table is
described in Table 29.3.

3)
'Restrictions' section has this:
Conflict log tables (see conflict_log_destination parameter) are never
published, even when using FOR ALL TABLES in a publication.

We shall also mention the restriction of publishing 'for tables in
schema pg_conflict'.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 29 Jan 2026 16:46:10 +0530

Dilip, I was testing patch002, few observations:

1)
remote_commit_ts is '2026-01-29 14:38:02.777599+05:30' while
local_conflicts.commit_ts is of format
'2026-01-29T14:37:35.805819+05:30' i.e. a 'T' to separate date and
time in local-commit_ts case. IIUC, this is the expected (standard)
way for timestamps to be in json-format. Can you please confirm?

2)
Differences between LOG and TABLE:

a) In LOG, the column names are not included in the remote or local
tuples, whereas in the TABLE, they are. For example:

LOG: Could not apply remote change: remote row (2, 3, 4)
TABLE: remote_tuple: {"a":2,"b":3,"c":4}

b) remote_commit_ts is not recorded in LOG, but it is included in the TABLE.

IMO, both of these differences are okay as TABLE is expected to store
(or can store) more detailed information. I’m just noting these here
for others’ input. Let me know your thoughts.

3)
If we look at the table overall, the name 'local_conflicts' can be a
bit misleading. This column actually stores the details about local
rows involved in a conflict, rather than describing the conflict
itself. See example below. A clearer name might be local_data,
local_tuples_data, or local_tuples_detail. Thoughts?

local_conflicts for one update_exists:
{"{\"xid\":\"791\",\"commit_ts\":\"2026-01-29T14:37:35.805819+05:30\",\"origin\":null,\"key\":{\"i\":40},\"tuple\":{\"i\":40,\"j\":10}}"}
~~

patch002 works well in my testing done so far.

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 30 Jan 2026 12:05:59 +0530

> way for timestamps to be in json-format. Can you please confirm?

Yes, I will check that and respond.

> for others’ input. Let me know your thoughts.

Yeah, I think we do need additional data for table e.g. column name in
tuple so that users can use queries to extract value for specific
columns etc.

> {"{\"xid\":\"791\",\"commit_ts\":\"2026-01-29T14:37:35.805819+05:30\",\"origin\":null,\"key\":{\"i\":40},\"tuple\":{\"i\":40,\"j\":10}}"}

IMHO can we name it local_conflict_tuples?  Because remote tuples may
conflict with multiple local tuples, I think local_conflict_tuple
might be more relevant.  But OTOH we are storing other information as
well, not just tuples so maybe
local_tuples_detail/local_tuples_info/local_tuples_data make more
sense.  So maybe I am fine with 'local_tuples_detail' lets see if
there are any other thoughts?

-- 
Regards,
Dilip Kumar
Google

---

From: Nisha Moond <[email-masked]>
Date: Mon, 18 May 2026 14:42:14 +0530

While testing with all patches(v34) applied, I noticed an unexpected
behavior change in \dRs+ output.

I see that we changed the \dRs+ output format to display "Conflict log
table:" separately instead of as a column, but the output ordering
also seems to have changed.

Without the patch, both \dRs and \dRs+ display subscriptions in
alphabetical order by name. With this patch, \dRs still shows the
expected ordering, but \dRs+ now appears ordered by subscription
creation order (likely subid) instead of subscription name.

This is not a major issue, but it seems to break consistency. For
example, \dRp+ has a similar display pattern, but its output is
ordered by pub-name.

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Mon, 18 May 2026 18:05:40 +0530

On Wed, 13 May 2026 at 11:43, Peter Smith <[email-masked]> wrote:
>
> Hi Dilip/Vignesh.
>
> Some review comments for v33-0001.
>
> ======
> src/backend/executor/execMain.c
>
> 11.
> +
> + /*
> + * replication conflicts.  We allow DELETE and TRUNCATE to permit users to
> + * manually prune these logs, but manual data insertion or modification
> + * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of the
> + * system-generated logs.
> + *
> + * Since TRUNCATE is handled as a separate utility command, we only need
> + * to explicitly permit CMD_DELETE here.
> + */
> + 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.")));
>
> It somehow feels backwards to check "operation != CMD_DELETE", with
> the obscure comment that TRUNCATE is handled elsewhere.
>
> How about just check if "(operation == CMD_INSERT || operation ==
> CMD_UPDATE || operation == CMD_MERGE)".

I felt the existing is ok here, as it is mentioned "we only need to
explicitly permit CMD_DELETE" . Are you seeing any commands other than
INSERT, UPDATE & MERGE possible here?

> 12.
> I was not sure what was meant by "CONFLICT relations.".
>
> Does it mean "... relations in the pg_conflict schema.". Anyway, is
> there any value to that 2nd sentence because it is much the same text
> as the errmsg.

 Yes, it means the relations in pg_conflict schema. Removed the second sentence.

> ======
> src/backend/replication/logical/conflict.c
>
> 13.
> +const char *const ConflictLogDestNames[] = {
> + [CONFLICT_LOG_DEST_LOG] = "log",
> + [CONFLICT_LOG_DEST_TABLE] = "table",
> + [CONFLICT_LOG_DEST_ALL] = "all"
> +};
> +
> +const ConflictLogColumnDef v[] = {
> + { .attname = "relid",            .atttypid = OIDOID },
> + { .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 },
> 13a.
> Both these arrays could benefit with some comments.

Added comments

> ~
>
> 13b.
> In the ConflictLogSchema, would it be better to keep all those
> "remote_" columns grouped together, instead of being broken by
> "replica_identity".

Modified

> ~
>
> 13c.
> TBH, I preferred code how it used to be -- where all the CLT constants
> and structs and enums and schemas were kept together. Now they are
> split across conflict.h and conflict.c making it harder to read as
> well as introducing need for static asserts that were not needed
> before.

No change done, as this change is required. Amit has given the
explanation at [1].

Rest of the comments were addressed. The attached v35 version patch
has the changes for the same.

I have kept the review comment fixes as separate patches so that Dilip
can merge them when convenient. Due to the additional review-fix
patches, Dilip's original patches 0001, 0002, 0003, and 0004 are now
renumbered as 0001, 0003, 0005, and 0007 respectively.  The
intermediate patches contain the review comment fixes:
a) 0002 contains fixes for 0001 b) 0004 contains fixes for 0003 c)
0006 contains fixes for 0005 d) 0008 contains fixes for 0007

Also comments from [2] and [3] are addressed in this.

[1] - https://www.postgresql.org/message-id/CAA4eK1Ki5mBgAubBkUPcBjN%3DO1jeT3AUh7vLQBm8w%3DgQiHO5Jw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHut%2BPv%2BBK7iM3KZNcrXzPMYagrL2O4%3D46Hi3stT2XT-RmsjRQ%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CAJpy0uARoVZkTA_PV4PB1MtUXZMyxkun1Cg5o1YOxaKsCbWxCA%40mail.gmail.com

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Tue, 19 May 2026 16:31:51 +1000

> On Wed, 13 May 2026 at 11:43, Peter Smith <[email-masked]> wrote:

Hi Vignesh.

Thanks for addressing lots of my previous v33-0001 review comments.

Here are some more review comments for the combined v35-0001/0002 patches.

======
Commit message.

1.
If the user chooses to enable logging to a table (by selecting 'table'
or 'all'),
an internal logging table named pg_conflict_log_<subid> is automatically
created within a dedicated, system-managed 'pg_conflict' namespace to prevent
users from manually dropping or altering it. This also prevents accidental
name collisions with user-created tables. This table is linked to the
subscription via an internal dependency, ensuring it is automatically dropped
when the subscription is removed

~

The internal name of the CLT table has changed slightly, so the commit
message needs updating.

======
src/backend/catalog/heap.c

2.
+ * Don't allow creating relations in pg_catalog/pg_conflict directly, even
+ * though it is allowed to move user defined relations there. Semantics
+ * with search paths including pg_catalog are too confusing for now.

I think "pg_catalog/pg_conflict" could be misinterpreted. Better to
say "pg_catalog or pg_conflict".

~~~

3.
+ if (!allow_system_table_mods && IsNormalProcessingMode())
+ {
+ if ((IsCatalogNamespace(relnamespace) && relkind != RELKIND_INDEX) ||
+ IsToastNamespace(relnamespace))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to create \"%s.%s\"",
+ get_namespace_name(relnamespace), relname),
+ errdetail("System catalog modifications are currently disallowed.")));
+ }
+
+ if (IsConflictNamespace(relnamespace))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to create \"%s.%s\"",
+ get_namespace_name(relnamespace), relname),
+ errdetail("Conflict schema modifications are currently disallowed.")));
+ }
+ }

The curly-braces are unnecesary for those nested if-blocks.

======
src/backend/catalog/namespace.c

CheckSetNamespace:

4.
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move objects into or out of pg_conflict schema")));

Is it better to say "the pg_conflict schema".

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

5.
-

Looks like this was some unintended whitespace removal just after the
static function forward declarations.

~~~

AlterSubscription:

6.
+ bool want_table = (opts.conflictlogdest == CONFLICT_LOG_DEST_TABLE ||
+    opts.conflictlogdest == CONFLICT_LOG_DEST_ALL);
+ bool has_oldtable = (old_dest == CONFLICT_LOG_DEST_TABLE ||
+ old_dest == CONFLICT_LOG_DEST_ALL);

These should be simplified using the new macro: CONFLICTS_LOGGED_TO_TABLE.

======
src/backend/commands/tablecmds.c

DropSubscription:

7.
+ ObjectAddress object;

This can be declared at the lower scope closer to where it is actually used.

~~~

8.
+ if (OidIsValid(form->subconflictlogrelid))
+ {
+ char *conflictrelname = get_rel_name(form->subconflictlogrelid);
+ /*

There should be a blank line before that block comment.

> INSERT, UPDATE & MERGE possible here?

9.
YMMV.

No, I'm not seeing other commands. I guess the current code works.

My previous review comment was because:
1. IMO, conditions that are positive instead of negative are easier to
comprehend
2. It would make the checking code consistent with the comment
“(INSERT, UPDATE, MERGE) is prohibited”, and with the error message
“cannot modify or insert”.
3. Doing it the suggested way eliminates any need to mention that
strange comment “Since TRUNCATE…”

CheckValidRowMarkRel:

10.
+ ereport(ERROR,

Should that say "in the"?

======

> No change done, as this change is required. Amit has given the

By refactoring the conflict functions into conflict.c, it means nearly
everything is now kept together anyhow, just in the .c file instead of
the .h file :-)

~~~

11.
+StaticAssertDecl(lengthof(ConflictLogSchema) == NUM_CONFLICT_ATTRS,
+ "ConflictLogSchema length mismatch");
+
+

11a.
In fact, NUM_CONFLICT_ATTRS is not used outside this file, so now it
can be defined right here. It means the assertion is unnecessary.

Instead, the code here should look like:
#define NUM_CONFLICT_ATTRS lengthof(ConflictLogSchema)

~

11b.
Unnecessary extra whitespace here.

~~~

create_conflict_log_table:

12.
+ Assert(relid != InvalidOid);

Favour using the macro OidIsValid(relid).

======
src/include/catalog/pg_subscription.h

13.
 #include "catalog/objectaddress.h"
 #include "parser/parse_node.h"
+#include "replication/conflict.h"

I am guessing that this #include is probably no longer needed, because
you removed the extern function that was using ConflictLogDest.

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

14.
+/* Structure to hold metadata for one column of the conflict log table */
+typedef struct ConflictLogColumnDef
+{
+ const char *attname;    /* Column name */
+ Oid         atttypid;   /* Data type OID */
+} ConflictLogColumnDef;
+

AFAIK, you can move this into conflict.c now because it is only used
in that file.

~~~

15.
+/* The single source of truth for the conflict log table schema */
+extern PGDLLIMPORT const ConflictLogColumnDef ConflictLogSchema[];
+

AFAIK, you can remove this because all usages are now within conflict.c.

~~~

16.
+#define NUM_CONFLICT_ATTRS 11
+

Move this into conflict.c -- see an earlier review comment.

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

---

From: vignesh C <[email-masked]>
Date: Tue, 19 May 2026 19:30:43 +0530

On Fri, 15 May 2026 at 15:59, Nisha Moond <[email-masked]> wrote:
>
> Thanks for the patches. Please find below comments for v34 patch-set.
>
> patch-003:
> 4) conflict.c: ReportApplyConflict()
> + bool log_dest_clt = false;
> + bool log_dest_logfile;
>
> log_dest_logfile should also be initialized to false, since for dest
> == CONFLICT_LOG_DEST_TABLE, it is never assigned.

It is not required to be initialized now as it is being assigned
before used in this function now.

> 5) worker_internal.h
>  extern PGDLLIMPORT List *table_states_not_ready;
>
> +extern XLogRecPtr remote_final_lsn;
> +extern TimestampTz remote_commit_ts;
> +extern TransactionId remote_xid;
>
> Should these new declarations also use PGDLLIMPORT?

I think these don't require PGDLLIMPORT as it will be used by the same
apply worker backend process.

Rest of the comments are handled, the attached v36 version patches
have the changes for the same.
Also the comment from [1] has been fixed in this version.

[1] - https://www.postgresql.org/message-id/CABdArM5XgHE4-HCryi54BxENgNqLDn81cMCUyqBdCeF9d3dbvA%40mail.gmail.com

Regards,
Vignesh

---

From: shveta malik <[email-masked]>
Date: Wed, 20 May 2026 12:02:11 +0530

> Also the comment from [1] has been fixed in this version.

Thanks Vignesh.

A few comments for 0001 and 002 combined (I merged them and reviewed
for ease of review)

1)

+ * IsConflictLogTableClass
+ * True iff namespace is pg_conflict.
+ *
+ * Does not perform any catalog accesses.
  */
 bool
-IsConflictClass(Form_pg_class reltuple)
+IsConflictLogTableClass(Form_pg_class reltuple)

I think this function is trying to find if the reltuple is a CLT
rather than namepspace is pg_conflict.
We should change this comment. See IsToastRelation, IsToastClass.

Suggestion:
True iff Form_pg_class tuple represents a subscription-specific
Conflict Log Table.

2)

Both DropSubscription and AlterSubscription has below code to drop CLT:

+ if (OidIsValid(subconflictlogrelid))
+ {
+ char *conflictrelname = get_rel_name(subconflictlogrelid);
+
+ /*
+ * Conflict log tables are recorded as internal dependencies of the
+ * subscription.  We must drop the dependent objects before the
+ * subscription itself is removed.  By using
+ * PERFORM_DELETION_SKIP_ORIGINAL, we ensure that only the conflict log
+ * table is reaped while the subscription remains for the final
+ * deletion step.
+ */
+ ObjectAddressSet(object, SubscriptionRelationId, subid);
+ performDeletion(&object, DROP_CASCADE,
+ PERFORM_DELETION_INTERNAL |
+ PERFORM_DELETION_SKIP_ORIGINAL);
+
+ ereport(NOTICE,
+ errmsg("dropped conflict log table \"%s\" for subscription \"%s\"",
+    get_qualified_objname(PG_CONFLICT_NAMESPACE, conflictrelname),
+    subname));
+ }

Why don't we create a function
drop_conflict_log_table(subconflictlogrelid) and use it both places.

3)
+++ b/src/backend/commands/subscriptioncmds.c

+#include "catalog/heap.h"
+#include "catalog/pg_am_d.h"

It compiles now without these inclusion. 002 should remove these as well.

4)
AlterSubscription:

Shall we replace checks at both places with CONFLICTS_LOGGED_TO_TABLE

~~

003,004: No comments

~~

Reviewing further.

thanks
Shveta

---

From: vignesh C <[email-masked]>
Date: Wed, 20 May 2026 15:05:13 +0530

> The internal name of the CLT table has changed slightly, so the commit

This change is done as part of 0002 review comment fixes patch. I will
let Dilip do this change when he merges the review comment fixes patch
to 0001 patch.

> No, I'm not seeing other commands. I guess the current code works.

I preferred the current way in this case.

> everything is now kept together anyhow, just in the .c file instead of

No change done here because of the reason stated in the earlier mail.

Rest of the comments were fixed.
The attached v37 version patch has the changes for the same. Also
Peter's comments on the documentation patch from [1] and Shveta's
comments from [2] are addressed in the attached patch.

[1] - https://www.postgresql.org/message-id/CAHut%2BPsrnU2BB1%2BM3c%2BDr5h62BLYfwBzhTg%3DBM7QtBoPwHYrKw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAJpy0uCX53c40xopqmHtWSWBmh78BqhLVGXa88fU42eOi6w%2BLQ%40mail.gmail.com

Regards,
Vignesh

---

From: shveta malik <[email-masked]>
Date: Wed, 20 May 2026 16:12:02 +0530

> [2] - https://www.postgresql.org/message-id/CAJpy0uCX53c40xopqmHtWSWBmh78BqhLVGXa88fU42eOi6w%2BLQ%40mail.gmail.com

I have not yet looked at v37. But here are a few comments on v36-005,
006. I have merged them and reviewed together.

1)

conflict.c compiles without above inclusions.

2)

A better and more clear name would be log_dest_table instead of
log_dest_clt here.

3)
@@ -6069,6 +6049,8 @@ DisableSubscriptionAndExit(void)
  */
  pgstat_report_subscription_error(MyLogicalRepWorker->subid);

+ ProcessPendingConflictLogTuple();

It does not look obvious as in why we are trying to process
conflict-tuple during disable-subscription? A comment will help here.

4)
tuple_table_slot_to_indextup_json():

+ indexDesc = index_open(indexid, NoLock);
+
+ build_index_datums_from_slot(estate, localrel, slot, indexDesc, values,
+ isnull);
+ tupdesc = RelationGetDescr(indexDesc);
+
+ /* Bless the tupdesc so it can be looked up by row_to_json. */
+ BlessTupleDesc(tupdesc);

We get the index's relcache pointer and pass it directly to
BlessTupleDesc which internally changes it by assigning tdtypmod. Is
this intentional i.e. do we want to change the relcache entry of index
directly? Shouldn't we copy it (CreateTupleDescCopy) and then Bless
it?

5)
build_conflict_tupledesc() does 'CreateTemplateTupleDesc' and Bless it
each time the conflict is raised. Since the tuple-descriptor here is
not going to change, IMO, it will be better to create and bless it
once and reuse it everytime. We can have a 'static' TupleDesc here.
Thoughts?

thanks
Shveta

---

From: Shlok Kyal <[email-masked]>
Date: Wed, 20 May 2026 16:20:34 +0530

> [2] - https://www.postgresql.org/message-id/CAJpy0uCX53c40xopqmHtWSWBmh78BqhLVGXa88fU42eOi6w%2BLQ%40mail.gmail.com
Hi Vignesh,
Here are some minor comments:

Comment for all patches.
1. At multiple places (code comments and test cases) we are using the
word 'internal conflict log table'.
Do we need to use the word 'internal'? I think using 'conflict log
table' is sufficient?

Comments for 0002:
2. We can rename the schema pg_conflict to a different schema name.
Is it ok to hardcode the schema name to 'pg_conflict'?
-                errmsg("cannot move objects into or out of CONFLICT schema")));
+                errmsg("cannot move objects into or out of
pg_conflict schema")));

Example:
postgres=# ALTER SCHEMA pg_conflict RENAME TO sc1;
ALTER SCHEMA
postgres=# ALTER TABLE t2 SET SCHEMA sc1;
ERROR:  cannot move objects into or out of pg_conflict schema

Comment for 0005/0006:
3.
static const char *const ConflictTypeNames[] = {
    [CT_INSERT_EXISTS] = "insert_exists",
    [CT_UPDATE_ORIGIN_DIFFERS] = "update_origin_differs",
    [CT_UPDATE_EXISTS] = "update_exists",
    [CT_UPDATE_MISSING] = "update_missing",
    [CT_DELETE_ORIGIN_DIFFERS] = "delete_origin_differs",
    [CT_UPDATE_DELETED] = "update_deleted",
    [CT_DELETE_MISSING] = "delete_missing",
    [CT_MULTIPLE_UNIQUE_CONFLICTS] = "multiple_unique_conflicts"
};
There are a few extra blank lines after declaration of ConflictTypeNames.

Thanks,
Shlok Kyal

---

From: Peter Smith <[email-masked]>
Date: Thu, 21 May 2026 10:02:25 +1000

On Wed, May 20, 2026 at 8:50 PM Shlok Kyal <[email-masked]> wrote:
..
> ERROR:  cannot move objects into or out of pg_conflict schema

Yikes!

I am not sure that the error message is the problem here. There are
worse things that are similar to this. e.g. I found that you can do
the same trick of renaming the 'pg_catalog' schema, and it breaks
anything that refers to that schema by name -- all the internal SQL!!

test_pub=# ALTER SCHEMA pg_catalog RENAME TO mycatalog;
ALTER SCHEMA
test_pub=# \dRp+
ERROR:  relation "pg_catalog.pg_publication" does not exist
LINE 9: FROM pg_catalog.pg_publication
             ^

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

---

From: Peter Smith <[email-masked]>
Date: Thu, 21 May 2026 11:19:18 +1000

Hi Vignesh.

I checked the latest v37-0001/0002 patches combined.

My only comment is below.

======

1.
+/*
+ * drop_conflict_log_table
+ *      Drop the conflict log table associated with a subscription.
+ *
+ * The conflict log table is registered as an internal dependency of the
+ * subscription. This function removes the dependency by performing a
+ * cascading deletion on the subscription object, which in turn drops the
+ * associated conflict log table.
+ *
+ * This is used to clean up conflict log tables that are no longer required,
+ * preventing accumulation of stale or orphaned relations.
+ *
+ * NOTE:
+ * Only conflict log tables are currently managed via this internal dependency
+ * mechanism. If additional internal dependencies are introduced in future,
+ * this function may require refinement to avoid unintended deletions.
+ */
+void
+drop_conflict_log_table(Oid subid, char *subname, Oid subconflictlogrelid)
+{
+ ObjectAddress object;
+ char *conflictrelname;
+
+ conflictrelname = get_rel_name(subconflictlogrelid);
+
+ ObjectAddressSet(object, SubscriptionRelationId, subid);
+ performDeletion(&object, DROP_CASCADE,
+ PERFORM_DELETION_INTERNAL |
+ PERFORM_DELETION_SKIP_ORIGINAL);
+
+ ereport(NOTICE,
+ errmsg("dropped conflict log table \"%s\" for subscription \"%s\"",
+ get_qualified_objname(PG_CONFLICT_NAMESPACE, conflictrelname),
+ subname));
+}
+

IIUC, this is a function that drops the subscription dependencies via
cascade. Since the CLT happens to be the only such dependency, it gets
dropped.

The current implementation feels backwards to me. IMO, this is really
a subscription function, so it should be refactored to be called
something like 'drop_subscription_dependencies', and not be in the
conflicts.c file. Refactoring/renaming to what it *really* does means
you won't need to give the other warnings like "may require refinement
to avoid unintended deletions". Maybe the callers do not need to be
guarded anymore -- this code can check internally so that it only does
anything when there is a known CLT associated with the subscription.

Also, the function comment should make it clearer that
PERFORM_DELETION_SKIP_ORIGINAL means the parent subscription object is
not deleted.

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

---

From: Peter Smith <[email-masked]>
Date: Thu, 21 May 2026 13:59:52 +1000

Hi Vignesh.

Thanks for addressing my review comments for the documentation.

Here is one more comment for the v37-0008/0009 (combined) docs patches

======

1.
+      <row>
+       <entry><literal>replica_identity</literal></entry>
+       <entry><type>json</type></entry>
+       <entry>The JSON representation of the replica identity.</entry>
+      </row>
+      <row>

I think patch 0002 modified the CLT column order. This doc's table row
order should match the order of the CLT columns, so please compare
again with the schema defined by the latest conflict.c.

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

---

From: Peter Smith <[email-masked]>
Date: Thu, 21 May 2026 14:57:10 +1000

Hi Vignesh

Some trivial review comments for the combined v37-0003/0004 (transfer
ownership) patches.

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

1.
+ALTER SUBSCRIPTION regress_conflict_test1 owner to regress_subscription_user2;

/owner to/OWNER TO/

~~~

2.
+-- Restore the original subscription owner.
+ALTER SUBSCRIPTION regress_conflict_test1 owner to regress_subscription_user;

/owner to/OWNER TO/

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

---

From: Peter Smith <[email-masked]>
Date: Thu, 21 May 2026 15:51:02 +1000

Hi Vignesh,

Some minor review comments for patches v37-0005/0006 combined.

======

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 }
+};
+
+#define NUM_LOCAL_CONFLICT_ATTRS lengthof(LocalConflictSchema)
+

IMO this belongs *below* the ConflictLogSchema[], which is where
'local_conflicts' attribute was introduced, instead of above it.

~~~

2.
+
+
 static int errcode_apply_conflict(ConflictType type);

~

There are some spurious blank lines here that should not be in the patch.

~~~

ProcessPendingConflictLogTuple:

3.
+ /* Open conflict log table and insert the tuple */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);
+ Assert(CONFLICTS_LOGGED_TO_TABLE(dest));

Maybe here it's better to say Assert(conflictlogrel);

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

---

From: shveta malik <[email-masked]>
Date: Thu, 21 May 2026 11:31:29 +0530

Amit, Vignesh,

A part of 007 patch is about preserving subscription-oid. Another
thread (origin migration) also needs the same logic as per discussion
at [1]. And there was a old thread which already attempted preserving
subscription-oid at [2], but the idea was rejected at that time. Why
don't we attempt to resume the same thread ([2]) and implement
preserving subscription-oid as a separate thread as we now have
multiple dependencies on it? Thoughts?

[1]:  https://www.postgresql.org/message-id/CALDaNm2-uwpbJ8fnrssp%2BhORvOutsqRoZAsa05xVVzXe5Bt3bw%40mail.gmail.com
[2]:  https://www.postgresql.org/message-id/flat/CALDaNm2Wj63VcbB0SY2NECHr1mKM1YSaV1ZydrdQVxyox2O2hg%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 21 May 2026 12:38:28 +0530

A few comments on v36-007:

1)

AlterSubscriptionConflictLogDestination
+ want_table = (logdest == CONFLICT_LOG_DEST_TABLE ||
+   logdest == CONFLICT_LOG_DEST_ALL);
+ has_oldtable = (old_dest == CONFLICT_LOG_DEST_TABLE ||
+ old_dest == CONFLICT_LOG_DEST_ALL);

Shall we replace checks at both places with CONFLICTS_LOGGED_TO_TABLE?

2)
I think we can move 'AlterSubscriptionConflictLogDestination' into the
configuration patch itself (if needed). It is not directly used
anywhere in upgrade flow as such. IIUC, even if upgrade flow uses it,
it will only be used through AlterSubscription.

3)
AlterSubscriptionConflictLogDestination:

+ if (want_table && !has_oldtable)
+ {
+ char relname[NAMEDATALEN];
+
+ snprintf(relname, NAMEDATALEN, "pg_conflict_log_for_subid_%u", sub->oid);
+
+ /*
+ * In upgrade scenarios, the conflict log table already exists. Update
+ * the catalog to record the association.
+ */
+ relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
+ if (!OidIsValid(relid))
+ relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);

So this function will now be used during upgrade where destination is
TABLE/ALL as well as regular Alter-Subscription to change destination
from LOG to TABLE/ALL. In upgrade case, we expect the relid (CLT) to
be present already while in regular case, we don't expect any CLT to
be present.

The above code does not take care of maintaining the sanity checks. It
should be able to distinguish the 2 cases and Assert/Error if the
condition is opposed to what we expect.

4)
Also , I do not understand how can upgrade ever pass this check:

+ if (want_table && !has_oldtable)

It is not obvious how the upgrade flow will pass this check because
theoretically both the old and new setup should have the exact same
configuration; i.e. if  'want_table'  is true, 'has_oldtable' will be
true. We can add a comment to clarify the situation here.

thanks
Shveta

---

From: vignesh C <[email-masked]>
Date: Thu, 21 May 2026 12:41:03 +0530

> LINE 9: FROM pg_catalog.pg_publication

I noticed this behavior with several other commands as well. For example:
postgres=# ALTER SCHEMA pg_catalog RENAME TO test;
ALTER SCHEMA
postgres=# \d
ERROR:  relation "pg_catalog.pg_class" does not exist
LINE 6: FROM pg_catalog.pg_class c
             ^
postgres=# \dn
ERROR:  relation "pg_catalog.pg_namespace" does not exist
LINE 4: FROM pg_catalog.pg_namespace n
             ^

I observed similar behavior when creating a table in the renamed schema:
postgres=# CREATE TABLE test.t1(c1 int);
ERROR:  schema "pg_catalog" does not exist
LINE 1: CREATE TABLE test.t1(c1 int);
                     ^

Given that this appears to be a broader issue related to renaming
pg_catalog, I think we can skip handling this case for now. If we
decide to address it, it would be better to handle it together with
the general pg_catalog rename behavior.

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Thu, 21 May 2026 12:43:15 +0530

Hi Vignesh,

I reviewed v37-0007 patch. Here is some review comments:

1. subinfo[i].subconflictlogdest is assigned multiple times:

+       if (PQgetisnull(res, i, i_sublogdestination))
+           subinfo[i].subconflictlogdest = NULL;
+       else
+           subinfo[i].subconflictlogdest =
+               pg_strdup(PQgetvalue(res, i, i_sublogdestination));
+
+       if (PQgetisnull(res, i, i_sublogdestination))
+           subinfo[i].subconflictlogdest = NULL;
+       else
+           subinfo[i].subconflictlogdest =
+               pg_strdup(PQgetvalue(res, i, i_sublogdestination));

2. I think we should add a version check before:
+   appendPQExpBuffer(query,
+                     "\n\nALTER SUBSCRIPTION %s SET
(conflict_log_destination = %s);\n",
+                     qsubname,
+                     subinfo->subconflictlogdest);

When we run pg_dump on a server with Postgres 18, we get the following output.
ALTER SUBSCRIPTION sub2 SET (conflict_log_destination = (null));

Thanks,
Shlok Kyal

---

From: shveta malik <[email-masked]>
Date: Thu, 21 May 2026 16:09:23 +0530

Few comments on doc patches v36-008 and 009 combined:

1)
+       An array of JSON objects representing the local state for each
conflict attempt.

'each conflict attempt' looks misleading. We do not attempt to cause
conflicts; we attempt to apply, but it may result in conflicts.

Shall we rephrase to:
'An array of JSON objects representing the state of existing local
row(s) that caused the conflict.'

There could be multiple rows as well for multiple_unique_conflicts,
thus the 'row(s)'

2)
+   The <link linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link>
+   parameter automatically creates a dedicated conflict log table.

'conflict_log_destination' parameter does not create the table
automatically unless it is set to table. We shall clarify it.

The conflict_log_destination when set to table or all automatically
creates a dedicated conflict log table.

3)
+   Conflicts that occur during replication are, by default, logged as
plain text

When we say 'Conflicts' here, we shall make it a link to '29.8.
Conflicts' chapter. That way it will be more clear.

thanks
Shveta

---

From: Nisha Moond <[email-masked]>
Date: Fri, 22 May 2026 10:21:21 +0530

> comments from [2] are addressed in the attached patch.

Here are few comments based on v37 testing:

1) Should we consider using TOAST tables for tuple-data columns like
remote_tuple and local_conflicts (the JSON columns)?
This may be a corner case, but if the tuple data becomes too large to
fit into an 8KB heap tuple, then the apply worker keeps failing while
inserting into the CLT with errors like:

  ERROR: row is too big: size 19496, maximum size 8160
  LOG: background worker "logical replication apply worker" (PID
41226) exited with exit code 1

Noticed that even disable_on_error=true does not disable the
subscription in this case. We can think about optimizations such as
deciding when TOAST tables should be created, or avoiding the error by
trimming/capping the data size before inserting into the CLT if don't
want TOAST.
~~~

2) Currently, parallel apply workers do not seem to insert conflicts
into the CLT. The parallel worker logs the conflict to the logfile and
then exits with an error without handling CLT insertion.
A small test to reproduce this with a 't1' table subscription using a CLT table:
-- on publisher
ALTER SYSTEM SET logical_decoding_work_mem = '64kB';
SELECT pg_reload_conf();

-- Create a conflict scenario on subscriber: pre-insert a row that will conflict
INSERT INTO t1 VALUES (99999, 11);

-- on publisher: big transaction that hits the conflict
BEGIN;
INSERT INTO t1 SELECT i, i FROM generate_series(1, 10000) i;
INSERT INTO t1 VALUES (99999, 99); -- this conflicts
COMMIT;

logfile:
ERROR: conflict detected on relation "public.t1": conflict=insert_exists
DETAIL: Could not apply remote change: remote row (99999, 99).
Key already exists in unique index "t1_pkey", modified locally in
transaction 842 at 2026-05-21 21:10:51.497681+05:30: key (a)=(99999),
local row (99999, 42).
...
ERROR: logical replication parallel apply worker exited due to error
CONTEXT: processing remote data for replication origin "pg_16398"
during message type "INSERT" for replication target relation
"public.t1" in transaction 720
logical replication parallel apply worker
processing remote data for replication origin "pg_16398" during
message type "STREAM COMMIT" in transaction 720, finished at
0/01AC9758
LOG: subscription "sub1" has been disabled because of an error
ERROR: lost connection to the logical replication parallel apply worker
LOG: background worker "logical replication parallel worker" (PID
66271) exited with exit code 1
~~~

3) I think somewhere in patch-0005, the remote_tuple and
replica_identity columns may have been swapped.
The replica identity key seems to be written into the remote_tuple
column, while the remote slot row is written into replica_identity,
for example:

postgres=# select relname, conflict_type, remote_xid, remote_tuple,
replica_identity from pg_conflict_log_for_subid_16398;
relname | conflict_type | remote_xid | remote_tuple | replica_identity
---------+-----------------------+------------+--------------+------------------
t1 | insert_exists | 699 | | {"a":3,"b":11}
t1 | update_origin_differs | 700 | {"a":3} | {"a":3,"b":111}
(2 rows)

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Fri, 22 May 2026 15:42:20 +0530

> Here are few comments based on v37 testing:

Here are few more review comments -
1) Patch-0001 + 0002:
In subscription.sql:
 -- Verify the table OID for reap check
 SELECT 'pg_conflict_log_for_subid_' || oid AS internal_tablename FROM
pg_subscription WHERE subname = 'regress_conflict_test1' \gset
 SET client_min_messages = WARNING;
 DROP SUBSCRIPTION regress_conflict_test1;
 -- should return NULL, meaning the conflict log table was reaped via dependency
 SELECT to_regclass(:'internal_tablename');

Here, internal_tablename becomes "pg_conflict_log_*" without the
pg_conflict. schema prefix. So, "SELECT
to_regclass(:'internal_tablename');" will always return NULL even if
the table still exists in the pg_conflict schema, which skips the
actual drop verification scenario.
Should we instead use:
   "SELECT 'pg_conflict.pg_conflict_log_' || oid AS internal_tablename..."
~~~

For Patch-0007:
2)
@@ -2067,9 +2095,31 @@ selectDumpableNamespace(NamespaceInfo *nsinfo,
Archive *fout)
 static void
 selectDumpableTable(TableInfo *tbinfo, Archive *fout)
....
+ if (strcmp(tbinfo->dobj.namespace->dobj.name, "pg_conflict") == 0)
...
+ * Dump pg_conflict tables only during binary upgrade. The schema
+ * is assumed to already exist.
+ */
+ tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION;
....
+ else
+ tbinfo->dobj.dump = DUMP_COMPONENT_NONE;
+ }
+

For conflict log tables during binary upgrade, we set:
   tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION;

but then execution falls through to the later logic:
...
  else
    tbinfo->dobj.dump = tbinfo->dobj.namespace->dobj.dump_contains;

which seems to overwrite the earlier 'dobj.dump' value. So it looks
like DUMP_COMPONENT_DEFINITION may never actually survive here.
Should we return from this block instead of continuing further?

3)
@@ -5656,6 +5757,11 @@ dumpSubscription(Archive *fout, const
SubscriptionInfo *subinfo)

  appendPQExpBufferStr(query, ");\n");

+ appendPQExpBuffer(query,
+   "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n",
+   qsubname,
+   subinfo->subconflictlogdest);
+

The above ALTER SUBSCRIPTION command seems to be dumped
unconditionally for every subscription.
Since the default value during subscription creation is already
"subconflictlogdest = 'log' ", should we emit this command only when
subconflictlogdest is non-NULL and not 'log'?

4)
  /* Decide whether we want to dump it */

Looks like the same if-else block is repeated twice here.

--
Thanks,
Nisha

---

From: vignesh C <[email-masked]>
Date: Sat, 23 May 2026 11:40:40 +0530

> once and reuse it everytime. We can have a 'static' TupleDesc here.

Thanks for the comments, these comments are addressed in the v38
version attached.
Apart from this, the comments from [1], [2], [3], [4], [5], [6], [7],
and [8] are also addressed.

[1] - https://www.postgresql.org/message-id/CAJpy0uC43NTKheuLo%2BMsHG7Sfh-QWQM9QP-EVPL5LChiPfisJw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CANhcyEU8qr9%2BPMU2Kn0qqZakVptVvRsbRu3Ee2Q40YX9aivXww%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CAJpy0uB19XxfF2Yj1w%3DC90iVBLMHb%3DDMBZ1h3rqzJhEbTSwtag%40mail.gmail.com
[4] - https://www.postgresql.org/message-id/CAHut%2BPvSaJAYwNUS9GnO6MCTfuPpVLdU1r8cZBf6gjGjvnbWpQ%40mail.gmail.com
[5] - https://www.postgresql.org/message-id/CAHut%2BPtUWTnUD8QpfmNpU8iU6Pg%2BE29nDALYAfMUudad8oYezw%40mail.gmail.com
[6] - https://www.postgresql.org/message-id/CAHut%2BPvW%3DFd-OSM6oe-9D3ycAG0qLfGEnaT%3DBUB%2BPMeUFeEAyQ%40mail.gmail.com
[7] - https://www.postgresql.org/message-id/CAHut%2BPu4ErbjstY86kWbKOepHn623Zp9MNiKW4DoMG3iVdG2fA%40mail.gmail.com
[8] - https://www.postgresql.org/message-id/CANhcyEUGoaSpJKDJaQfrQR6%2B-4%2B_PgQ%3D0DmZZztPAEheMkMw7w%40mail.gmail.com

Regards,
Vignesh

---

From: Amit Kapila <[email-masked]>
Date: Sat, 23 May 2026 08:40:08 -0700

> multiple dependencies on it? Thoughts?

Agreed, but I think we can move the discussion/review to a separate
thread. However, at this stage, we can make initial patches ready and
then move to it.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 25 May 2026 11:36:42 +1000

Hi Vignesh,

Some review comments for v38-0001/0002 combined.

======

1.
 * Update the conflict log table associated with a subscription when its
 * conflict log destination is changed.

Somehow, that 'its' sounded awkward to me.

SUGGESTION.
When the subscription's 'conflict_log_destination' is changed, update
the conflict log table if required.

~~~

2.
+ * If the new destination requires a conflict log table and none was previously
+ * required, this function validates an existing conflict log table identified
+ * by the subscription specific naming convention or creates a new one.

What does this mean: "validates an existing conflict log table". How
is there an "existing" CLT when you already said "none was previously
required". Maybe this needs more explanation. If it is talking about
"not already associated with another subscription", then it should
just say that.

Anyway, it seems validation that the comment claims this function is
doing is not done here at all, but is really done by
'create_conflict_log_table'.

~~~

3.
+static bool
+AlterSubscriptionConflictLogDestination(Subscription *sub,
+ ConflictLogDest logdest,
+ Oid *conflicttablerelid)

3a.
There was no forward declaration of this static function, but there
was for all the others.

~

3b.
Static functions should use snake-case names.

~~~

4.
Personally, I think it is more natural to read LEFT-TO-RIGHT,
OLD-THEN-NEW, etc., so I felt that the has_oldtable check should
always come before want_table.

Also, the 'ifs' seemed tricky because it's not obvious what
has/need_table combinations are missing. e.g. The following seems
easier to me. And probably lots of comments could be moved to here in
the code as well, instead of in the function comment.

SUGGESTION
if (has_old_table)
{
  /* There is a CLT already. */

  if (!want_table)
  {
    /* Remove it. */
    drop_subscription_dependencies(sub->oid, sub->name, sub->conflictlogrelid);
    update_relid = true;
  }
}
else
{
  /* There was no previous CLT. */

  if (want_table)
  {
    /* Create one. */
    relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);
    update_relid = true;
  }
}

~~~

5.
+static void
+drop_subscription_dependencies(Oid subid, char *subname,
+    Oid subconflictlogrelid)
+{
+ ObjectAddress object;
+ char *conflictrelname;
+
+ conflictrelname = get_rel_name(subconflictlogrelid);
+
+ /*
+ * By using PERFORM_DELETION_SKIP_ORIGINAL, we ensure that only the
+ * conflict log table is deleted while the subscription remains.
+ */
+ ObjectAddressSet(object, SubscriptionRelationId, subid);
+ performDeletion(&object, DROP_CASCADE,
+ PERFORM_DELETION_INTERNAL |
+ PERFORM_DELETION_SKIP_ORIGINAL);
+
+ ereport(NOTICE,
+ errmsg("dropped conflict log table \"%s\" for subscription \"%s\"",
+ get_qualified_objname(PG_CONFLICT_NAMESPACE, conflictrelname),
+ subname));
+}
+

One day, this function might do more than just remove the CLT, so IMO
all this function body should be within a check:

if (OidIsValid(subconflictlogrelid))
{
  /* Drop any dependent CLT */
  ...
}

~~~

DropSubscription

6.
+ if (OidIsValid(subconflictlogrelid))
+ drop_subscription_dependencies(subid, subname, subconflictlogrelid);

Make it unconditional. Instead, add the condition inside the
'drop_subscription_dependencies', per the previous review comment #5.

======

7.
+--
+-- PUBLICATION: Verify conflict log tables are not publishable
+--
+-- pg_relation_is_publishable should return false for internal conflict log
+-- tables to prevent them from being accidentally included in publications
+--

Everywhere else, you had removed the word "internal", but this one
(maybe others?) was missed.

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

---

From: shveta malik <[email-masked]>
Date: Mon, 25 May 2026 09:48:43 +0530

> thread. However, at this stage, we can make initial patches ready and

Okay, works for me.

thanks
Shveta

---

From: vignesh C <[email-masked]>
Date: Mon, 25 May 2026 10:12:53 +0530

> Looks like the same if-else block is repeated twice here.

Thanks for the comments, the attached v39 version patch has the
changes for the same.

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Tue, 26 May 2026 11:23:51 +0530

> (maybe others?) was missed.

Thanks for the comments, these are addressed in the v40 version patch attached.

Regards,
Vignesh

---

From: Peter Smith <[email-masked]>
Date: Tue, 26 May 2026 16:28:43 +1000

Hi Vignesh.

I had only one trivial review comment for v40-0001/0002 combined.

======

1.
+ if (OidIsValid(subconflictlogrelid))
+ {
+ ObjectAddress object;
+ char *conflictrelname;
+
+ /* Drop any dependent conflict log table */
+ conflictrelname = get_rel_name(subconflictlogrelid);

That "Drop any..." comment doesn't have anything to do with the
statement that follows it. I think this comment belongs outside the
if.

e.g.
/* Drop any dependent conflict log table */
if (OidIsValid(subconflictlogrelid))
{
  ...
}

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

---

From: shveta malik <[email-masked]>
Date: Tue, 26 May 2026 15:08:19 +0530

> Thanks for the comments, the attached v39 version patch has the

I have not yet looked at v40, but please find a few ocmments on
v39-0001 and 0002 merged together.

1)
heap_create:
+ errdetail("Conflict schema modifications are currently disallowed.")));
LookupCreationNamespace:
+ errmsg("cannot move objects into or out of the pg_conflict schema")));

Can we make it same through-out, either we use 'Conflict schema' at
both the places or pg_conflict schema.  Since in these 2 functions, in
previous messages, we are using names like 'System catalog', 'TOAST
schema' etc, I think we can use Conflict schema at both the places.
What do others think on this?

2)
drop_subscription_dependencies():

We can actually have a sanity check that we got the CLT using the relid.
Assert(conflictrelname != NULL);

3)
+ /*
+ * Special handling for the JSON array type for proper
+ * TupleDescInitEntry call.
+ */
+ if (type_oid == JSONARRAYOID)
+ type_oid = get_array_type(JSONOID);

Why do we have this special handling? Do we expect that 'type_oid' can
be different from JSONARRAYOID if we use get_array_type? On debugging,
I found it to be same pre and post get_array_type()

4)
Do we need to have CommandCounterIncrement() after
heap_create_with_catalog() in create_conflict_log_table()? I think
even if we are not doing any table_open etc for CLT in same
transaction, we should call CommandCounterIncrement() (to be
consistent with other such calls of heap_create_with_catalog and to
make it future proof). Thoughts?

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Wed, 27 May 2026 08:50:23 +1000

On Tue, May 26, 2026 at 7:38 PM shveta malik <[email-masked]> wrote:
...
> What do others think on this?

The suggested name of "Conflict schema" LGTM. My only concern was that
a user may not know where that is referring to. OTOH, things like
"System catalog" have 100s of mentions and whole documentation
chapters dedicated to them. If we go with "Conflict schema", then the
documentation needs to also consistently use that term, describe what
it is for, and make it very easy to look up and discover that
"Conflict schema" is 'pg_conflict'.

Currently (in patches 0008/9) there is very little explanation even
about what pg_conflict is, apart from just observing in passing that
the CLT gets written to that "dedicated namespace". It seems a bit
backwards describing the parent schema by the contents: Instead of
saying when there is a CLT it gets written there, IMO it should be the
other way around, and say there is a "Conflict schema" which is where
the CLTs (if any) reside.

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

---

From: shveta malik <[email-masked]>
Date: Wed, 27 May 2026 09:39:01 +0530

> "Conflict schema" is 'pg_conflict'.

I agree that if we use 'Conflict schema' in the error messages, we
need to refer it the same way in doc. Let's wait for others' opinions
on this too.

> the CLTs (if any) reside.

Yes, the suggestion makes sense. I will look at the doc patch again for this.

thanks
Shveta

---

From: vignesh C <[email-masked]>
Date: Wed, 27 May 2026 14:04:37 +0530

> make it future proof). Thoughts?

I felt this is not required as we are not doing a table open on the
newly created table.

I have fixed the rest of the comments. The attached v41 version patch
has the changes for the same.  Additionally the comments from [1] have
also been fixed.

[1] - https://www.postgresql.org/message-id/CAHut%2BPvB3rUs2ccUxJ1q1YEmvtHN3HJGSEjT4Cbc%3D5pjoGO9Yg%40mail.gmail.com

Regards,
Vignesh

---

From: shveta malik <[email-masked]>
Date: Wed, 27 May 2026 16:08:40 +0530

I have not yet looked at v41. Here are the comments for v40

0003 and 0004: No comments.

0004 and 0005:

1)
In build_local_conflicts_json_array(), we have these:

+ json_datum = heap_copy_tuple_as_datum(tuple, tupdesc);
+
+ /*
+ * Build the higher level JSON datum in format described in function
+ * header.
+ */
+ json_datum = DirectFunctionCall1(row_to_json, json_datum);

We have first allocation to 'json_datum' via
heap_copy_tuple_as_datum() and then second via row_to_json() call. So
we are overwriting first allocation. Which memory context are we using
here for this allocation? IIUC, if the conflict is non-error one, we
may accumulate these memory chunks in long running worker loop which
may gradually bloat the memory. Let me know if my undertstanding is
wrong.

Same situation in tuple_table_slot_to_indextup_json and
tuple_table_slot_to_json_datum as well.

2)
Same in ReportApplyConflict(), if elevel is not ERROR, should we worry
about freeing 'err_detail' after error-reporting or does some
short-lived context handle it?

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 14:26:47 -0700

> I felt this is not required as we are not doing a table open on the

Okay, command counter increment would be required here if we further
access that relation in the same command.  I think I am facing a
related problem w.r.t newly created subscription. After applying first
six patches, the create subscription fails as follows:
postgres=# create subscription sub1 connection 'dbname=postgres'
publication pub1 with (conflict_log_destination='all');
ERROR:  dependent subscription was concurrently dropped

I debugged and found that we get the above ERROR when we are trying to
find the subscription which is not yet created. In this case, it seems
to be happening because we are using a subscription that is yet not
created for dependency recording. This raises a question as to why are
we creating the conflict_log_table before subscription, at least this
needs some comments.

*
+ if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
+ {
+ if (IsConflictLogTableClass(classForm))
+ {
+ /*
+ * For conflict log tables, allow non-superusers to perform
+ * DELETE and TRUNCATE for cleanup and maintenance. Also allow
+ * INSERT and UPDATE to pass ACL checks so that later checks
+ * can raise the dedicated "cannot modify or insert data into
+ * conflict log table" error instead of a generic permission
+ * denied error. Still restrict USAGE for non-superusers.
+ */
+ mask &= ~(ACL_USAGE);

I see the point of giving a specific error instead of a generic error
but this functionality is used by pg_class_aclmask() which is an
exposed function. If we go with your proposed change, isn't there a
risk that some extension or outside core-code using pg_class_aclmask()
won't invoke that later functionality (CheckValidResultRel())? If we
decide to go this way then we can change this comment as proposed in
the attached?

-- 
With Regards,
Amit Kapila.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index e583187c7a6..19c568f4705 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3347,9 +3347,9 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
 				 * For conflict log tables, allow non-superusers to perform
 				 * DELETE and TRUNCATE for cleanup and maintenance. Also allow
 				 * INSERT and UPDATE to pass ACL checks so that later checks
-				 * can raise the dedicated "cannot modify or insert data into
-				 * conflict log table" error instead of a generic permission
-				 * denied error. Still restrict USAGE for non-superusers.
+				 * in CheckValidResultRel() can raise the specific error instead
+				 * of a generic permission denied error. Still restrict USAGE for
+				 * non-superusers.
 				 */
 				mask &= ~(ACL_USAGE);
 			}

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 15:09:45 -0700

> short-lived context handle it?

Isn't this the case even without this patch? If so, this can be
investigated separately.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 27 May 2026 16:08:11 -0700

> Assert(conflictrelname != NULL);

elog will suit this place better as this can't be a direct coding
mistake. I see that at other places we used elog. See
if (result == NULL)
elog(ERROR, "cache lookup failed for index %u", indexId);

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Thu, 28 May 2026 11:20:03 +1000

Hi Vignesh.

Here are some review comments for the v41-0008/9 combined (docs) patch.

======
doc/src/sgml/ddl.sgml

(5.11.6. The Conflict Schema)

1.
+   <para>
+    Similarly, the <literal>pg_conflict</literal> schema (sometimes referred to
+    as the <emphasis>conflict schema</emphasis>) contains system managed
+    conflict log tables used for logical replication conflict tracking. These
+    tables are created and maintained by the system and are not intended for
+    direct user manipulation. Unlike <literal>pg_catalog</literal>, the
+    <literal>pg_catalog</literal> schema is not implicitly included in the
+    search path, so objects within it must be referenced explicitly or by
+    adjusting the search path.
+   </para>

1a.
/Similarly, the/The/

~

1b.
IMO don't say "sometimes".

Also, case. /conflict schema/Conflict schema/

~

1c.
"conflict log tables" -- I think it will be helpful if this includes a
link to "29.8.2. Table-based logging #".

~

1d.
"Unlike <literal>pg_catalog</literal>, the
<literal>pg_catalog</literal> schema..."

typo. That 2nd pg_catalog should say pg_conflict.

======
doc/src/sgml/glossary.sgml

2.
+  <glossentry id="glossary-conflict-schema">
+   <glossterm>conflict schema</glossterm>
+   <glossdef>
+    <para>
+     The <literal>pg_conflict</literal> schema that contains system-managed
+     conflict log tables for logical replication. These tables are created
+     and maintained automatically by the system and are not intended for
+     direct user manipulation. See <xref linkend="ddl-schemas-conflict"/>.
+    </para>
+   </glossdef>
+  </glossentry>
+

case. /conflict schema/Conflict schema/

======

(29.2. Subscription)

3.
+   automatically manages a dedicated <firstterm>conflict log table</firstterm>,
+   which is created an dropped along with the subscription. This significantly
+   improves post-mortem analysis and operational visibility of the replication
+   setup.

typo.  /created an dropped/created and dropped/

~~~

(29.8.2. Table-based logging)

4.
+    a dedicated conflict log table will be automatically created. This table is
+    created in the <literal>pg_conflict</literal> namespace. The name of the

Instead of "<literal>pg_conflict</literal> namespace", this should now
say "Conflict schema" and have a link to that new docs section.

======

(Parameters - conflict_log_destination)

5.
+             named <literal>pg_conflict_log_for_subid_&lt;subid&gt;</literal>
+             in the <literal>pg_conflict</literal> schema. This allows for easy

Same as review comment #4. Instead of "<literal>pg_conflict</literal>
schema", this should now say "Conflict schema" and have a link to that
new docs section.

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

---

From: vignesh C <[email-masked]>
Date: Thu, 28 May 2026 20:19:14 +0530

> has the changes for the same.  Additionally the comments from [1] have

I was evaluating whether the existing pg_upgrade changes for conflict
log tables can handle the addition of new columns in a future release.
To validate this, I performed the following:
Added two new columns to the conflict log table:
v20_new_col1 TEXT
v20_new_col2 TEXT

These changes are present in patch '0001'.

For adding new columns during binary upgrade, the following
version-specific logic is required in 'pg_dump':
ALTER TABLE pg_conflict.pg_conflict_log_for_subid_oid
ADD COLUMN v20_new_col1 TEXT;

ALTER TABLE pg_conflict.pg_conflict_log_for_subid_oid
ADD COLUMN v20_new_col2 TEXT;

These changes are included in patch '0001'.
One important point here is that when 'ALTER TABLE ... ADD COLUMN' is
run, the server does not rewrite existing rows on disk. Instead, it
only updates the system catalog with the new column metadata.

While selecting data from the table, the server handles this as follows:
1. Deform what is physically present - 'slot_deform_heap_tuple()'
reads the raw tuple bytes from disk, but only up to 't_natts', which
is the number of columns recorded in the tuple header at the time that
row was inserted. It stops there because the tuple has no physical
data for columns added later.
2. Fill in what is missing -   After deforming the tuple, if the
number of populated columns is still less than the number of columns
requested by the query, it calls 'slot_getmissingattrs()' to cover the
gap.   Since the new columns were added with no default value,
'slot_getmissingattrs()' sets:
tts_isnull[attnum] = true;

This is how NULL is returned for the newly added columns in existing rows.

These changes were tested on a new server with the v40 version patch +
'0001' patch.
1. Pre-upgrade state using v40 version patches
Simulated conflicts using a setup where the schema does not include
the new columns:
postgres=# select * from pg_conflict.pg_conflict_log_for_subid_16396 ;
....
(4 rows)

2. Upgrade using 'pg_upgrade'
The upgrade was performed on a cluster initialized with patches v40 +
'0001', and it completed successfully.
Post-upgrade verification:
postgres=# select conflict_type, v20_new_col1, v20_new_col2 from
pg_conflict.pg_conflict_log_for_subid_16396 ;
 conflict_type | v20_new_col1 | v20_new_col2
---------------+--------------+--------------
 insert_exists |              |
 insert_exists |              |
 insert_exists |              |
 insert_exists |              |
(4 rows)

Existing rows were preserved, and the newly added columns are visible
and populated with NULLs, as expected.

3. Post-upgrade conflict insertion
After starting the old publisher again to continue generating conflicts:
postgres=# select conflict_type, v20_new_col1, v20_new_col2 from
pg_conflict.pg_conflict_log_for_subid_16396 ;
 conflict_type | v20_new_col1 | v20_new_col2
---------------+--------------+--------------
 insert_exists |              |
 insert_exists |              |
 insert_exists |              |
 insert_exists |              |
 insert_exists | v20_new_col1 | v20_new_col2
 insert_exists | v20_new_col1 | v20_new_col2
 insert_exists | v20_new_col1 | v20_new_col2
(7 rows)

New conflicts are inserted successfully, and the newly added columns
are correctly populated for new entries.

Based on this testing, the current 'pg_upgrade' framework, along with
the additional dump-time adjustments, appears sufficient to support
schema evolution of conflict log tables, specifically for adding new
columns in future releases.

Thoughts?

Regards,
Vignesh

---

From: Amit Kapila <[email-masked]>
Date: Thu, 28 May 2026 16:41:34 -0700

> 41226) exited with exit code 1

In the docs, it is mentioned: "column_value is the column value. The
large column values are truncated to 64 bytes." [1], so I wonder, if
we follow this why we need toast entries? Did you tried any case where
you are getting above ERROR?

> subscription in this case.
>

Hmm, I think we need to have a documented reason if such a case
doesn't disable the subscription with the disable_on_error as true?

[1]: https://www.postgresql.org/docs/devel/logical-replication-conflicts.html

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 29 May 2026 14:52:58 +0530

On Thu, May 28, 2026 at 2:57 AM Amit Kapila <[email-masked]> wrote:
> access that relation in the same command.

I think CommandCounterIncrement() is called wherever we need to open
the relation in the same command.  In this particular case we do not
need to open the conflict log table so we do not need to call CCI

  I think I am facing a
> we creating the conflict_log_table before subscription, at least this

This error occurs because in the commit below [1], we disallowed
recording a dependency on an object that does not exist. Therefore, we
now need to record the dependency after the subscription is created.
And we create CLT before so that we can add the conflict log relid in
pg_subscription without an additional update, I will add a comment
explaining this.

[1]
commit 2fbb21170e9053720c2c374b21eb650a22b8aaea
Author: Heikki Linnakangas <[email-masked]>
Date:   Wed May 27 18:35:58 2026 +0300
    Avoid orphaned objects dependencies

-- 
Regards,
Dilip Kumar
Google

---

From: Amit Kapila <[email-masked]>
Date: Fri, 29 May 2026 11:47:49 -0700

> now need to record the dependency after the subscription is created.

But don't we normally create dependency immediately after creating the
object? Do you see such examples at other places in the code?

> pg_subscription without an additional update,
>

But will this additional update matter to an extent in DDL execution
that we don't follow our usual way to record dependency? I feel unless
we follow similar coding pattern at other places, it is better to
create the CLT after subscription.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 30 May 2026 03:24:39 +0530

> you are getting above ERROR?

But in this case we are talking about the JSON column of the CLT which
might contain a full local tuple or even multiple local tuples if a
remote tuple conflicts with multiple local rows.  So, IMHO, we need a
toast table. Nisha, have you already tested the scenario? If yes, can
you share your test case?

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 30 May 2026 03:36:59 +0530

> you share your test case?

After putting more thought, I think instead of executing a three-step
process i.e. inserting the pg_subscription tuple, creating the table
with its dependency, and then going back to update the tuple with the
new relation ID, it is much cleaner to do it linearly, i.e. we should
create the conflict log table first to get its OID, insert the
subscription tuple pre-populated with that ID, and then record the
dependency. This achieves the exact same state in a single direct
sequence without the redundant catalog update within the same command.
I agree with that code we would have to keep the record dependency
code in CreateSubscription and AlterSubscription functions, but after
putting more thought I think in thoese function we are already
recording subscription dependencies with other object so wouldn't it
be more natural to add this depednecy as well at the same place?

Anyway I am ready to change that if we have strong opinion against
this approach.

Here is the updated patch and changes are
1. 0003 and 0004 are merged on 0001
2. Merged Amit's v41_amit_1.patch.txt to 0002
3. Fix the dependency order issue (i.e. create dependency after
inserting subscription tuple) and merged in 0002

Open Items:
1. Need to create toast table for CLT after testing with larger JSON row
2. Fixed review comments of Shveta on 0004 and 0005
3.  Rebase Vignesh's patch of
"v41-0007-Preserve-conflict-log-destination-and-subscripti" I think we
can do that once we have concensus on whether to create conflict log
table first or insert the subscription row first as based on this
change we would have to rebase this patch again.
4. Once we rebase
"v41-0007-Preserve-conflict-log-destination-and-subscripti" after
dependency order consensus I would rebase doc patch and \dRs+ change
patch of Vignesh.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 30 May 2026 06:01:05 +0530

> dependency order consensus I would rebase doc patch and \dRs+ change

Here is a topup patch so create conflict log table after inserting
subscription tuple and then update the tuple with clt relid..

Main changes will look like this[1]

[1]
/*
* If logging to a table is required, physically create it now. We create
* the conflict log table here. Also update the pg_subscription row
* after creating the conflict log table with its reloid.
*/
if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
{
bool replaces[Natts_pg_subscription];
Oid logrelid =
create_conflict_log_table(subid, stmt->subname, owner);

/* Form a new tuple. */
memset(values, 0, sizeof(values));
memset(nulls, false, sizeof(nulls));
memset(replaces, false, sizeof(replaces));

values[Anum_pg_subscription_subconflictlogrelid - 1] =
ObjectIdGetDatum(logrelid);
replaces[Anum_pg_subscription_subconflictlogrelid - 1] =
true;

/* Make subscription tuple visible before updating it. */
CommandCounterIncrement();

tup = heap_modify_tuple(tup, RelationGetDescr(rel), values, nulls,
replaces);

CatalogTupleUpdate(rel, &tup->t_self, tup);
}

-- 
Regards,
Dilip Kumar
Google
From a8e1329bb58436b44d4f0daadc5240daebd39bae Mon Sep 17 00:00:00 2001
From: Dilip Kumar <[email-masked]>
Date: Sat, 30 May 2026 05:33:21 +0530
Subject: [PATCH v42] Create conflict log table after inserting subscription
 row

---
 src/backend/commands/subscriptioncmds.c    | 73 ++++++++++------------
 src/backend/replication/logical/conflict.c | 17 +++++
 2 files changed, 49 insertions(+), 41 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 88f22bbb286..67311a65e29 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -651,7 +651,6 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 	uint32		supported_opts;
 	SubOpts		opts = {0};
 	AclResult	aclresult;
-	Oid			logrelid = InvalidOid;
 
 	/*
 	 * Parse and check options.
@@ -844,22 +843,45 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 	values[Anum_pg_subscription_subconflictlogdest - 1] =
 		CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
 
+	values[Anum_pg_subscription_subconflictlogrelid - 1] =
+									ObjectIdGetDatum(InvalidOid);
+
+	tup = heap_form_tuple(RelationGetDescr(rel), values, nulls);
+
+	/* Insert tuple into catalog. */
+	CatalogTupleInsert(rel, tup);
+	//heap_freetuple(tup);
+
 	/*
 	 * If logging to a table is required, physically create it now. We create
-	 * the conflict log table here so its relation OID can be stored when
-	 * inserting the pg_subscription tuple below.
+	 * the conflict log table here.  Also update the pg_subscription row
+	 * after creating the conflict log table with its reloid.
 	 */
 	if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
-		logrelid = create_conflict_log_table(subid, stmt->subname, owner);
+	{
+		bool		replaces[Natts_pg_subscription];
+		Oid			logrelid =
+				create_conflict_log_table(subid, stmt->subname, owner);
 
-	/* Store table OID in the catalog. */
-	values[Anum_pg_subscription_subconflictlogrelid - 1] =
-						ObjectIdGetDatum(logrelid);
+		/* Form a new tuple. */
+		memset(values, 0, sizeof(values));
+		memset(nulls, false, sizeof(nulls));
+		memset(replaces, false, sizeof(replaces));
 
-	tup = heap_form_tuple(RelationGetDescr(rel), values, nulls);
+		values[Anum_pg_subscription_subconflictlogrelid - 1] =
+									ObjectIdGetDatum(logrelid);
+		replaces[Anum_pg_subscription_subconflictlogrelid - 1] =
+									true;
+
+		/* Make subscription tuple visible before updating it. */
+		CommandCounterIncrement();
+
+		tup = heap_modify_tuple(tup, RelationGetDescr(rel), values, nulls,
+								replaces);
+
+		CatalogTupleUpdate(rel, &tup->t_self, tup);
+	}
 
-	/* Insert tuple into catalog. */
-	CatalogTupleInsert(rel, tup);
 	heap_freetuple(tup);
 
 	recordDependencyOnOwner(SubscriptionRelationId, subid, owner);
@@ -876,25 +898,6 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 		recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
 	}
 
-	/*
-	 * If conflicts are logs to table establish an internal dependency
-	 * between the conflict log table and the subscription.
-	 *
-	 * We use DEPENDENCY_INTERNAL to signify that the table's lifecycle is
-	 * strictly tied to the subscription, similar to how a TOAST table relates
-	 * to its main table or a sequence relates to an identity column.
-	 *
-	 * This ensures the conflict log table is automatically reaped during a
-	 * DROP SUBSCRIPTION via performDeletion().
-	 */
-	if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
-	{
-		ObjectAddress clt;
-
-		ObjectAddressSet(clt, RelationRelationId, logrelid);
-		recordDependencyOn(&clt, &myself, DEPENDENCY_INTERNAL);
-	}
-
 	/*
 	 * A replication origin is currently created for all subscriptions,
 	 * including those that only contain sequences or are otherwise empty.
@@ -1507,20 +1510,8 @@ alter_sub_conflictlogdestination(Subscription *sub, ConflictLogDest logdest,
 		/* There was no previous conflict log table. */
 		if (want_table)
 		{
-			ObjectAddress clt;
-			ObjectAddress subobj;
-
 			relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);
 			update_relid = true;
-
-			/*
-			 * Establish an internal dependency between the conflict log table
-			 * and the subscription.  For details refer comments in
-			 * CreateSubscription function.
-			 */
-			ObjectAddressSet(clt, RelationRelationId, relid);
-			ObjectAddressSet(subobj, SubscriptionRelationId, sub->oid);
-			recordDependencyOn(&clt, &subobj, DEPENDENCY_INTERNAL);
 		}
 	}
 
diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c
index 15b0ef7f3ca..0f077b28678 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -175,6 +175,8 @@ create_conflict_log_table(Oid subid, char *subname, Oid subowner)
 {
 	TupleDesc	tupdesc;
 	Oid			relid;
+	ObjectAddress	myself;
+	ObjectAddress	subaddr;
 	char    	relname[NAMEDATALEN];
 
 	snprintf(relname, NAMEDATALEN, "pg_conflict_log_for_subid_%u", subid);
@@ -218,6 +220,21 @@ create_conflict_log_table(Oid subid, char *subname, Oid subowner)
 									 NULL); /* typaddress */
 	Assert(OidIsValid(relid));
 
+	/*
+	 * Establish an internal dependency between the conflict log table and
+	 * the subscription.
+	 *
+	 * We use DEPENDENCY_INTERNAL to signify that the table's lifecycle is
+	 * strictly tied to the subscription, similar to how a TOAST table relates
+	 * to its main table or a sequence relates to an identity column.
+	 *
+	 * This ensures the conflict log table is automatically reaped during a
+	 * DROP SUBSCRIPTION via performDeletion().
+	 */
+	ObjectAddressSet(myself, RelationRelationId, relid);
+	ObjectAddressSet(subaddr, SubscriptionRelationId, subid);
+	recordDependencyOn(&myself, &subaddr, DEPENDENCY_INTERNAL);
+
 	/* Release tuple descriptor memory. */
 	FreeTupleDesc(tupdesc);
 
-- 
2.49.0

---

From: Nisha Moond <[email-masked]>
Date: Sat, 30 May 2026 08:19:15 +0530

> you share your test case?

Hi Dilip, Amit,
Yes, I tested the scenario. Used below steps to reproduce the error:

#Publisher:
  CREATE TABLE fat2 (id int PRIMARY KEY, col1 text, col2 text);
  INSERT INTO fat2 VALUES (
      1,
      (SELECT string_agg(md5(i::text), '') FROM generate_series(1, 200) i),
      (SELECT string_agg(md5(i::text), '') FROM generate_series(201, 400) i)
  );
  ALTER TABLE fat2 REPLICA IDENTITY FULL;
  CREATE PUBLICATION p3 FOR TABLE fat2;

#Subscriber
 -- create subscription s3 for publication p3 with conflict log table
(after table syncs):
 -- modifying the row locally
  UPDATE fat2 SET col1 = (SELECT string_agg(md5(i::text), '') FROM
generate_series(601, 800) i) WHERE id = 1;

 #Publisher (triggers the conflict):
  UPDATE fat2 SET col1 = (SELECT string_agg(md5(i::text), '') FROM
generate_series(801, 1000) i) WHERE id = 1;

Above should cause the reported failure.

--
Thanks,
Nisha

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 30 May 2026 13:42:27 +0530

> CatalogTupleUpdate(rel, &tup->t_self, tup);

In latest patch set I have fixed Nisha's comments by creating a toast
table, a separate patch
(v43-0005-Create-conflict-log-table-after-inserting-subscr.patch)
attached for creating conflict log table after inserting subscription
row.

-- 
Regards,
Dilip Kumar
Google

---

From: Amit Kapila <[email-masked]>
Date: Sat, 30 May 2026 11:21:10 -0700

> be more natural to add this depednecy as well at the same place?

It makes sense to me and anyway for serverid also we are creating
dependency after creation of subscription, so your solution looks good
to me. One minor suggestion related to this changes:

+ if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
+ {
+ ObjectAddress clt;
+
+ ObjectAddressSet(clt, RelationRelationId, logrelid);
+ recordDependencyOn(&clt, &myself, DEPENDENCY_INTERNAL);
+ }

Let's name clt as cltaddr or cltobj to make it consistent with naming
at some other similar places in code. Change this at both places where
we use this code.

> dependency order consensus I would rebase doc patch and \dRs+ change

I see that my second comment in email [1] and another comment in email
[2] are still not answered and are neither listed in open items.

[1] - https://www.postgresql.org/message-id/CAA4eK1%2BzdaLF7%3DAVKd8xNGTuvPvn8BYSxHfnLZd7whWZ%2Bv3B-Q%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAA4eK1K6tVUmKY-yqKgTX00yrSVAdSZN4Ao761JEXdtQkAYT4g%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Sun, 31 May 2026 11:32:51 +0530

> elog(ERROR, "cache lookup failed for index %u", indexId);

Yes it make sense to report elog, I will change this.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Sun, 31 May 2026 11:42:54 +0530

> decide to go this way then we can change this comment as proposed in

I do not understand this change; my original patch 0001 has like this,
that mean we are only allowing ACL_TRUNCATE and ACL_DELETE for
conflict log table, whats the reason for changing the same in 0002?

  if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
ACL_USAGE)) &&
- IsSystemClass(table_oid, classForm) &&
- classForm->relkind != RELKIND_VIEW &&
+ IsConflictClass(classForm) &&
  !superuser_arg(roleid))
- mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);
+ mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_USAGE);
+ else if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE |
ACL_TRUNCATE | ACL_USAGE)) &&
+ IsSystemClass(table_oid, classForm) &&
+ classForm->relkind != RELKIND_VIEW &&
+ !superuser_arg(roleid))
+ mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Sun, 31 May 2026 12:27:48 +0530

> tuple_table_slot_to_json_datum as well.

IIUC logical these all memory will be allocated under
ApplyMessageContext which is temporary and getting reset on every
logical message, so I think that contex is really for the purpose of
temporary allocation during each message processing and get reset
after the message is processed.

> short-lived context handle it?

Same is true for this as well.

-- 
Regards,
Dilip Kumar
Google

---

From: Amit Kapila <[email-masked]>
Date: Sun, 31 May 2026 04:54:08 -0700

On Sat, May 30, 2026 at 1:12 AM Dilip Kumar <[email-masked]> wrote:

Few comments on 0001 and 0002
===========================
1.
+ Oid         subconflictlogrelid; /* Relid of the conflict log table. */
 #ifdef CATALOG_VARLEN /* variable-length fields start here */
+ /*
+ * Strategy for logging replication conflicts:
+ * 'log' - server log only,
+ * 'table' - conflict log table only,
+ * 'all' - both log and table.
+ */
+ text subconflictlogdest BKI_FORCE_NOT_NULL;

'log' sounds redundant in the above two field names. I feel naming
them as subconflictrelid and subconflictdest should be sufficient.

2. If you agree with the above, then let's make similar changes at
other places in the patch. We can change
alter_sub_conflictlogdestination to alter_sub_conflict_destination.
Also, similar to AlterSubscription_refresh and
AlterSubscription_refresh_seq, we can name this new function as
AlterSubscription_conflict_dest.

3. Now, let's consider whether we should change the option name to
conflict_data_destination instead of conflict_log_destination? The
reason I am asking to consider this change is that one of the option
values is 'log', so it sounded a bit odd to name the option as
conflict_log_destination. If we change this then we can consider
changing the name of Enum ConflictLogDest as well.

Apart from above, I have made some changes in the attached. Kindly
review and see which all can be incorporated in the next version.

-- 
With Regards,
Amit Kapila.
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 46d27ed02a9..b7fcaaf6796 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -234,8 +234,7 @@ IsToastClass(Form_pg_class reltuple)
 
 /*
  * IsConflictLogTableClass
- *		True iff Form_pg_class tuple represents a subscription-specific
- *      Conflict Log Table.
+ *		True iff pg_class tuple represents a Conflict Log Table.
  *
  *		Does not perform any catalog accesses.
  */
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 3b6a163c184..96def3019b3 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -327,7 +327,7 @@ heap_create(const char *relname,
 			ereport(ERROR,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("permission denied to create \"%s.%s\"",
-					 get_namespace_name(relnamespace), relname),
+							get_namespace_name(relnamespace), relname),
 					 errdetail("Conflict schema modifications are currently disallowed.")));
 	}
 
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 93791210e35..2a27f6f4266 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -156,7 +156,8 @@ check_publication_add_schema(Oid schemaid)
  * is really inadequate for that, since the information_schema could be
  * dropped and reloaded and then it'll be considered publishable.  The best
  * long-term solution may be to add a "relispublishable" bool to pg_class,
- * and depend on that instead of OID checks.
+ * and depend on that instead of OID checks.  IsConflictLogTableClass()
+ * excludes tables in conflict schema.
  */
 static bool
 is_publishable_class(Oid relid, Form_pg_class reltuple)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 88f22bbb286..af2470f60c4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -845,9 +845,8 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 		CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
 
 	/*
-	 * If logging to a table is required, physically create it now. We create
-	 * the conflict log table here so its relation OID can be stored when
-	 * inserting the pg_subscription tuple below.
+	 * We create the conflict log table here, if required, so that its relation
+	 * OID can be stored when inserting the pg_subscription tuple below.
 	 */
 	if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
 		logrelid = create_conflict_log_table(subid, stmt->subname, owner);
@@ -877,8 +876,8 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 	}
 
 	/*
-	 * If conflicts are logs to table establish an internal dependency
-	 * between the conflict log table and the subscription.
+	 * Establish an internal dependency between conflict log table and
+	 * subscription.
 	 *
 	 * We use DEPENDENCY_INTERNAL to signify that the table's lifecycle is
 	 * strictly tied to the subscription, similar to how a TOAST table relates
@@ -887,7 +886,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 	 * This ensures the conflict log table is automatically reaped during a
 	 * DROP SUBSCRIPTION via performDeletion().
 	 */
-	if (CONFLICTS_LOGGED_TO_TABLE(opts.conflictlogdest))
+	if (OidIsValid(logrelid))
 	{
 		ObjectAddress clt;
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2dcf64fa53b..0a0ca2b850e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2457,8 +2457,8 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
 	 * pg_largeobject and pg_largeobject_metadata to be truncated as part of
 	 * pg_upgrade, because we need to change its relfilenode to match the old
 	 * cluster, and allowing a TRUNCATE command to be executed is the easiest
-	 * way of doing that.  We also allow TRUNCATE on the conflict log tables,
-	 * to permit users to manually prune these logs to manage disk space.
+	 * way of doing that. We also allow TRUNCATE on the conflict log tables,
+	 * to permit users to manually prune conflict data to manage disk space.
 	 */
 	if (!allowSystemTableMods && IsSystemClass(relid, reltuple) &&
 		!IsConflictLogTableClass(reltuple)

---

From: vignesh C <[email-masked]>
Date: Sun, 31 May 2026 17:38:05 +0530

> + mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);

This was done to fix Shveta's comments from [1] to throw "cannot
modify or insert data into conflict log table" instead of a generic
permission denied error for the owner of the conflict log table.
[1] - https://www.postgresql.org/message-id/CAJpy0uANkzTyUjO2W0=RtaJCGg=VYcwLGGCpqax=[email-masked]

Regards,
Vignesh

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 1 Jun 2026 04:36:36 +0530

> [1] - https://www.postgresql.org/message-id/CAJpy0uANkzTyUjO2W0=RtaJCGg=VYcwLGGCpqax=[email-masked]

Thanks for pointing it, I will analyze this behavior and give my opinion.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 1 Jun 2026 04:53:47 +0530

> Thanks for pointing it, I will analyze this behavior and give my opinion.

While thinking more about this, wouldn't the behaviour is same as
pg_toast table, I mean, the superuser will get "cannot change TOAST
relation "pg_toast_16404" whereas the owner of the toast will get a
permission denied error?

-- 
Regards,
Dilip Kumar
Google

---

From: Peter Smith <[email-masked]>
Date: Mon, 1 Jun 2026 11:39:58 +1000

> Few comments on 0001 and 0002

I think removing the word 'log' would introduce ambiguity.

IMO
A "conflict" is a *thing* (e.g. constraint violation) that happened.
And the "conflict log" is the *log* of the thing that happened.

e.g. "subconflictlogrelid" is clearly the relid of the conflict log
(CLT), but if it was just called "subconflictrelid" that sounds more
like the relid of where the conflict occurred.

> changing the name of Enum ConflictLogDest as well.

Another alternative for option name could be "conflict_reporting" with
values "log/table/all"

But, renaming the option may cause a cascade of necessary name changes
to everything else in docs and code.
e.g. Because the current option is "conflict_log_destination" we have
the CLT (Conflict Log Table)
e.g. If the option is "conflict_data_destination" then shouldn't the
CLT become the CDD (Conflict Data Table).
e.g. If the option is "conflict_reporting" then shouldn't the CLT
become the CRT (Conflict Report Table).
etc.

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

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 1 Jun 2026 09:03:36 +0530

> like the relid of where the conflict occurred.

I agree with Peter that `subconflictlogrelid` makes more sense to
indicate this is the conflict log table's relid, not the conflicting
table's relid.  I do not have any strong opinion about
subconflictlogdest, IMHO subconflictlogdest, subconflictdest both
sounds fine.

> > changing the name of Enum ConflictLogDest as well.

IMHO the 'log' in name conflict_log_destination doesn't really sounds
confusing with destination 'log'.  The conflict_log_destination
clearly indicates the destination for the conflict log whereas the log
in values "log/table/all" says the destination is server logs.

-- 
Regards,
Dilip Kumar
Google

---

From: Amit Kapila <[email-masked]>
Date: Mon, 1 Jun 2026 17:11:52 +0530

> in values "log/table/all" says the destination is server logs.

Fair enough. But let's at least change function names as I suggest. I
would like to suggest changing GetLogDestination to
GetConflictLogDest(). drop_sub_dependencies() is too generi, the
function only drops the conflict log tab, so, calling it
drop_sub_dependencies implies it handles all subscription
dependencies. drop_sub_conflict_log_table() is more accurate and won't
confuse future readers who see it called from DropSubscription.
CONFLICTS_LOGGED_TO_FILE(): "File" is not what the server log is
called anywhere in PostgreSQL, it should be CONFLICTS_LOGGED_TO_LOG().
Additionally, IsConflictLogTableClass() vs IsConflictNamespace()
sounds inconsistent.

-- 
With Regards,
Amit Kapila.

---

From: Nisha Moond <[email-masked]>
Date: Mon, 1 Jun 2026 17:53:08 +0530

> attached for creating conflict log table after inserting subscription

Thanks for the patches. Please find a couple of comments on v43:

1) A non-superuser cannot read the new columns 'subconflictlogrelid'
and 'subconflictlogdest' from pg_subscription.
A comment in system_views.sql say:
"-- All columns of pg_subscription except subconninfo are publicly readable."

I think we should grant public access to these new columns as well.

2) patch-002: conflict.c

-const ConflictLogColumnDef ConflictLogSchema[] = {
+StaticAssertDecl(lengthof(ConflictLogDestNames) == 3,
+ "ConflictLogDestNames length mismatch");
+

Should we use "CONFLICT_LOG_DEST_ALL + 1" instead of the hard-coded value "3"?

The attached diff fixes the above items, along with a few indentation
and whitespace. Please consider it if you agree with the changes.

--
Thanks,
Nisha
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 19c568f4705..16ab446a4a7 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3357,14 +3357,14 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
 				classForm->relkind != RELKIND_VIEW)
 			{
 				/*
-				* Deny anyone permission to update a system catalog unless
-				* pg_authid.rolsuper is set.
-				*
-				* As of 7.4 we have some updatable system views; those
-				* shouldn't be protected in this way.  Assume the view rules
-				* can take care of themselves.  ACL_USAGE is if we ever have
-				* system sequences.
-				*/
+				 * Deny anyone permission to update a system catalog unless
+				 * pg_authid.rolsuper is set.
+				 *
+				 * As of 7.4 we have some updatable system views; those
+				 * shouldn't be protected in this way.  Assume the view rules
+				 * can take care of themselves.  ACL_USAGE is if we ever have
+				 * system sequences.
+				 */
 				mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
 						  ACL_USAGE);
 			}
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 46d27ed02a9..9aabef3ccef 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -293,7 +293,6 @@ IsConflictNamespace(Oid namespaceId)
 	return namespaceId == PG_CONFLICT_NAMESPACE;
 }
 
-
 /*
  * IsReservedName
  *		True iff name starts with the pg_ prefix.
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 73a1c1c4670..166329551da 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1527,7 +1527,8 @@ GRANT SELECT (oid, subdbid, subskiplsn, subname, subowner, subenabled,
               subbinary, substream, subtwophasestate, subdisableonerr,
 			  subpasswordrequired, subrunasowner, subfailover,
               subretaindeadtuples, submaxretention, subretentionactive,
-              subserver, subslotname, subsynccommit, subpublications, suborigin)
+              subserver, subconflictlogrelid, subconflictlogdest,
+			  subslotname, subsynccommit, subpublications, suborigin)
     ON pg_subscription TO public;
 
 CREATE VIEW pg_stat_subscription_stats AS
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 67311a65e29..83615277941 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -844,7 +844,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 		CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
 
 	values[Anum_pg_subscription_subconflictlogrelid - 1] =
-									ObjectIdGetDatum(InvalidOid);
+		ObjectIdGetDatum(InvalidOid);
 
 	tup = heap_form_tuple(RelationGetDescr(rel), values, nulls);
 
@@ -869,9 +869,9 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
 		memset(replaces, false, sizeof(replaces));
 
-									true;
+			true;
 
 		/* Make subscription tuple visible before updating it. */
 		CommandCounterIncrement();
@@ -1897,9 +1897,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 						if (update_relid)
 						{
 							values[Anum_pg_subscription_subconflictlogrelid - 1] =
-														ObjectIdGetDatum(relid);
+								ObjectIdGetDatum(relid);
 							replaces[Anum_pg_subscription_subconflictlogrelid - 1] =
-														true;
+								true;
 						}
 					}
 				}
diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c
index 27830ccb0cc..22442cbbe4f 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -44,7 +44,7 @@ const char *const ConflictLogDestNames[] = {
 	[CONFLICT_LOG_DEST_ALL] = "all"
 };
 
-StaticAssertDecl(lengthof(ConflictLogDestNames) == 3,
+StaticAssertDecl(lengthof(ConflictLogDestNames) == CONFLICT_LOG_DEST_ALL + 1,
 				 "ConflictLogDestNames length mismatch");
 
 /* Structure to hold metadata for one column of the conflict log table */
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index cc31b4d00bc..89d2300abe1 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -95,7 +95,7 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) BKI_SHARED_RELATION BKI_ROW
 	Oid			subserver BKI_LOOKUP_OPT(pg_foreign_server);	/* If connection uses
 																 * server */
 
-	Oid         subconflictlogrelid; /* Relid of the conflict log table. */
+	Oid			subconflictlogrelid; /* Relid of the conflict log table. */
 #ifdef CATALOG_VARLEN			/* variable-length fields start here */
 	/*
 	 * Strategy for logging replication conflicts:
diff --git a/src/include/replication/worker_internal.h b/src/include/replication/worker_internal.h
index 6a447da6510..6b6525dc2e2 100644
--- a/src/include/replication/worker_internal.h
+++ b/src/include/replication/worker_internal.h
@@ -260,7 +260,7 @@ extern PGDLLIMPORT List *table_states_not_ready;
 
 extern XLogRecPtr remote_final_lsn;
 extern TimestampTz remote_commit_ts;
-extern TransactionId	remote_xid;
+extern TransactionId remote_xid;
 
 extern void logicalrep_worker_attach(int slot);
 extern LogicalRepWorker *logicalrep_worker_find(LogicalRepWorkerType wtype,

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 2 Jun 2026 10:51:03 +0530

> and whitespace. Please consider it if you agree with the changes.

PFA, latest version of patch

Changes:
1. Includes Nisha's suggestion from v43_comments_nisha.patch
2. Include Amit's suggestion from v43-0001_amit.1.patch
3. Include Amit's suggestion: change GetLogDestination to
GetConflictLogDest(), drop_sub_conflict_log_table() to
drop_sub_conflict_log_table(), CONFLICTS_LOGGED_TO_FILE to
CONFLICTS_LOGGED_TO_LOG and renamed IsConflictNamespace() to
IsConflictLogTableNamespace()
4. Include Vignesh's fix Parallel_apply_failure.patch
5. Change conflict log table name back to conflict_log_table_<subid>
6. Changed acl to exclude INSERT/UPDATE/USAGE as it was there in initial patch
7. Merged 0002 to 0001 and 0004 to 0003 after review

Open
1. Vignesh please rebase upgrade and \dRs+ page
2. I will review and update the doc patch accordingly

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 2 Jun 2026 12:16:50 +0530

> 2. I will review and update the doc patch accordingly

I missed to commit conflict table name change in 0002 due to that
there was a test failure, PFA updated version

--
Regards,
Dilip Kumar
Google

---

From: Peter Smith <[email-masked]>
Date: Tue, 2 Jun 2026 17:41:33 +1000

Hi Dilip.

I've been thinking more about this new option:
conflict_log_destination (enum) = 'log' or 'table' or 'all'.

I think it behaves more like a bitmap than an enum. Also, having 'all'
as an enum value seems odd to me. IMO, this new option is actually
more similar to the 'publish' option from CREATE PUBLICATION.

I suggest it might be better to implement this as a *string* option:
conflict_log_destination (string), and has allowed values of 'log' and 'table'.

e.g.
conflict_log_destination = 'log'
conflict_log_destination = 'table'
conflict_log_destination = 'log, table'

Apart from being more intuitive and readable, this way is also
future-proof in case some 3rd/4th/etc way of logging is invented --
e.g. the user can define whatever combinations they want instead of
being stuck with only "all".

Thoughts?

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

---

From: Peter Smith <[email-masked]>
Date: Tue, 2 Jun 2026 18:10:35 +1000

Hi Dilip.

Some minor review comments for patch v44-0001.

======
src/backend/catalog/aclchk.c

pg_class_aclmask_ext:

1.
+ /*
+ * For conflict log tables, allow non-superusers to perform
+ * DELETE and TRUNCATE for cleanup and maintenance. While still
+ * restricting INSERT, UPDATE, and USAGE.
+ */

/and maintenance. While still/and maintenance, while still/

======

heap_create:

2.
+ if (IsConflictLogTableNamespace(relnamespace))
+ ereport(ERROR,

This should use the new function to get the schema-qualified relname,
and substitute into "%s" instead of "%s.%s".

======

CreateSubscription:

3.
+ /*

/between conflict log table and subscription./between the conflict log
table and the subscription./

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 2 Jun 2026 16:22:30 +0530

> being stuck with only "all".

I think if we support other formats where conflict logs can be stored
then 'all' will be more relevant as instead of typing all the formats
individually users can use 'all'. Also, we should be able to extend
this option to multi-value if required in future, so the current
proposal seems reasonable to me.

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Wed, 3 Jun 2026 10:10:57 +0530

> 1. Vignesh please rebase upgrade and \dRs+ page

Here is the rebased version of the upgrade and \dRs patch which is
present in v45-0003 and v45-0004. There is no change in v45-0001 and
v45-0002, they are the same patch as in [1].
[1] - https://www.postgresql.org/message-id/CAFiTN-vWZR9%2BU-kg6d2L3J0WGr6fqESnjah8vrguVCmpEG7SMA%40mail.gmail.com

Regards,
Vignesh

---

From: Nisha Moond <[email-masked]>
Date: Wed, 3 Jun 2026 15:24:52 +0530

> v45-0002, they are the same patch as in [1].

Thanks for the updated patches, please find my comments on the v44
patches below. These appear to apply to v45-001 and v45-002 as well,
as both are unchanged.

1) The patches require a catversion bump. It may be worth adding it
now so it does not get missed later during commit.
~~~

2) As ProcessPendingConflictLogTuple() may perform a heap_insert, it
could fail for various reasons, such as table access errors, write
failures, WAL write failures, or ownership-related issues.
I looked into the error handling when CLT insert fails:

2a) When disable_on_error = false

/* Try to allocate a worker for the streaming transaction. */
@@ -5666,6 +5674,7 @@ start_apply(XLogRecPtr origin_startpos)
  */
  AbortOutOfAnyTransaction();
  pgstat_report_subscription_error(MySubscription->oid);
+ ProcessPendingConflictLogTuple();

  PG_RE_THROW();
  }

If ProcessPendingConflictLogTuple() fails, PG_RE_THROW() is never
reached, so the original conflict error is lost and only the secondary
CLT-related error is reported, e.g.:

  ERROR:  could not open relation with OID 16421
  LOG:  background worker "logical replication apply worker" exited
with exit code 1

If the failure persists, the apply worker will keep retrying and users
may never see the actual conflict error. The same concern applies to
the parallel worker case too.

Is there a clean way to ensure the original apply/conflict error is
still logged even if ProcessPendingConflictLogTuple() itself fails?
~~

2b) When disable_on_error = true i.e. DisableSubscriptionAndExit() path

@@ -6040,6 +6049,8 @@ DisableSubscriptionAndExit(void)
  */
  pgstat_report_subscription_error(MyLogicalRepWorker->subid);

+ ProcessPendingConflictLogTuple();
+
  /* Disable the subscription */
  StartTransactionCommand();

This introduces a potential failure point before the subscription is
disabled. If ProcessPendingConflictLogTuple() fails, the subscription
may never be disabled and the worker could repeatedly hit the same
error.

Would it be safer to move ProcessPendingConflictLogTuple() until after
the subscription has been disabled, so that disabling the subscription
is not dependent on successful CLT processing?
~~

2c) When "conflict_log_destination = table" the log reports:

  ERROR:  conflict detected on relation "public.t1": conflict=insert_exists
  DETAIL:  Conflict details are logged to the conflict log table:
pg_conflict_log_16403

However, the subsequent CLT write can still fail:
  ...
  ERROR:  could not open relation with OID 16426
  LOG:  background worker "logical replication apply worker" exited
with exit code 1

Since the CLT write has not yet occurred when this message is emitted,
would it make sense to change it to:
  "Conflict details will be logged to the conflict log table: ..."

to avoid implying that the logging has already succeeded?
~~~

3) The v45-002 commit message still refers to the old conflict log
table name, pg_conflict.pg_conflict_log_for_subid_16396. It should be
updated to pg_conflict_log_16396.
~~~

4) 035_conflicts.pl: I see that v45-002 has corrected the CLT name  in
below test -
+# Verify the contents of the Conflict Log Table (CLT)
+# This section ensures that the CLT contains the expected
+# type and specific key data.
+my $subid = $node_subscriber->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'sub_tab';");
+my $clt = "pg_conflict.pg_conflict_log_$subid";
+
+# Wait for the conflict to be logged in the CLT
+my $log_check = $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT count(*) > 0 FROM $clt;"
+);

I wonder why this wasn't caught in the v44 tests. It seems the check
below is silently broken because poll_query_until() keeps retrying
until timeout when the table does not exist.
Should we replace it with an ok() test instead? something like -

# Wait for the conflict to be logged in the CLT
ok($node_subscriber->poll_query_until(
    'postgres',
    "SELECT count(*) > 0 FROM $clt;"),
    'conflict appeared in CLT after insert');
~~~

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Wed, 3 Jun 2026 16:18:42 +0530

> patches below. These appear to apply to v45-001 and v45-002 as well,

Here are couple more issues found during tests:

5) remote_commit_ts is available when streaming=on, in that case, we
are not updating it and hence see NULL in the CLT.
A simple test case:
 -- publisher
  ALTER SYSTEM SET logical_decoding_work_mem = '64kB';
  SELECT pg_reload_conf();
  CREATE TABLE t (a int PRIMARY KEY);
  CREATE PUBLICATION pub FOR TABLE t;

 -- subscriber (pre-insert the conflicting row, so copy_data=false)
  CREATE TABLE t (a int PRIMARY KEY);
  INSERT INTO t VALUES (1);
  CREATE SUBSCRIPTION sub
    CONNECTION 'host=localhost port=9933 dbname=postgres'
    PUBLICATION pub WITH (conflict_log_destination = 'all', streaming
= on, copy_data = false);

Trigger a streaming conflict
  -- publisher
  BEGIN;
  INSERT INTO t SELECT generate_series(2, 50000);
  INSERT INTO t VALUES (1);        -- conflicts with subscriber's existing row
  COMMIT;

  Check the CLT - remote_commit_ts is NULL despite timestamp being known
  -- subscriber
  SELECT conflict_type, remote_commit_ts, remote_commit_lsn
  FROM pg_conflict.pg_conflict_log_<subid>;
   conflict_type  | remote_commit_ts | remote_commit_lsn
  ----------------+------------------+-------------------
   insert_exists  |                  | 0/1234ABC

I think we need to update it in apply_handle_stream_commit() -> case
TRANS_LEADER_APPLY:
  remote_commit_ts = commit_data.committime;
~~~

6) Different ERRORs for superuser vs non-superuser
I created a subscription owned by a non-superuser (nisha), and the CLT
for it is:
postgres=# \d
                   List of relations
   Schema    |         Name          | Type  |  Owner
-------------+-----------------------+-------+---------
 pg_conflict | pg_conflict_log_16469 | table | nisha

Now if I try to UPDATE the CLT as a superuser:

postgres=# update pg_conflict_log_16469 set conflict_type=
'INSERT_EXIST' where conflict_type='insert_exists';
ERROR:  cannot modify or insert data into conflict log table
"pg_conflict_log_16469"
DETAIL:  Conflict log tables are system-managed and only support
cleanup via DELETE or TRUNCATE.

However, running the same command as nisha (the sub/clt owner) results in:

postgres=# SET SESSION AUTHORIZATION nisha;
SET
postgres=> update pg_conflict_log_16469 set conflict_type=
'INSERT_EXIST' where conflict_type='insert_exists';
ERROR:  permission denied for table pg_conflict_log_16469

I think the error should be consistent with the superuser case, rather
than failing with a generic permission error.

--
Thanks,
Nisha

---

From: Zsolt Parragi <[email-masked]>
Date: Wed, 3 Jun 2026 08:58:51 -0500

Hello

+				snprintf(conflictlogtable,
+						 sizeof(conflictlogtable),
+						 "%s.pg_conflict_log_for_subid_%s",
+						 conflict_schema, subid);

This and a few some places / commit message use
pg_conflict_log_for_subid_<oid>, but the proper name seems to be

+	snprintf(relname, NAMEDATALEN, "pg_conflict_log_%u", subid);

@@ -1767,6 +1771,10 @@ apply_handle_stream_start(StringInfo s)
 				(errcode(ERRCODE_PROTOCOL_VIOLATION),
 				 errmsg_internal("invalid transaction ID in streamed replication
transaction")));

+	remote_xid = stream_xid;
+	remote_final_lsn = InvalidXLogRecPtr;
+	remote_commit_ts = 0;

Shouldn't apply_handle_begin_prepare also write out the xid/commit_ts?

---

From: shveta malik <[email-masked]>
Date: Thu, 4 Jun 2026 11:31:36 +0530

> relation "pg_toast_16404" whereas the owner of the toast will get a

Okay, we can have the same behaviour as the Toast table here. I had
asked others' opinions on that, but I did  not notice it was already
fixed without any feedback or comments.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Thu, 4 Jun 2026 16:29:31 +1000

Some review comments for patch v45-0003.

======

heap_create:

1.
+ * Allow creation of conflict table in binary-upgrade mode.

/of conflict table/of the conflict log table/

======

alter_sub_conflictlogdestination:

2.

- relid = create_conflict_log_table(sub->oid, sub->name, sub->owner);
- update_relid = true;
+ snprintf(relname, NAMEDATALEN, "pg_conflict_log_%u", sub->oid);

Currently, we are generating the CLT name here and also in
create_conflict_log_table. I think there should be only *one* place
that does the CLT name generation. e.g. suggest making a new common
function like get_conflict_log_table_name(sub->oid, &relname,
NAMEDATALEN);

~~~

3.
+ relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
+ if (OidIsValid(relid))
+ {
+ /* Existing table from upgrade */
+ Assert(IsBinaryUpgrade);
+ }
+ else

Should this be the other way around?

if (IsBinaryUpgrade)
{
  relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
  Assert(OidIsValid(relid));
}

~~~

4.
+ /*
+ * Establish an internal dependency between the conflict log
+ * table and the subscription.  For details refer comments in
  * CreateSubscription function.
  */

SUGGESTION
Refer to comments in the CreateSubscription function for details.

======
src/bin/pg_dump/pg_dump.c

selectDumpableTable:

5.
+ if (strcmp(tbinfo->dobj.namespace->dobj.name, "pg_conflict") == 0)
+ {
+ if (dopt->binary_upgrade)
+ {
+ /*
+ * Dump pg_conflict tables only during binary upgrade. The schema
+ * is assumed to already exist.
+ */
+ tbinfo->dobj.dump = DUMP_COMPONENT_DEFINITION;
+
+ /*
+ * Suppress the "ALTER TABLE ... OWNER TO ..." command for this
+ * table. This prevents pg_dump from outputting the owner change.
+ */
+ tbinfo->rolname = NULL;
+ }
+ else
+ tbinfo->dobj.dump = DUMP_COMPONENT_NONE;

Doesn't the comment "Dump pg_conflict tables only during binary
upgrade..." really belong above the "if (dopt->binary_upgrade)".

~~~

6.
+ if (fout->remoteVersion >= 190000)
+ appendPQExpBufferStr(query,
+ " s.subconflictlogrelid, s.subconflictlogdest\n");
  else
- appendPQExpBufferStr(query, " NULL AS subservername\n");
+ appendPQExpBufferStr(query,
+ " NULL AS subconflictlogrelid, NULL AS subconflictlogdest\n");

It seems more natural to think of the 'subconflictlogdest' before the
subconflictlogrelid. Perhaps the SQL should swap those around.

~~~

7.
+ if (PQgetisnull(res, i, i_subconflictlogrelid))
+ subinfo[i].subconflictlogrelid = InvalidOid;
+ else
+ {
+ TableInfo  *tableInfo;
+
+ subinfo[i].subconflictlogrelid =
+ atooid(PQgetvalue(res, i, i_subconflictlogrelid));
+
+ if (subinfo[i].subconflictlogrelid)
+ {
+ tableInfo = findTableByOid(subinfo[i].subconflictlogrelid);
+ if (!tableInfo)
+ pg_fatal("could not find conflict log table with OID %u",
+ subinfo[i].subconflictlogrelid);
+
+ addObjectDependency(&subinfo[i].dobj, tableInfo->dobj.dumpId);
+ }
+ }
+
+ if (PQgetisnull(res, i, i_sublogdestination))
+ subinfo[i].subconflictlogdest = NULL;
+ else
+ subinfo[i].subconflictlogdest =
+ pg_strdup(PQgetvalue(res, i, i_sublogdestination));
+

7a.
Those new attributes come as a pair -- e.g. the relid only makes sense
depending on the destination value; maybe only look for the CLT relid
is the destination is not LOG/NULL

Knowing the destination also means you can also do integrity checks
for PQgetisnull(res, i, i_subconflictlogrelid) -- e.g. must be valid
Oid, or must be invalid Oid.

~

7b.
The 'tableInfo' can be declared/assigned in the if block.

~~~

dumpSubscription:

8.
+ if (subinfo->subconflictlogdest &&
+ (pg_strcasecmp(subinfo->subconflictlogdest, "log") != 0))
+ appendPQExpBuffer(query,
+   "\n\nALTER SUBSCRIPTION %s SET (conflict_log_destination = %s);\n",
+   qsubname,
+   subinfo->subconflictlogdest);
+

8a.
Why was this implemented as a separate ALTER SUBSCRIPTION instead of
just another CREATE SUBSCRIPTION option like all the others? If there
is some good reason, these should be a comment here.

~~~

8b.
Not sure why this has a double \n\n prefix instead of just \n. I
didn't see that pattern elsewhere in this file.

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

---

From: shveta malik <[email-masked]>
Date: Thu, 4 Jun 2026 12:28:24 +0530

> [1] - https://www.postgresql.org/message-id/CAFiTN-vWZR9%2BU-kg6d2L3J0WGr6fqESnjah8vrguVCmpEG7SMA%40mail.gmail.com

A few comments:

1)
We have introduced errors like these:

+ errdetail("Conflict schema modifications are currently disallowed.")));
+ errmsg("cannot move objects into or out of the conflict schema")));

But here:

{
/* Can't be system namespace */
if (IsCatalogNamespace(schemaid) || IsToastNamespace(schemaid) ||
IsConflictLogTableNamespace(schemaid))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("cannot add schema \"%s\" to publication",
get_namespace_name(schemaid)),
errdetail("This operation is not supported for system schemas.")));

We have included conflict-schema in system schemas, I feel here too,
we shall make a new condition and error mentioning 'conflict schema'.
Irrespective of how TOAST-scehma is handled here, we can have
cosnsitent handling at-least for new conflcit-schema.

2)
+#include "catalog/dependency.h"
+#include "commands/subscriptioncmds.h"

conflict.c cimpiles without above 2 inclusions.

v45-0002:
3)
old name (pg_conflict_log_for_subid_16396) referred in commit message of 002

4)
Since ProcessPendingConflictLogTuple() is called in CATCH-block at
multiple places, it would be better if
ProcessPendingConflictLogTuple() had its own try-catch block. This
block could log/WARN CLT-insertion errors, clear them, and allow the
caller's original error to remain. In rare-scenarios, we may end up
overriding the actual error with a CLT insertion error.

~~
Reviewing more..

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 4 Jun 2026 12:55:22 +0530

> now so it does not get missed later during commit.

Yeah we can do this

> still logged even if ProcessPendingConflictLogTuple() itself fails?

Does this happening when destination is table or with all as well?

> is not dependent on successful CLT processing?

Yeah, I think it make sense, I don't see obvious issue with moving it
after disabling the subscription.

> to avoid implying that the logging has already succeeded?

IMHO "Conflict details are logged to the conflict log table" doesn't
mean a conflict has been logged to the table; what I interpret from
this message is that conflict logging to the table is enabled.

>     'conflict appeared in CLT after insert');

Why would CLT not exist, IIUC this is positive test where we are
trying to validate the conflict data from CLT, no?

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 4 Jun 2026 14:09:36 +0530

>    conflict_type  | remote_commit_ts | remote_commit_lsn

Yeah we need to fix this, I will fix in next version.

>    Schema    |         Name          | Type  |  Owner

This has already been discussed, and the same behavior difference
exists for the toast table as well, because for superuser we do not
mask the permission at acl check instead we block at the operation
level however for non superuser it is blocked at the acl check and we
are keeping the same behavious for the conflict log table as well.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 4 Jun 2026 14:30:50 +0530

> cosnsitent handling at-least for new conflcit-schema.

IMHO then the next immediate thought would be that for toast and
catalog the error is covering them under the system catalog and we are
diverging behaviour for the conflict schema.  I think the current
behaviour is fine.

> conflict.c cimpiles without above 2 inclusions.

Will fix

> old name (pg_conflict_log_for_subid_16396) referred in commit message of 002

Will fix

> overriding the actual error with a CLT insertion error.

I think this make sense, let me put more thought on this.

-- 
Regards,
Dilip Kumar
Google

---

From: Nisha Moond <[email-masked]>
Date: Thu, 4 Jun 2026 14:43:43 +0530

> Does this happening when destination is table or with all as well?

It can happen with both conflict_log_destination = 'table' and 'all',
since for conflicts such as insert_exists (ERROR), we call
ProcessPendingConflictLogTuple() to insert the conflict into the CLT
in both cases.

I used the following steps to reproduce the error:
 - Attach gdb to the apply worker and set a breakpoint in
ProcessPendingConflictLogTuple().
 - Trigger an insert_exists conflict.
 - After execution passes StartTransactionCommand(), change
conflict_log_destination to log from another session.
 - The conflict log table is dropped, causing table_open() to fail and
resulting in the above error.

I think Shveta's suggestion #4 in [1] should address this issue.

> this message is that conflict logging to the table is enabled.

I agree it can be read either way, but I felt the intent might be
ambiguous from a user's perspective, so I raised it.
That said, I'm fine with it if others also don't see any ambiguity.

> trying to validate the conflict data from CLT, no?

Ideally, the CLT should exist. I initially assumed the tests were
clean in v44 despite the incorrect table name being used.
I re-ran the tests and confirmed that poll_query_until() does report a
failure if it times out because the CLT is unavailable. Sorry for the
noise.

[1] https://www.postgresql.org/message-id/CAJpy0uAA_XszCAcoBuCUM3VobD39DbMDwCPUT%2BXW7wFfE%2B_E8w%40mail.gmail.com

--
Thanks,
Nisha

---

From: shveta malik <[email-masked]>
Date: Thu, 4 Jun 2026 14:47:35 +0530

Currently we allow inheritance from CLT. I see 2 side-effects

a) Queries on the CLT will always include rows from inherited tables.
Whether this is desirable or not is subjective, but it can lead to
unexpected results for DBAs querying the CLT directly.

CREATE TABLE public.my_custom_extended_log() INHERITS
(pg_conflict.pg_conflict_log_16392);
INSERT INTO public.my_custom_extended_log (relid, schemaname, relname,
conflict_type) VALUES (99999, 'public', 'fake_table',
'forged_conflict_type');

postgres=# SELECT count(*), conflict_type FROM
pg_conflict.pg_conflict_log_16392 GROUP BY conflict_type;
 count |    conflict_type
-------+----------------------
     5 | insert_exists
     2 | forged_conflict_type

b)
DROP SUBSCRIPTION may unintentionally drop inherited tables.

When a subscription is dropped, the associated CLT is removed
internally using DROP-CASCADE. As a result, any user table inheriting
from the CLT would also be dropped, causing the user to lose any data
stored in those inherited tables.

postgres=# DROP SUBSCRIPTION sub1;
NOTICE:  drop cascades to table my_custom_extended_log
NOTICE:  dropped conflict log table
"pg_conflict.pg_conflict_log_16392" for subscription "sub1"
NOTICE:  dropped replication slot "sub1" on publisher

OTOH, regular tables provide an opportunity for the user to review
dependencies and decide how to proceed:

postgres=# create table tab1( i int);
CREATE TABLE

postgres=# create table tab_i() inherits (tab1);
CREATE TABLE

postgres=# drop table tab1;
ERROR:  cannot drop table tab1 because other objects depend on it
DETAIL:  table tab_i depends on table tab1
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

~~

I previously thought inheriting from CLT should be acceptable as that
is purely the user's decision to create a child table for its own
purpose, but upon rethinking, I believe it should be blocked.
Thoughts?

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 4 Jun 2026 14:54:26 +0530

> pg_conflict.pg_conflict_log_16392 GROUP BY conflict_type;

Yeah, I don't think there should be any use case for inheriting from
CLT.  IMHO restricting this might help us handle more such cases.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Thu, 4 Jun 2026 16:05:06 +0530

I noticed that it is currently possible to acquire explicit locks on a CLT:

--Session locks table and does not commit txn:
postgres=# BEGIN;
LOCK TABLE pg_conflict.pg_conflict_log_16481 IN SHARE MODE;
BEGIN
LOCK TABLE

Doing so can cause the apply worker to block indefinitely when it
attempts to modify the CLT:

[247433] LOG:  logical replication apply worker for subscription
"sub1" has started
[247433] LOG:  process 247433 still waiting for RowExclusiveLock on
relation 16482 of database 5 after 1001.030 ms
[247433] DETAIL:  Process holding the lock: 245584. Wait queue: 247433.
[247433] CONTEXT:  waiting for RowExclusiveLock on relation 16482 of database 5

Toast Table behaviour:
postgres=*# LOCK TABLE pg_toast.pg_toast_16384 IN SHARE MODE;
ERROR:  cannot lock relation "pg_toast_16384"
DETAIL:  This operation is not supported for TOAST tables.

Should we consider disallowing explicit LOCK TABLE operations on CLTs,
similar to how PostgreSQL handles TOAST tables? Or does anyone see any
legitimate use-case (I don't) where we would need to allow explicit
LOCKs on CLT?

thanks
Shveta

---

From: Nisha Moond <[email-masked]>
Date: Thu, 4 Jun 2026 16:24:43 +0530

> v45-0002, they are the same patch as in [1].

Please find a couple of minor comments for v45-001:

1) drop_sub_conflict_log_table()

+ conflictrelname = get_rel_name(subconflictlogrelid);
+ if (conflictrelname == NULL)
+ elog(ERROR, "cache lookup failed for index %u",
+ subconflictlogrelid);

Shouldn't the error say -  "cache lookup failed for relation" instead
of "index"?

2) AlterSubscription()
+ if (IsSet(opts.specified_opts, SUBOPT_CONFLICT_LOG_DEST))
+ {
+ ConflictLogDest old_dest =
+ GetConflictLogDest(sub->conflictlogdest);
+
+ if (opts.conflictlogdest != old_dest)
+ {
+ bool update_relid;
+ Oid relid = InvalidOid;
+
+ values[Anum_pg_subscription_subconflictlogdest - 1] =
+ CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
+ replaces[Anum_pg_subscription_subconflictlogdest - 1] = true;
+
+ update_relid = alter_sub_conflictlogdestination(sub,
+ opts.conflictlogdest,
+ &relid);

Small optimization: we call GetConflictLogDest() twice in this flow,
once to compute old_dest and again inside
alter_sub_conflictlogdestination().
Could we pass old_dest to alter_sub_conflictlogdestination() instead?

--
Thanks,
Nisha

---

From: Peter Smith <[email-masked]>
Date: Fri, 5 Jun 2026 12:29:04 +1000

Hi Vignesh.

Some review comments for the patch v45-0004.

======
src/bin/psql/describe.c

describeSubscriptions:

1.
+ if (pset.sversion >= 190000)
+ {
+ appendPQExpBuffer(&buf,
+   ", (select srvname from pg_foreign_server where oid=subserver) AS \"%s\"\n",
+   gettext_noop("Server"));

- /* Skip LSN is only supported in v15 and higher */
- if (pset.sversion >= 150000)
- appendPQExpBuffer(&buf,
-   ", subskiplsn AS \"%s\"\n",
-   gettext_noop("Skip LSN"));
+ appendPQExpBuffer(&buf,
+   ", subretaindeadtuples AS \"%s\"\n",
+   gettext_noop("Retain dead tuples"));

  appendPQExpBuffer(&buf,
-   ",  pg_catalog.obj_description(oid, 'pg_subscription') AS \"%s\"\n",
-   gettext_noop("Description"));
+   ", submaxretention AS \"%s\"\n",
+   gettext_noop("Max retention duration"));
+
+ appendPQExpBuffer(&buf,
+   ", subretentionactive AS \"%s\"\n",
+   gettext_noop("Retention active"));
+
+ ncols += 4;
+ }

These can all be combined together, so there is just a single
appendPQExpBuffer. Same as other single code fragments in the same
function.

~~~

2.
+ appendPQExpBuffer(&buf,
+   ",  pg_catalog.obj_description(oid, 'pg_subscription') AS \"%s\"\n",
+   gettext_noop("Description"));
+ ncols++;
+
+ /* Conflict log destination is supported in v19 and higher */
+ if (pset.sversion >= 190000)
+ {
+ appendPQExpBuffer(&buf,
+   ", subconflictlogdest AS \"%s\"\n",
+   gettext_noop("Conflict log destination"));
+ ncols++;

I think the "Description" should remain as the very last column of the
table, same as it always has been. All those others are options so
they belong together, but description is different.

~~~

3.
+ printTableAddHeader(&cont, gettext_noop("Description"), true, align);
+ printTableAddCell(&cont, PQgetvalue(res, i, current_col++), false, false);
+
+ if (pset.sversion >= 190000)
+ {
+ char    *logdest;
+
+ printTableAddHeader(&cont, gettext_noop("Conflict log destination"),
+ true, align);

Ditto. Leave the description as the very last column.

~~~

4.
+ if (pset.sversion >= 190000)
+ {
+ char    *logdest;
+
+ printTableAddHeader(&cont, gettext_noop("Conflict log destination"),
+ true, align);
+
+ logdest = PQgetvalue(res, i, current_col++);
+
+ printTableAddCell(&cont, logdest, false, false);
+
+ if (strcmp(logdest, "table") == 0 ||
+ strcmp(logdest, "all") == 0)
+ {
+ char conflictlogtable[NAMEDATALEN + 32];
+
+ snprintf(conflictlogtable,
+ sizeof(conflictlogtable),
+ "%s.pg_conflict_log_for_subid_%s",
+ conflict_schema, subid);
+
+ printTableAddFooter(&cont, _("Conflict log table:"));
+ printTableAddFooter(&cont, psprintf("    %s", conflictlogtable));
+ }
+ }
+

4a.
This 'snprintf' is yet another code fragment that is building the
generated CLT name. This should be using some common function to do
it. See my review comment for an earlier patch in this set [1, comment
#2].

~~~

4b.
+ printTableAddFooter(&cont, _("Conflict log table:"));
+ printTableAddFooter(&cont, psprintf("    %s", conflictlogtable));

Normally in these describe commands, the footer part (e.g. "Except
tables:" etc) puts the names in quotes. So, even though it may not be
strictly necessary, this should do the same for consistency:

e.g.
BEFORE
Conflict log table:
    pg_conflict.pg_conflict_log_for_subid_16392

AFTER
Conflict log table:
    "pg_conflict.pg_conflict_log_for_subid_16392"

~~~

4c.
IMO there should be a separate function for handling the subscription
footer/s, same as there is already a function
addFooterToPublicationDesc.

======
src/test/regress/expected/subscription.out

Some general comments about the describe testing:

5.
AFAICT there this patch is not being tested properly because there are
no tests where the conflict_log_destination is "table" or "all".

~~~

6.
Even though the \dRs code was changed a lot, it seems there were no
impacted tests because they were all using \dRs+ and never \dRs. So I
think there needs to be more "describe" tests that are using *both*
forms of this command.

Also consider using expanded form \x for some of the \dRs and \dRs+
tests, so that the expected output is easier to read.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPt3%3DrZO%2ByJqj7o3xH0LcgrztFCvhoayiBJWbhmEio6teQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 5 Jun 2026 11:53:33 +0530

> legitimate use-case (I don't) where we would need to allow explicit

We need to add namespace-based checks here, as the current logic
relies solely on relkind [1], which classifies TOAST tables
separately. In my view, choosing to either allow or disallow this
behavior will not cause significant inconvenience or seem unusual to
anyone. Therefore, I prefer the path that minimizes special-purpose
code. Since explicitly disallowing this requires additional
special-purpose logic (as shown below [1]), allowing it seems to be
the cleaner approach.  Thoughts?

[1]
@@ -92,6 +93,14 @@ RangeVarCallbackForLockTable(const RangeVar *rv,
Oid relid, Oid oldrelid,

                                                rv->relname),

                                 errdetail_relkind_not_supported(relkind)));

+       /* Disallow explicit LOCK TABLE on conflict log tables */
+       if (IsConflictLogTableNamespace(get_rel_namespace(relid)))
+               ereport(ERROR,
+                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                errmsg("cannot lock relation \"%s\"",
+                                               rv->relname),
+                                errdetail("This operation is not
supported for conflict log tables.")));
+

        /*
         * Make note if a temporary relation has been accessed in this
         * transaction.
@@ -198,6 +207,14 @@ LockViewRecurse_walker(Node *node,
LockViewRecurse_context *context)
                                relkind != RELKIND_VIEW)
                                continue;

+                       /* Disallow locking conflict log tables even
via views. */
+                       if
(IsConflictLogTableNamespace(get_rel_namespace(relid)))
+                               ereport(ERROR,
+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                                errmsg("cannot lock
relation \"%s\"",
+                                                               relname),
+                                                errdetail("This
operation is not supported for conflict log tables.")));
+

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Fri, 5 Jun 2026 15:06:13 +0530

> the cleaner approach.  Thoughts?

Okay, upon analyzing this new logic,  I too prefer to allow it.

I was thinking if there is a way to set lock_timeout in
ProcessPendingConflictLogTuple() or try to acquire lock and if it
fails we hit 'ERRCODE_LOCK_NOT_AVAILABLE', log a different warning in
the log file and let the apply worker proceed.

But if this too is complicated, I am fine with the current
implementation. Since LOCK TABLE is a well-known command, if a user
explicitly locks a CLT, they should be responsible for the
consequences such as blocking the apply worker.

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 5 Jun 2026 16:21:56 +0530

> consequences such as blocking the apply worker.

+1

Here is the updated patch which fixes all open issues except Peter
reported on 0004 patch, Vignesh would you take care of that?

--
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Mon, 8 Jun 2026 11:42:50 +0530

> reported on 0004 patch, Vignesh would you take care of that?

Thank You Dilip.

v46-001:

1)

+static bool alter_sub_conflictlogdestination(Subscription *sub,
+ ConflictLogDest oldlogdest,
+ ConflictLogDest newlogdest,
+ Oid *conflicttablerelid);

+static void drop_sub_conflict_log_table(Oid subid, char *subname,
+

Can we name alter_sub_conflictlogdestination to
alter_sub_conflict_log_dest?  Feel free to ignore if you find current
name better.

2)
Ran all the tests again on 0001 alone, inheritance is still working.
Let me know if we decided to retain it.  IMO, it is better to block it
for the reasons stated earlier.  The changes can be made in
MergeAttributes and ATExecAddInherit; we already have similar relation
based restrictions there, one more can be added for CLT.

~~

No major issue in 0001, it seems be in good shape. Will do one more
round of reveiw and testing on next version though.

thanks
Shveta

---

From: Nisha Moond <[email-masked]>
Date: Mon, 8 Jun 2026 14:52:02 +0530

> reported on 0004 patch, Vignesh would you take care of that?

Thanks Dilip for the updated patches.

Patch 001 + 002
1) Issue #5 from [1] still appears to be present. I still see NULL
values in remote_commit_ts. From patch, it looks like remote_commit_ts
is updated in v46, but too late in the flow. I tested by moving:
    /* Set remote_commit_ts for conflict logging. */
    remote_commit_ts = commit_data.committime;
before the apply_spooled_messages() call, and remote_commit_ts was
logged correctly. It seems the assignment needs to happen earlier.

2) CLT reports NULL or stale remote_xid for two-phase transactions
Test case:
setup : pub-sub nodes with max_prepared_transactions = 10 to enable two_phase
-- Publisher setup:
  CREATE TABLE clt_test (id int PRIMARY KEY);
  CREATE PUBLICATION clt_pub FOR TABLE clt_test;

-- Subscriber setup:
  CREATE TABLE clt_test (id int PRIMARY KEY);
  INSERT INTO clt_test VALUES (1);   -- pre-existing row to conflict
  CREATE SUBSCRIPTION clt_sub
      CONNECTION 'port=9933 dbname=postgres'
      PUBLICATION clt_pub
      WITH (two_phase = true, conflict_log_destination = 'all');

-- Trigger a conflict via a prepared transaction on the publisher:
-- Publisher:
  BEGIN;
  select txid_current();
  INSERT INTO clt_test VALUES (1);   -- will conflict on subscriber
  PREPARE TRANSACTION 'clt_bug_test';
  COMMIT PREPARED 'clt_bug_test';

-- Check the CLT on the subscriber. The publisher transaction was 699,
but the CLT contains:
postgres=# SELECT conflict_type, remote_xid, remote_commit_ts FROM
pg_conflict.pg_conflict_log_16398 ;
 conflict_type | remote_xid |         remote_commit_ts
---------------+------------+----------------------------------
 insert_exists |        698 | 2026-06-08 14:04:34.903284+05:30
 insert_exists |            |
 insert_exists |            |
...
It looks like remote_xid is either stale values from a previous
transaction or remain unset when apply worker tries again.
The change below fixes it.
@@ -1300,6 +1300,8 @@ apply_handle_begin_prepare(StringInfo s)
        set_apply_error_context_xact(begin_data.xid, begin_data.prepare_lsn);

        remote_final_lsn = begin_data.prepare_lsn;
+       remote_xid = begin_data.xid;
+       remote_commit_ts = 0;
~~~

patch-003
3) alter_sub_conflictlogdestination():
+ relid = get_relname_relid(relname, PG_CONFLICT_NAMESPACE);
+ if (OidIsValid(relid))
+ {
+ /* Existing table from upgrade */
+ Assert(IsBinaryUpgrade);
+ }
+ else
+ {
+ relid = create_conflict_log_table(sub->oid, sub->name,
+   sub->owner);
+ }
+

I think Assert() above should be replaced with an ERROR similar to the
one in create_conflict_log_table().
One possible scenario is that a user manually creates
pg_conflict.pg_conflict_log_<subid> with allow_system_table_mods = on.
In that case:
 -- Assert builds will hit the assertion.
 -- Non-assert builds will silently skip table creation, and later CLT
operations may fail in unexpected ways because the user-created table
definition may not match the expected schema.

Before patch-003, create_conflict_log_table() correctly reported an
error if a table with the same name already existed, which seems
safer.

4) 002_pg_dump.pl
- WITH (connect = false, origin = any, streaming = on);',
+ WITH (connect = false, origin = any, streaming = on,
conflict_log_destination= table);',

typo: space before "=".
/conflict_log_destination= table/conflict_log_destination = table/
~~~

patch-005
5) doc/src/sgml/ddl.sgml

typo : repeated "pg_catalog"
It should be: Unlike <literal>pg_catalog</literal>, the
<literal>pg_conflict</literal> schema is not implicitly...
~~~

[1] https://www.postgresql.org/message-id/CABdArM5N0LgkHc%2BJOKcbDjpx%3D0hdWDx%2BJ7y%3DUS2apEEmi07oyw%40mail.gmail.com

--
Thanks,
Nisha

---

From: shveta malik <[email-masked]>
Date: Mon, 8 Jun 2026 15:10:20 +0530

v46-0002:

1)
I was trying to verify TRY-CATCH block of ProcessPendingConflictLogTuple().

When I force InsertConflictLogTuple() to fail while atatching
debugger, I see a new error due to CATCH block. See this dump:

-----------------
[27532] WARNING:  could not log conflict to table for subscription
"sub1": cannot open relation "pg_conflict_log_16391"
[27532] ERROR:  errstart was not called
[27102] LOG:  background worker "logical replication apply worker"
(PID 27532) exited with exit code 1
[27548] LOG:  logical replication apply worker for subscription "sub1"
has started
[27548] ERROR:  conflict detected on relation "public.tab1":
conflict=insert_exists
[27548] DETAIL:  Could not apply remote change: remote row (4).
        Key already exists in unique index "tab1_pkey", modified in
transaction 793: key (i)=(4), local row (4).
[27548] CONTEXT:  processing remote data for replication origin "pg_16391...
-----------------

'ERROR:  errstart was not called' is raised perhaps due to
'FlushErrorState' which sets errordata_stack_depth to -1. If I get rid
of FlushErrorState(), the internal ERROR is not cleared, which results
in the worker exiting (which we are trying to avoid).

-------------------------
[30031] WARNING:  could not log conflict to table for subscription
"sub1": cannot open relation "pg_conflict_log_16391"
[30031] ERROR:  cannot open relation "pg_conflict_log_16391"
------------->this needs to be handled.
[30031] DETAIL:  This operation is not supported for tables.
[30011] LOG:  background worker "logical replication apply worker"
(PID 30031) exited with exit code 1
[30043] LOG:  logical replication apply worker for subscription "sub1"
has started
[30043] ERROR:  conflict detected on relation "public.tab1":
conflict=insert_exists
[30043] DETAIL:  Could not apply remote change: remote row (12).
        Key already exists in unique index "tab1_pkey", modified in
transaction 872: key (i)=(12), local row (12).
------------------------

I am still thinking how this can be done cleanly. Meanwhile putting it
here for others to review/comment.

2)
Also, I think InsertConflictLogTuple() in the non-error path (via
ReportApplyConflict()) should be wrapped in its own TRY-CATCH block.
When I force an error during that insert, execution falls through to
the start_apply CATCH block, which then attempts to insert the same
conflict record again via ProcessPendingConflictLogTuple(). That
insert fails again for the same reason, causing the apply worker to
error out.

Should we keep this behavior and allow the apply worker to halt on a
CLT insertion failure, or would it be better to avoid disrupting
replication by encapsulating the insertion logic in its own TRY-CATCH
block and handling the issue locally by emiting it as WaRNING?
Thoughts?

thanks
Shveta

---

From: vignesh C <[email-masked]>
Date: Mon, 8 Jun 2026 17:05:33 +0530

>   Assert(OidIsValid(relid));

This is done intentionally so that the Assert fails if the table
exists in case of non binary upgrade mode.

> subconflictlogrelid. Perhaps the SQL should swap those around.

I think the existing is better as it does select in the same order as
it is maintained in the table.

> is the destination is not LOG/NULL

I think the existing is better as it does select in the same order as
it is maintained in the table.

> Oid, or must be invalid Oid.

I'm not sure if it is pg_dump's responsibility to do these integrity
checks in this case.

> didn't see that pattern elsewhere in this file.

Generally there is 2 line gaps between the create and alter
subscription, it was done to maintain the same line gaps like below:
CREATE SUBSCRIPTION sub3 CONNECTION 'dbname=doesnotexist' PUBLICATION
pub1 WITH (connect = false, slot_name = 'sub3', streaming = on);

ALTER SUBSCRIPTION sub3 SET (conflict_log_destination = table);

ALTER SUBSCRIPTION sub3 OWNER TO vignesh;

The rest of the comments were fixed. The attached v47 version patch
has the changes for the same.

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Mon, 8 Jun 2026 17:09:15 +0530

> addFooterToPublicationDesc.

It is not required in this case as we don't have multiple footers from
different places to be added here.

> no tests where the conflict_log_destination is "table" or "all".

That is not done intentionally as the description of subscriptions for
conflict log table oid will change and tests can fail. Instead it is
verified using select after create subscription

> think there needs to be more "describe" tests that are using *both*

There are tests for \dRs, since the existing format of \dRs was not
changed, except need not be updated.

> tests, so that the expected output is easier to read.

Since no new tests were added, nothing done for this.

Rest of the comments were fixed. These were handled as part of the v47
version posted at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm2V3EuSKMaTqDvaiLQW3jwBX90aXTkMST1ft%3DuJ8J%2BR5A%40mail.gmail.com

Regards,
Vignesh

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 8 Jun 2026 19:01:28 +0530

> alter_sub_conflict_log_dest?  Feel free to ignore if you find current

Yeah we may change that.

> round of reveiw and testing on next version though.

After rethinking my previous stance on blocking these operations, let
me clarify the core principle I think we should follow for CLTs. I am
completely open to feedback on this approach:

1. Block Direct Mutations: We should block any operation that directly
modifies the CLT or its underlying data (e.g., DROP TABLE, ALTER
TABLE, INSERT, UPDATE), which impact the operation on CLT or update
the CLT data.
2. Don't block Indirect/Edge-Case Operations: We should not write
custom code just to block edge cases that don't directly modify CLT
data or impact the operations on CLT. For example, if a user decides
to inherit from a CLT, that constitutes unexpected usage. We already
document (or can document) that dropping a subscription internally
drops the associated CLT. If a user inherits from it anyway and their
child tables are impacted when the subscription is dropped, that is
expected behavior and their usage issue.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Tue, 9 Jun 2026 09:18:19 +0530

> expected behavior and their usage issue.

Fair enough. I'm okay with this approach, provided we document it
clearly, perhaps as a CAUTION: users must be aware that DROP
SUBSCRIPTION cascades the drop to the CLT and all its dependent
objects, including any user-created inherited tables, view etc

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 9 Jun 2026 09:27:48 +0530

> objects, including any user-created inherited tables, view etc

Make sense..

---

From: Amit Kapila <[email-masked]>
Date: Wed, 10 Jun 2026 12:48:29 +0530

On Mon, Jun 8, 2026 at 7:01 PM Dilip Kumar <[email-masked]> wrote:
> data or impact the operations on CLT.

Unlike system catalogs which are allowed to be modified with the
allow_system_table_mods GUC,  the TOAST model seems more appropriate
for conflict log tables where the external modifications are blocked,
due to following reasons:
1. The apply worker assumes a fixed schema. ConflictLogSchema[] in
conflict.c is a hardcoded array of column definitions used to build
tuples at runtime. If allow_system_table_mods=on lets someone add a
column or attach a trigger that errors out, the apply worker's
insertion path breaks with error or crash. There is no recovery path —
it's not like a user catalog where a DBA might legitimately need to
patch something in an emergency.
2. allow_system_table_mods allowed to modify catalog tables and that
seems to be designed for bootstrap, not conflict log tables. The GUC
exists so initdb can modify system catalogs they own. Conflict log
tables are subscription-specific runtime objects. No legitimate
internal tooling scenario requires adding triggers or rules to them.

There could be other cases for allow_system_table_mods to allow
modifying system catalog for emergency repair of catalog table or some
upgrade scripts used by extensions to allow adding additional entries
in catalog tables like pg_proc but I don't see such maintenance
required for conflict log tables. So, it seems better to block all the
additional operations discussed.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 10 Jun 2026 15:07:27 +0530

> additional operations discussed.

+1. irrespective of GUC value, we can block such operations. Some
operations are allowed even with this GUC set to false (as stated in
my previous emails).

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 11 Jun 2026 09:42:32 +0530

> operations are allowed even with this GUC set to false (as stated in

Yeah that makes sense. Here is the POC patch to block such operations
for the conflict log table. Please let me know your thoughts

-- 
Regards,
Dilip Kumar
Google
From 33bcdda8b0b84bab60282403f6eb3c2c930c7e53 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <[email-masked]>
Date: Wed, 10 Jun 2026 16:52:46 +0530
Subject: [PATCH v1] Report error for ddls on conflict log tables

---
 src/backend/commands/lockcmds.c     | 19 ++++++++
 src/backend/commands/policy.c       | 12 +++++
 src/backend/commands/statscmds.c    | 12 +++++
 src/backend/commands/tablecmds.c    | 73 +++++++++++++++++++++++++++++
 src/backend/commands/trigger.c      | 25 ++++++++++
 src/backend/rewrite/rewriteDefine.c | 22 +++++++++
 6 files changed, 163 insertions(+)

diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index f66b8f17b9b..de56dcc6d48 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -16,6 +16,7 @@
 
 #include "access/table.h"
 #include "access/xact.h"
+#include "catalog/catalog.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_inherits.h"
 #include "commands/lockcmds.h"
@@ -92,6 +93,17 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid,
 						rv->relname),
 				 errdetail_relkind_not_supported(relkind)));
 
+	/*
+	 * Conflict log tables are managed by the system for logical replication
+	 * and should not be locked explicitly.
+	 */
+	if (IsConflictLogTableNamespace(get_rel_namespace(relid)))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						rv->relname),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	/*
 	 * Make note if a temporary relation has been accessed in this
 	 * transaction.
@@ -198,6 +210,13 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
 				relkind != RELKIND_VIEW)
 				continue;
 
+			/*
+			 * Conflict log tables are managed by the system for logical
+			 * replication and should not be locked explicitly.
+			 */
+			if (IsConflictLogTableNamespace(get_rel_namespace(relid)))
+				continue;
+
 			/*
 			 * We might be dealing with a self-referential view.  If so, we
 			 * can just stop recursing, since we already locked it.
diff --git a/src/backend/commands/policy.c b/src/backend/commands/policy.c
index 21b8eebe32d..9919a7e4124 100644
--- a/src/backend/commands/policy.c
+++ b/src/backend/commands/policy.c
@@ -79,6 +79,18 @@ RangeVarCallbackForPolicy(const RangeVar *rv, Oid relid, Oid oldrelid,
 	if (!object_ownercheck(RelationRelationId, relid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not be modified directly, as it could disrupt
+	 * conflict logging.
+	 */
+	if (IsConflictLogTableClass(classform))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						rv->relname),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	/* No system table modifications unless explicitly allowed. */
 	if (!allowSystemTableMods && IsSystemClass(relid, classform))
 		ereport(ERROR,
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index b354723be44..18143a9cb76 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -147,6 +147,18 @@ CreateStatistics(CreateStatsStmt *stmt, bool check_rights)
 			aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind),
 						   RelationGetRelationName(rel));
 
+		/*
+		 * Conflict log tables are used internally for logical replication
+		 * conflict logging and should not have extended statistics, as it
+		 * could disrupt conflict logging.
+		 */
+		if (IsConflictLogTableClass(rel->rd_rel))
+			ereport(ERROR,
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission denied: \"%s\" is a conflict log table",
+							RelationGetRelationName(rel)),
+					 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 		/* Creating statistics on system catalogs is not allowed */
 		if (!allowSystemTableMods && IsSystemRelation(rel))
 			ereport(ERROR,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6ffd21c68c4..d8754aa93db 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2750,6 +2750,18 @@ MergeAttributes(List *columns, const List *supers, char relpersistence,
 					 errmsg("cannot inherit from partition \"%s\"",
 							RelationGetRelationName(relation))));
 
+		/*
+		 * Conflict log tables are managed by the system for logical
+		 * replication and should not be used as parent tables, as
+		 * inheritance could interfere with the logging behavior.
+		 */
+		if (IsConflictLogTableNamespace(relation->rd_rel->relnamespace))
+			ereport(ERROR,
+					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+					 errmsg("cannot inherit from conflict log table \"%s\"",
+							RelationGetRelationName(relation)),
+					 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 		if (relation->rd_rel->relkind != RELKIND_RELATION &&
 			relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
 			relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
@@ -3887,6 +3899,19 @@ renameatt_check(Oid myrelid, Form_pg_class classform, bool recursing)
 	if (!object_ownercheck(RelationRelationId, myrelid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(myrelid)),
 					   NameStr(classform->relname));
+
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not be modified directly, as it could disrupt
+	 * conflict logging.
+	 */
+	if (IsConflictLogTableClass(classform))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						NameStr(classform->relname)),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemClass(myrelid, classform))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -6889,6 +6914,18 @@ ATSimplePermissions(AlterTableType cmdtype, Relation rel, int allowed_targets)
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(rel->rd_rel->relkind),
 					   RelationGetRelationName(rel));
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not be altered directly, as it could disrupt conflict
+	 * logging.
+	 */
+	if (IsConflictLogTableClass(rel->rd_rel))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						RelationGetRelationName(rel)),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -10198,6 +10235,18 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 				 errmsg("referenced relation \"%s\" is not a table",
 						RelationGetRelationName(pkrel))));
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not be referenced by foreign keys, as it could
+	 * disrupt conflict logging.
+	 */
+	if (IsConflictLogTableClass(pkrel->rd_rel))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						RelationGetRelationName(pkrel)),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemRelation(pkrel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -19818,6 +19867,18 @@ RangeVarCallbackOwnsRelation(const RangeVar *relation,
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relId)),
 					   relation->relname);
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not be modified directly, as it could disrupt
+	 * conflict logging.
+	 */
+	if (IsConflictLogTableClass((Form_pg_class) GETSTRUCT(tuple)))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						relation->relname),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods &&
 		IsSystemClass(relId, (Form_pg_class) GETSTRUCT(tuple)))
 		ereport(ERROR,
@@ -19853,6 +19914,18 @@ RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid, Oid oldrelid,
 	if (!object_ownercheck(RelationRelationId, relid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not be altered directly, as it could disrupt conflict
+	 * logging.
+	 */
+	if (IsConflictLogTableClass(classform))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						rv->relname),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	/* No system table modifications unless explicitly allowed. */
 	if (!allowSystemTableMods && IsSystemClass(relid, classform))
 		ereport(ERROR,
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index b87b4b40d07..5fce1cf4d48 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -314,6 +314,18 @@ CreateTriggerFiringOn(const CreateTrigStmt *stmt, const char *queryString,
 						RelationGetRelationName(rel)),
 				 errdetail_relkind_not_supported(rel->rd_rel->relkind)));
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not have triggers, as it could disrupt conflict
+	 * logging.
+	 */
+	if (IsConflictLogTableClass(rel->rd_rel))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						RelationGetRelationName(rel)),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemRelation(rel))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -1443,6 +1455,19 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
 	/* you must own the table to rename one of its triggers */
 	if (!object_ownercheck(RelationRelationId, relid, GetUserId()))
 		aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
+
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not have triggers, as it could disrupt conflict
+	 * logging.
+	 */
+	if (IsConflictLogTableClass(form))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						rv->relname),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemClass(relid, form))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c
index 6a223fbeaa4..343d74cc72e 100644
--- a/src/backend/rewrite/rewriteDefine.c
+++ b/src/backend/rewrite/rewriteDefine.c
@@ -262,6 +262,17 @@ DefineQueryRewrite(const char *rulename,
 						RelationGetRelationName(event_relation)),
 				 errdetail_relkind_not_supported(event_relation->rd_rel->relkind)));
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not have rules, as it could disrupt conflict logging.
+	 */
+	if (IsConflictLogTableClass(event_relation->rd_rel))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						RelationGetRelationName(event_relation)),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemRelation(event_relation))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -772,6 +783,17 @@ RangeVarCallbackForRenameRule(const RangeVar *rv, Oid relid, Oid oldrelid,
 				 errmsg("relation \"%s\" cannot have rules", rv->relname),
 				 errdetail_relkind_not_supported(form->relkind)));
 
+	/*
+	 * Conflict log tables are used internally for logical replication conflict
+	 * logging and should not have rules, as it could disrupt conflict logging.
+	 */
+	if (IsConflictLogTableClass(form))
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied: \"%s\" is a conflict log table",
+						rv->relname),
+				 errdetail("Conflict log tables are managed by the system for logical replication.")));
+
 	if (!allowSystemTableMods && IsSystemClass(relid, form))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-- 
2.49.0

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 11 Jun 2026 10:43:51 +0530

> for the conflict log table. Please let me know your thoughts

Please find the rebased patch
1. It includes the new 0005 patch for reporting errors for DDLs on clt.

Open comments:
1. Recent comments from Nisha and Shveta after v47 are still open
2. Vignesh's patch for "describe related" changes needs a rebase. Can
you do that, Vignesh? Meanwhile, I will close all the open comments
and try to share a new version by EOD today.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Thu, 11 Jun 2026 16:04:13 +0530

> and try to share a new version by EOD today.

Thanks. A few comments:

1)
If I rename pg_conflict schema; apply worker handles it well as I do
not see any issue, but pg_dump starts giving problem:.

 ./pg_dump -d postgres  -p 5434 > sub.log
pg_dump: error: query failed: ERROR:  permission denied:
"pg_conflict_log_16390" is a conflict log table
DETAIL:  Conflict log tables are managed by the system for logical replication.
pg_dump: detail: Query was: LOCK TABLE public.tab1,
conflict_new.pg_conflict_log_16390 IN ACCESS SHARE MODE

Can dependency on name be removed even from pg_dump to avoid above issue?

2)
+ /*
+ errdetail("Conflict log tables are managed by the system for logical
replication.")));

The comment is too generic. Suggestion:

/*
 * Disallow extended statistics on conflict log tables. These are
 * internal, system-managed tables with no indexes, making
 * statistics useless for planning while adding unnecessary
 * catalog dependency overhead.
 */

3)
If we want to keep one generic DETAIL msg, shall it be changed to:

Conflict log tables are system-managed tables for logical replication conflicts.
~~

Will review more tomorrow.

thanks
Shveta

---

From: vignesh C <[email-masked]>
Date: Thu, 11 Jun 2026 17:52:50 +0530

> and try to share a new version by EOD today.

Here is the rebased version of the patch attached.

Regards,
Vignesh

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 13 Jun 2026 15:46:40 +0530

> Here is the rebased version of the patch attached.

Please find attached the latest patch. I have reordered the series,
moving 0006 to 0002, and updated the lock restrictions. We now allow
ACCESS SHARE mode exclusively to ensure pg_dump can acquire its
necessary locks, while blocking higher-level locks to prevent
interference with insertions into the conflict log tables.

-- 
Regards,
Dilip Kumar
Google

---

From: Amit Kapila <[email-masked]>
Date: Sat, 13 Jun 2026 16:57:54 +0530

+ /*
+ * Conflict log tables are managed by the system for logical replication
+ * conflicts and should not be locked explicitly. However, AccessShareLock
+ * is allowed to support pg_dump, which must lock tables to prevent them
+ * from being dropped or altered between fetching the table list and
+ * performing the dump. This read-only lock is safe because it does not
+ * interfere with concurrent insertions into the conflict log tables.
+ */
+ if (IsConflictLogTableNamespace(get_rel_namespace(relid)) &&
+ lockmode > AccessShareLock)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a conflict log table",
+ rv->relname),
+ errdetail("Conflict log tables are system-managed and cannot be
locked explicitly, except in ACCESS SHARE mode.")));

In favor of keeping this code simple, can we allow locking the CLT
table with the following comment: "Note: Conflict log tables are
deliberately NOT blocked here, even though other direct DDL on them is
rejected elsewhere. pg_dump relies on being able to take an ACCESS
SHARE lock on these tables to safely dump their definitions during
binary upgrade, so we permit LOCK so we allow LOCK on them and treat
them like ordinary tables here.
It's true that a strong lock (ShareLock or above) on such a table
would conflict with the RowExclusiveLock taken by the apply worker's
insertsand could stall conflict logging for as long as it is held.
But locking a system-managed conflict log table is an unusual thing to
do, and it doesn't seem worth the trouble of filtering by lock mode
here."

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Mon, 15 Jun 2026 09:27:52 +0530

> insertsand could stall conflict logging for as long as it is held.

A correction: It would not only stall conflict logging but would also
stall the apply worker (logical replication), or is there a default
lock timeout that I am not aware of? If there's no such timeout, we
can correct the comment slightly.

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Mon, 15 Jun 2026 15:04:44 +1000

Some review comments for v50-0002.

======
Commit message

1.
Missing commit message.

======
GENERAL

2.
+ if (IsConflictLogTableClass(rel->rd_rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a conflict log table",
+ RelationGetRelationName(rel)),
+ errdetail("Conflict log tables are system-managed tables for logical
replication conflicts.")));

2a.
There are many duplicate "permission denied" messages in this patch,
like above. Is it possible to do something like introduce an
OBJECT_SUBSCRIPTION_CLT so you can delegate all these to common
aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_SUBSCRIPTION_CLT, clt_name)
error processing?

Or, if that's a bit too hacky, put a common error function in catalog.c.

e.g.
if (IsConflictLogTableClass(rel->rd_rel))
  ConflictLogTablePermissionDenied(rel);

~

2b.
I'm not sure that the errdetail is really needed, since none of the
nearby messages have extra errdetail. Instead, you could just modify
the errmsg like:
"permission denied: \"%s\" is a system-managed conflict log table".

======
src/backend/commands/lockcmds.c

3.

Is ERRCODE_INSUFFICIENT_PRIVILEGE/"permission denied" the correct
errcode/errmsg to use here. e.g.,  is lack of privilege really the
problem given it was not supposed to be locked by *anybody*?

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

---

From: vignesh C <[email-masked]>
Date: Mon, 15 Jun 2026 10:46:07 +0530

> interference with insertions into the conflict log tables.

I noticed that declaring a cursor with 'FOR UPDATE' on a conflict log
table currently fails:
postgres=*# DECLARE cur1 CURSOR FOR
    SELECT *
    FROM pg_conflict.pg_conflict_log_16404
    WHERE relid = 16402
    FOR UPDATE;
ERROR:  cannot lock rows in the conflict log table "pg_conflict_log_16404"

I'm not sure whether this restriction is intentional.

One benefit of supporting 'FOR UPDATE' cursors is that they provide
protection against concurrent modifications. For example:
**Session 1**
BEGIN;
DECLARE cur1 CURSOR FOR SELECT * FROM t3 WHERE c1 < 100 FOR UPDATE;
FETCH NEXT FROM cur1;

**Session 2**
DELETE FROM t3 WHERE c1 < 100;

In this case, the 'DELETE' in Session 2 will wait until Session 1
releases the row locks, preventing concurrent modification of the rows
being processed.

This can be useful for workflows that need to archive rows before
deleting them. For example:
DO $$
DECLARE
    r t3%ROWTYPE;
    cur1 CURSOR FOR SELECT * FROM t3 WHERE c1 < 100 FOR UPDATE;
BEGIN
    OPEN cur1
    LOOP
        FETCH cur1 INTO r;
        EXIT WHEN NOT FOUND;

        INSERT INTO t3_archive  VALUES (r.c1);

        DELETE FROM t3 WHERE CURRENT OF cur1;
    END LOOP;

    CLOSE cur1;
END $$;

Given these use cases, should 'FOR UPDATE' be allowed on conflict log
tables, or is the current behavior intentional for some reason that
I'm overlooking?

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 15 Jun 2026 10:51:47 +0530

> interference with insertions into the conflict log tables.
Hi Dilip and Vignesh,

I checked the Cfbot and noticed that one of the 'CompilerWarnings' [1]
flow is failing with:
describe.c: In function 'describeSubscriptions':
describe.c:7456:29: error: 'logdest' may be used uninitialized
[-Werror=maybe-uninitialized]
7456 | if (strcmp(logdest, "table") == 0 || strcmp(logdest, "all") == 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~
describe.c:7356:29: note: 'logdest' was declared here
7356 | char *logdest;
| ^~~~~~~
cc1: all warnings being treated as errors

[1]: https://github.com/postgresql-cfbot/postgresql/actions/runs/27464417110/job/81184175578

Thanks,
Shlok Kyal

---

From: shveta malik <[email-masked]>
Date: Mon, 15 Jun 2026 11:27:34 +0530

A few comments on v50:

1)
+ /*

Suggestion:
/*
 * Conflict log tables are system-managed tables used internally for
 * logical replication conflict logging. Similar to indexes, user-defined
 * extended statistics are not supported on these tables at present.
 */

2)
Assertion hit in alter_sub_conflict_log_dest():

set allow_system_table_mods=on
create subscription sub1 connection '...' publication pub1
WITH(conflict_log_destination='log');
select oid from pg_subscription;

--CREATE clt USING SAME oid
create table pg_conflict.pg_conflict_log_16501(i int);

--change destination to table/all
alter subscription sub1 SET (CONFLICT_LOG_DESTINATION = 'ALL');

--this asserts here:
2026-06-15 11:10:40.540 IST [10626] LOG:  background worker "logical
replication tablesync worker" (PID 11397) exited with exit code 1
TRAP: failed Assert("IsBinaryUpgrade"), File: "subscriptioncmds.c",
Line: 1554, PID: 10662

alter_sub_conflict_log_dest(): Assert(IsBinaryUpgrade);

Should we fix Assert or restrict table creation in pg_conflict scehma?
We allow it for toast-schema though when allow_system_table_mods=ON.
But I don't see a use case for allowing this.

3)
I'm not sure whether the comments (at [1]) regarding the TRY-CATCH
block were considered or perhaps overlooked. Please ignore this if you
are already taking them into account.

Doc-patch:
<caught my eye while checking something, not reviewing the doc-patch
rigth now though>

4)

Second pg_catalog should be replaced with pg_conflict

5)

system managed or system-managed, we can use same at both the places.

~~

[1]: https://www.postgresql.org/message-id/CAJpy0uBSY7zTH%3D4TvAOS%3Dkj9vivBUc9NO%2BVp6KNw-Na9RiAsMg%40mail.gmail.com

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Mon, 15 Jun 2026 16:06:17 +1000

A trivial review comment for v50-0003.

======

1.
+void
+ProcessPendingConflictLogTuple(void)
+{
+ Relation conflictlogrel;
+ ConflictLogDest dest;
+
+ /* Nothing to do */
+ if (MyLogicalRepWorker->conflict_log_tuple == NULL)
+ return;
+
+ PG_TRY();
+ {

Those 2 variables are only used within the PG_TRY block, so they can
be declared there.

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

---

From: vignesh C <[email-masked]>
Date: Mon, 15 Jun 2026 11:56:53 +0530

> interference with insertions into the conflict log tables.

While reviewing operations on the pg_conflict schema, I noticed a few
behaviors that I wasn't sure were intentional.
1. REINDEX is allowed on conflict log tables
postgres=# REINDEX TABLE pg_conflict.pg_conflict_log_16404;
REINDEX

I was not sure whether allowing REINDEX on conflict log tables is
intentional, given that these are system-managed tables.

2. Views are disallowed, but functions are allowed
Creating a view in the pg_conflict schema is rejected:
postgres=# CREATE VIEW v1 AS
SELECT * FROM pg_conflict.pg_conflict_log_16435;
ERROR:  permission denied to create "pg_conflict.v1"
DETAIL:  Conflict schema modifications are currently disallowed.

However, creating a function in the same schema succeeds:
CREATE FUNCTION pg_conflict.get_conflict_count()
RETURNS bigint
LANGUAGE sql
AS $$
    SELECT count(*) FROM pg_conflict.pg_conflict_log_16404;
$$;
CREATE FUNCTION

I noticed similar behavior with the pg_toast schema as well, where
function creation is allowed:

CREATE FUNCTION pg_toast.get_toast_1213_count()
RETURNS bigint
LANGUAGE sql
AS $$
    SELECT count(*) FROM pg_toast.pg_toast_1213;
$$;
CREATE FUNCTION

I am not sure what the rationale is for permitting some object types
while rejecting others.

3. Functions can be created, but triggers cannot:
Although function creation succeeds, trigger creation on a conflict
log table is rejected:
CREATE TRIGGER conflict_audit
AFTER INSERT
ON pg_conflict.pg_conflict_log_16435
FOR EACH ROW
EXECUTE FUNCTION get_conflict_count();
ERROR:  permission denied: "pg_conflict_log_16435" is a conflict log table
DETAIL:  Conflict log tables are system-managed tables for logical
replication conflicts.

Again, I wasn't sure whether this distinction is intentional.

4. User-defined types and domains are allowed
CREATE TYPE pg_conflict.conflict_status AS ENUM ('ACTIVE', 'INACTIVE');
CREATE TYPE

CREATE DOMAIN pg_conflict.positive_int
AS integer
CHECK (VALUE > 0);
CREATE DOMAIN

I was also surprised that creating types and domains is permitted in
the pg_conflict schema.

Overall, I am having some difficulty understanding the rules governing
which operations are allowed and which are disallowed in the
pg_conflict namespace. If these behaviors are intentional, would it
make sense to document the supported and unsupported operations more
clearly? That would help avoid confusion for users.

Regards,
Vignesh

---

From: Amit Kapila <[email-masked]>
Date: Mon, 15 Jun 2026 14:20:26 +0530

> intentional, given that these are system-managed tables.

I think this should be disallowed.

>     SELECT count(*) FROM pg_conflict.pg_conflict_log_16404;

This is okay because the function is doing SELECT which we allow on
these tables.

> Again, I wasn't sure whether this distinction is intentional.

I have given the reason for functions above and this restriction is okay.

> clearly? That would help avoid confusion for users.

Ideally, we should allow only SELECT, DELETE, and TRUNCATE for
allowing users to view and maintain the CLT tables. We are also
planning to allow the LOCK command for the purpose of pg_dump.

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 15 Jun 2026 18:58:50 +1000

Some review comments for v50-0004.

======

dumpSubscription:

1.
+ * We skip the default value ('log') to match the handling of other
+ * default subscription options.
+ */

There is only one "default", so I think this should say "...other
subscription options", not "...other default subscription options".

======

2.
+#define CONFLICT_LOG_RELATION_NAME_FMT "pg_conflict_log_%u"
+

2a.
This format string (and first usage of it) really be in one of the
earlier patches.

~

2b.
Add a comment to explain that the %u is the subscription oid.

~

2c.
Is this the correct header file for this? IMO it is CLT specific, so
belongs in conflict.h.

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

---

From: Amit Kapila <[email-masked]>
Date: Mon, 15 Jun 2026 14:29:41 +0530

> tables, or is the current behavior intentional for some reason that

As told in the previous email, we are planning to allow only a minimal
set of commands initially that are necessary for observability and
maintainability of CLT tables. I think such use cases could be
achieved by LOCK command or the user needs to be careful not to
remove/truncate data from these tables till the same is required.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Mon, 15 Jun 2026 14:32:09 +0530

Sharing my thoughts:

+1

I think REINDEX is harmless for CLT as curently there are no indexes,
so it should be okay to let it be. For toast-tables too, we have
simialr behaviour. We restrict index creation while REINDEX is
allowed:

postgres=# show allow_system_table_mods;
 allow_system_table_mods
-------------------------
 on
(1 row)

postgres=# create index on pg_toast.pg_toast_826 (chunk_id);
ERROR:  cannot create index on relation "pg_toast_826"
DETAIL:  This operation is not supported for TOAST tables.

postgres=# REINDEX TABLE pg_toast.pg_toast_826;
REINDEX

> > DETAIL:  Conflict schema modifications are currently disallowed.

Okay, I can create views outside the pg_conflict schema for CLT
tables, but creation fails inside pg_conflict irrespective of
'allow_system_table_mods '.

postgres=#  CREATE VIEW public.v1 AS SELECT * FROM
pg_conflict.pg_conflict_log_24576;
CREATE VIEW

postgres=#  CREATE VIEW pg_conflict.v1 AS SELECT * FROM
pg_conflict.pg_conflict_log_24576;
ERROR:  permission denied to create "pg_conflict.v1"
DETAIL:  Conflict schema modifications are currently disallowed.

While for toast  table, behaviour is slightly different.

With allow_system_table_mods = off

postgres=# CREATE VIEW public.v2 AS select * from pg_toast.pg_toast_826;
CREATE VIEW

postgres=# CREATE VIEW pg_toast.v2 AS select * from pg_toast.pg_toast_826;
ERROR:  permission denied to create "pg_toast.v2"
DETAIL:  System catalog modifications are currently disallowed.

With allow_system_table_mods = on, view creation inside TOAST schema works:
postgres=# CREATE VIEW pg_toast.v2 AS select * from pg_toast.pg_toast_826;
CREATE VIEW

IMO, creating views can be allowed on CLT outside of CLT schema as it
can help DBAs to analyze the data better. For creating views inside
the CLT schema, we currently allow table creation (see my last email)
in the CLT schema as well. IMO, we can either disallow both or allow
both. The behaviour should be consistent.

+1

> I have given the reason for functions above and this restriction is okay.

Functions are allowed while triggers are not. IMO, it is fine. Using
triggers, one can simply skip or perform other actions for conflict
insertions in CLT which should be strictly avoided.

> planning to allow the LOCK command for the purpose of pg_dump.

Again, returning to my previous statement: table creations, view
creations, and type creations—either all should be allowed or none. I
do not see a strong use case for allowing tables inside the
pg_conflict schema. But since we allow these things inside other
system schemas, we can allow it here as well.  I don't have a strong
case for or against these.

postgres=# create table pg_catalog.pg_conflict_log_16502(i int);
CREATE TABLE
postgres=# create view pg_catalog.v1 as select * from pg_conflict_log_16502
postgres-# ;
CREATE VIEW
postgres=#  CREATE TYPE pg_catalog.conflict_status AS ENUM ('ACTIVE',
'INACTIVE');
CREATE TYPE

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Mon, 15 Jun 2026 14:36:09 +0530

> Those 2 variables are only used within the PG_TRY block, so they can

I request you to please collect all such code-level comments and share
as one email. I know it is not always possible and sometimes it is
good to share whatever one has reviewed. However, sometimes, I get the
feeling that I may overlook some other key design related comment by
someone which needs more discussion. I understand that I am no one to
tell how you or someone should share her/his review but this is just a
humble request and if you don't like it, feel free to ignore it.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 15 Jun 2026 16:24:24 +0530

> tables, or is the current behavior intentional for some reason that

IIUC maintaining the conflict log table is the user's responsibility.
Therefore, we don't need to overengineer use cases where a user
concurrently backs up and deletes the table. Furthermore, since only
the subscription owner can delete data from it, it is ultimately their
responsibility to ensure they do not delete rows they intend to copy.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 15 Jun 2026 16:45:38 +0530

>  * extended statistics are not supported on these tables at present.

I think the reason for index are different so here is my proposal for this

        /*
         * Conflict log tables are system-managed tables used internally for
         * logical replication conflict logging. Unlike user tables, they are
         * not expected to have complex query usage, so to keep things simple,
         * user-defined extended statistics are not required or supported at
         * present.
         */

> But I don't see a use case for allowing this.

I think this is in 0004 patch, because during upgrade we would have an
existing table created via schema dump, and hence we need to associate
this table to the subscription.  But I am surprise to see that
changinng destination from 'log' to 'all' is going in that path where
table already exists, Vignesh can you debug this change, I think it
got introduced in 0004

> are already taking them into account.

Yeah all 0003 review commants are not yet fixed, there are few
comments from you as well as from Nisha, will fix in next version

> [1]: https://www.postgresql.org/message-id/CAJpy0uBSY7zTH%3D4TvAOS%3Dkj9vivBUc9NO%2BVp6KNw-Na9RiAsMg%40mail.gmail.com

I am making a note so that we can do all doc changes later.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 15 Jun 2026 17:26:48 +0530

>   ConflictLogTablePermissionDenied(rel);

I do not really think we need a separate function or a new error code,
but I am open for what other thinks so for now I am not fixing it.

> src/backend/commands/lockcmds.c

Yeah we can remove errdetails considering the other nearby permission
check code, provided no one else objects.

> problem given it was not supposed to be locked by *anybody*?

Based on our previous discussion, we are now allowing locking of the
conflict log tables. Therefore, this code block will be removed in
upcoming version and a note will be added to explain why this is
permitted.

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Mon, 15 Jun 2026 18:25:33 +0530

>          * user-defined extended statistics are not required or supported at

Works for me.

> table already exists,

This is because we compute want_table and has_oldtable purely based on
old and new settings (not looking at table existence actually), so the
old setting is !has_oldtable and new is want_table, thus hitting the
Assert. IMO, it should be ERROR (similar to create_conflict_log_table)
rather than Assert if the table exists and the mode is not
BinaryUpgrade.

Thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Tue, 16 Jun 2026 09:49:10 +1000

> different places to be added here.

Sure, it's not "required", but I think:
A) Separating the footer code from the non-footer code makes it easier to read
B) The 'describeSubscriptions' function is too long. This would make
it 20 lines shorter.
C) Consistent footer handling for pub/sub describes.

//////

More review comments for v50-0005

======
src/bin/psql/describe.c

1.

The CLT is targeting PG20, right? So, that comment ought to say "is
supported in v20 and higher".

Ideally, there should be some "TODO" reminder comments here to ensure
the appropriate 190000's get replaced by 200000 as soon as the version
number is bumped. Better to flag/comment all those places now, so that
nothing gets missed later.

(A similar review comment probably applies also to the pg_dump changes
in the previous v50-0004 patch).

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

---

From: vignesh C <[email-masked]>
Date: Tue, 16 Jun 2026 09:27:19 +0530

> This is okay because the function is doing SELECT which we allow on

Shouldn’t creating a view also be allowed for the same reason?

The function is permitted because it only performs a SELECT on the
conflict log table, and such read operations are allowed. A view
likewise represents a stored query definition and does not modify the
conflict schema or its underlying tables. Given that direct queries on
the conflict log tables are already permitted, allowing users to
define views over them is consistent with the same rationale applied
to functions.

Regards,
Vignesh

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 16 Jun 2026 09:55:55 +0530

> define views over them is consistent with the same rationale applied

Whats is the behavior of toast table w.r.t the function vs view?

-- 
Regards,
Dilip Kumar
Google

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jun 2026 09:56:04 +0530

> define views over them is consistent with the same rationale applied

I think Function creation and view creation should be allowed.

a) Views are purely logical wrappers used to inspect data. Allowing
them causes no harm and provides a benefit to DBAs, who can use them
to customize how they monitor CLT data.

b) IMO, allowing function creation is safe because we internally
enforce table-level restrictions at execution time. If a user defines
a function that only performs a 'select', it will run cleanly. But if
a function attempts a restricted operation such as an INSERT, UPDATE;
our existing checks/restrictions will naturally intercept the
execution and fail with the appropriate error.

The question is whether we should allow users to create these inside
the CLT schema or only in public/other schema?

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jun 2026 09:59:32 +0530

> Whats is the behavior of toast table w.r.t the function vs view?

Please see [1] for details.

[1]: https://www.postgresql.org/message-id/CAJpy0uDoa0CYWkxj52h%3DRM53acfsqjRihCfKrm8W%3DvRvHg01UA%40mail.gmail.com

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Tue, 16 Jun 2026 10:20:52 +0530

> Whats is the behavior of toast table w.r.t the function vs view?

I think sticking to the minimum set of commands that are required for
the user to monitor/maintain CLT should be a good goal for the first
version. We can evaluate to allow other commands based on real user
need and its impact on the functionality. So, it is okay to block even
the view on CLT for now.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 16 Jun 2026 10:30:38 +0530

> need and its impact on the functionality. So, it is okay to block even

A view is essentially just a stored SELECT query, I think it should be
allowed in the first version itself. Rest (functions, indexes,
statistics etc) can be decided and implemented later based on user's
feedback.

thanks
Shveta

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 16 Jun 2026 15:13:24 +0530

> statistics etc) can be decided and implemented later based on user's

IMHI, while we could allow view creation, it is unlikely users will
perform operations critical enough to require views or stored
procedures on the CLT. For the initial version, providing SELECT,
DELETE, and TRUNCATE permissions should be sufficient. We can
introduce support for views and user- or system-created indexes in
subsequent releases.

-- 
Regards,
Dilip Kumar
Google

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 16 Jun 2026 16:11:55 +0530

> define views over them is consistent with the same rationale applied

o ensure we are aligned, I want to clarify the current behavior before
we decide on the next steps. Currently, view and function creation are
both permitted for toast and conflict tables. However, there is a
discrepancy in the schemas: view creation is blocked in the
'pg_conflict' and 'pg_toast' schemas, while function creation is
allowed in both.  So IIUC we need to take a call whether the function
creation should be blocked in pg_conflict schema or not, logically we
can say it can be blocked but we might need to discuss on this because
it's allowed in the `pg_toast` schema.  And if we are worried about
the descripency that functions are allowed in this schema but views
are not, I don't think we need to worry about this, this is an
existing behaviour for toast also and if we want to can investigate as
a separate thread and fix it for system schema al together?  Am I
missing something here?

postgres[2205601]=# CREATE VIEW v2 AS select * from pg_toast.pg_toast_16412;
CREATE VIEW

postgres[2205601]=# CREATE VIEW v3 AS select * from
pg_conflict.pg_conflict_log_16406;
CREATE VIEW

CREATE VIEW pg_toast.v5 AS select * from pg_toast.pg_toast_16412;
ERROR:  42501: permission denied to create "pg_toast.v5"
DETAIL:  System catalog modifications are currently disallowed.
LOCATION:  heap_create, heap.c:322

postgres[2205601]=# CREATE VIEW pg_conflict.v5 AS select * from
pg_conflict.pg_conflict_log_16406;
ERROR:  42501: permission denied to create "pg_conflict.v5"
DETAIL:  Conflict schema modifications are currently disallowed.
LOCATION:  heap_create, heap.c:329

postgres[2205601]=# CREATE FUNCTION pg_conflict.get_conflict_count()
RETURNS bigint
LANGUAGE sql
AS $$
SELECT count(*) FROM pg_conflict.pg_conflict_log_16406;
$$;
CREATE FUNCTION

postgres[2205601]=# CREATE FUNCTION pg_toast.get_toast_count()
RETURNS bigint
LANGUAGE sql
AS $$
SELECT count(*) FROM pg_toast.pg_toast_16412;
$$;
CREATE FUNCTION

-- 
Regards,
Dilip Kumar
Google