Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jun 30, 2026 at 11:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jun 30, 2026 at 9:22 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > ======
> > > src/backend/commands/tablecmds.c
> > >
> > > 2.
> > > + /*
> > > + * 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_INSUFFICIENT_PRIVILEGE),
> > > + errmsg("permission denied: cannot inherit from conflict log table \"%s\"",
> > > + RelationGetRelationName(relation)),
> > > + errdetail("Conflict log tables are system-managed tables for logical
> > > replication conflicts.")));
> > >
> > > It is not really a lack of "privilege" preventing this -- it is just
> > > disallowed for everybody. Would ERRCODE_WRONG_OBJECT_TYPE be a more
> > > appropriate errcode?
> >
> > I agree that we currently restrict most operations on CLTs, but the
> > issue isn't that the CLT is the wrong object type, they are still
> > relations. While we might open up certain operations in the future
> > based on business needs (such as allowing index creation), I still
> > believe this is a matter of privileges rather than object type.
> >
>
> If we look from this POV where we want to support some features in
> future, ERRCODE_FEATURE_NOT_SUPPORTED sounds like a better option.
> OTOH, in execMain.c, in 0001, WRONG_OBJECT_TYPE is any way a better
> fit because we never want to allow INSERT/UPDATE/MERGE. Having, single
> error_code makes it easy to understand/explain/extend this feature,
> otherwise, the future users will always be confused as to which
> error_code is better fit. I think insuffcient_privilige doesn't suit
> even if we want to support such features in future because those won't
> require a new grant kind of privilege. Even though WRONG_OBJECT_TYPE
> is slightly over stretched for some cases but I think consistency
> helps, so, I would prefer WRONG_OBJECT_TYPE in the case Peter pointed
> out and other DDL operations where we have a similar check.
>

I have changed the error code to WRONG_OBJECT_TYPE in the attached
v59_amit_2.txt. The other thing that bothers me about this patch is
the usage of DO blocks in the tests to hide the CLT name (as the same
has OID in its name which is non-deterministic) which doesn't seem to
be the best way as that bloats the test code and make the test cases
difficult to read. I have changed those specific tests in the attached
v59_amit_3.txt by using \gset and \set VERBOSITY sqlstate which we
also use in arrays.sql and a few other places. Let me know what you
think of attached top-up patches. Also, now let's merge your 0001 and
0002 patches.

-- 
With Regards,
Amit Kapila.