Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jun 30, 2026 at 2:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Jun 30, 2026 at 11:02 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 30, 2026 at 9:22 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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.

I have merged these patches and also merged 0001 and 0002. Also fixed
other open comments from Nisha, Shlok, and Peter

--
Regards,
Dilip Kumar
Google