On Mon, Jun 1, 2026 at 4:36 AM Dilip Kumar <[email protected]> wrote: > > On Sun, May 31, 2026 at 5:38 PM vignesh C <[email protected]> wrote: > > > > On Sun, 31 May 2026 at 11:43, Dilip Kumar <[email protected]> wrote: > > > > > > On Thu, May 28, 2026 at 2:57 AM Amit Kapila <[email protected]> > > > wrote: > > > > > > > > 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? > > > > > > 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); > > > > This was done to fix Shveta's comments from [1] to throw "cannot > > modify or insert data into conflict log table" instead of a generic > > permission denied error for the owner of the conflict log table. > > [1] - > > https://www.postgresql.org/message-id/CAJpy0uANkzTyUjO2W0=RtaJCGg=VYcwLGGCpqax=zkjgnbb...@mail.gmail.com > > Thanks for pointing it, I will analyze this behavior and give my opinion.
While thinking more about this, wouldn't the behaviour is same as pg_toast table, I mean, the superuser will get "cannot change TOAST relation "pg_toast_16404" whereas the owner of the toast will get a permission denied error? -- Regards, Dilip Kumar Google
