Re: Proposal: Conflict log history table for Logical Replication

On Wed, 20 May 2026 at 15:05, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 19 May 2026 at 12:02, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > 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.
>
> 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.
>
> > > > ======
> > > > 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.
>
> I preferred the current way in this case.
>
> > ======
> > 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 :-)
>
> 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
>
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