Re: Proposal: Conflict log history table for Logical Replication

On Mon, May 18, 2026 at 10:35 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, 13 May 2026 at 11:43, Peter Smith <smithpb2250(at)gmail(dot)com> 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.

> > ======
> > src/backend/executor/execMain.c
> >
> > 11.
> > +
> > + /*
> > + * Conflict log tables are managed by the system to record logical
> > + * 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?

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,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot lock rows in conflict log table \"%s\"",

Should that say "in the"?

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

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

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