Re: Proposal: Conflict log history table for Logical Replication

On Wed, May 27, 2026 at 1:34 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 26 May 2026 at 15:08, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Mon, May 25, 2026 at 10:13 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > >
> > > Thanks for the comments, the attached v39 version patch has the
> > > changes for the same.
> > >
> >
> > I have not yet looked at v40, but please find a few ocmments on
> > v39-0001 and 0002 merged together.
> > 4)
> > Do we need to have CommandCounterIncrement() after
> > heap_create_with_catalog() in create_conflict_log_table()? I think
> > even if we are not doing any table_open etc for CLT in same
> > transaction, we should call CommandCounterIncrement() (to be
> > consistent with other such calls of heap_create_with_catalog and to
> > make it future proof). Thoughts?
>
> I felt this is not required as we are not doing a table open on the
> newly created table.
>

Okay, command counter increment would be required here if we further
access that relation in the same command. I think I am facing a
related problem w.r.t newly created subscription. After applying first
six patches, the create subscription fails as follows:
postgres=# create subscription sub1 connection 'dbname=postgres'
publication pub1 with (conflict_log_destination='all');
ERROR: dependent subscription was concurrently dropped

I debugged and found that we get the above ERROR when we are trying to
find the subscription which is not yet created. In this case, it seems
to be happening because we are using a subscription that is yet not
created for dependency recording. This raises a question as to why are
we creating the conflict_log_table before subscription, at least this
needs some comments.

*
+ if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
+ {
+ if (IsConflictLogTableClass(classForm))
+ {
+ /*
+ * For conflict log tables, allow non-superusers to perform
+ * DELETE and TRUNCATE for cleanup and maintenance. Also allow
+ * INSERT and UPDATE to pass ACL checks so that later checks
+ * can raise the dedicated "cannot modify or insert data into
+ * conflict log table" error instead of a generic permission
+ * denied error. Still restrict USAGE for non-superusers.
+ */
+ mask &= ~(ACL_USAGE);

I see the point of giving a specific error instead of a generic error
but this functionality is used by pg_class_aclmask() which is an
exposed function. If we go with your proposed change, isn't there a
risk that some extension or outside core-code using pg_class_aclmask()
won't invoke that later functionality (CheckValidResultRel())? If we
decide to go this way then we can change this comment as proposed in
the attached?

--
With Regards,
Amit Kapila.