Re: Proposal: Conflict log history table for Logical Replication

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

Regards,
Vignesh