Re: Proposal: Conflict log history table for Logical Replication

On Thu, May 28, 2026 at 2:57 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?

I do not understand this change; my original patch 0001 has like this,
that mean we are only allowing ACL_TRUNCATE and ACL_DELETE for
conflict log table, whats the reason for changing the same in 0002?

if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
ACL_USAGE)) &&
- IsSystemClass(table_oid, classForm) &&
- classForm->relkind != RELKIND_VIEW &&
+ IsConflictClass(classForm) &&
!superuser_arg(roleid))
- mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);
+ mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_USAGE);
+ else if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE |
ACL_TRUNCATE | ACL_USAGE)) &&
+ IsSystemClass(table_oid, classForm) &&
+ classForm->relkind != RELKIND_VIEW &&
+ !superuser_arg(roleid))
+ mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);

--
Regards,
Dilip Kumar
Google