Re: Proposal: Conflict log history table for Logical Replication

On Mon, Jul 6, 2026 at 5:10 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Sat, Jul 4, 2026 at 11:07 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Fri, Jul 3, 2026 at 11:03 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > The wording of the comments above is extremely repetitive. That may
> > > not be noteworthy if the comment was just in one place, but it's
> > > everywhere.
> > >
> > > How about like:
> > >
> > > BEFORE
> > > Conflict log tables are used internally for logical replication
> > > conflict logging and should not have <xxx>, as it could disrupt
> > > conflict logging.
> > >
> > > SUGGESTION
> > > Conflict log tables are internal to logical replication and must not
> > > have <xxx>, since it could interfere with their operation.
> > >
> >
> > I agree with most of Amit's comments. Regarding this one, I really
> > don't think the suggested comment adds much value over what we
> > currently have. However, if anyone feels this must be changed, please
> > let me know, I will provide a patch.
> >
>
> Of course, changing or not changing is your call, but just so you
> know, whenever I post suggestions for rewording docs or code comments,
> those are always first validated using AI language tools. If I suggest
> one phrasing is better than another it is not purely my personal
> opinion
>

Fair enough. But note that many-a-times AI's judgement is not correct.
For example, here your original complaint was that the comment was
repetitive and your suggested wording won't solve that complaint;
rather it is a somewhat tighter version of the existing comment
without adding any new information to it. As the errmsg+errdetail
actually conveys what the comment wants to say in most except the one
in ATSimplePermissions(). So, we could remove the comments from all
places except the one in ATSimplePermissions() and keep a generic
comment atop IsConflictLogTableNamespace/IsConflictLogTableClass on
the following lines:

/*
* IsConflictLogTableNamespace
* True iff namespace is pg_conflict.
*
* Does not perform any catalog accesses.
*
* Tables in the pg_conflict namespace are conflict log tables, created and
* maintained by the system to record logical replication conflicts. The
* apply worker's inserts depend on their fixed structure, so they are treated
* as system-managed: call sites across the backend use this (and
* IsConflictLogTableClass()) to reject operations that could disrupt logging,
* such as DDL, manual INSERT/UPDATE/MERGE, and row locking. DELETE and
* TRUNCATE are the only data operations allowed, so that users can still
* prune old conflict rows.
*/

/*
* IsConflictLogTableClass
* True iff the relation is a conflict log table.
*
* Does not perform any catalog accesses. See
* IsConflictLogTableNamespace() for why operations on
these tables are
* restricted.
*/

--
With Regards,
Amit Kapila.