On Wed, May 27, 2026 at 1:34 AM vignesh C <[email protected]> wrote:
>
> On Tue, 26 May 2026 at 15:08, shveta malik <[email protected]> wrote:
> >
> > On Mon, May 25, 2026 at 10:13 AM vignesh C <[email protected]> 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.
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index e583187c7a6..19c568f4705 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3347,9 +3347,9 @@ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode
mask,
* 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.
+ * in CheckValidResultRel() can raise the
specific error instead
+ * of a generic permission denied error. Still
restrict USAGE for
+ * non-superusers.
*/
mask &= ~(ACL_USAGE);
}
