Re: Proposal: Conflict log history table for Logical Replication

[Amit Kapila]

On Wed, May 6, 2026 at 3:06 PM shveta malik <shveta.ma...@gmail.com> wrote:
>
> As a non super-user which does not have 'pg_create_subscription' privelege:
> postgres=>  alter table pg_conflict.pg_conflict_16487 add column i int;
> ERROR:  permission denied for schema pg_conflict
> <seems correct, as access is denied at schema level itself>
>
> As a non super-user which has 'pg_create_subscription' privelege, but
> does not own the respective sub:
> postgres=> alter table pg_conflict.pg_conflict_16487 add column i int;
> ERROR:  must be owner of table pg_conflict_16487
> <Due to 'pg_create_subscription', it seems schema access is provided,
> so it goes to check table access now and gives above error. Not sure
> about this error, even if the user were the owner, they still wouldn't
> be able to perform this operation>
>
> As a non super-user which has 'pg_create_subscription' privilege and
> also owns the respective sub:
> postgres=> alter table pg_conflict.pg_conflict_16498 add column i int;
> ERROR:  permission denied: "pg_conflict_16498" is a system catalog
> <okay>
>
> As a super-user, the error is same irrespective of fact whether it
> actually owns that table or not:
> postgres=# alter table pg_conflict.pg_conflict_16487 add column i int;
> ERROR:  permission denied: "pg_conflict_16487" is a system catalog
> <okay>
>
> For second case, not a strong opinion, but can the better error be:
> ERROR:  permission denied: "pg_conflict_16487" is a system catalog?
>
> I have not analyzed code myself for this yet.
>

I analyzed this case and think that the current behavior is okay. As
per RangeVarCallbackForAlterRelation(), we first ensure that the
current user is either a table owner or superuser and then check
actual permissions to perform the operations on the table. The same is
true for the DROP case. I don't see the need to change it.

Few cosmetic changes are attached in top-up patches. Dilip can include
these in the next version, if he is okay with them.

-- 
With Regards,
Amit Kapila.

diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index d438dc682ec..4578cd07140 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -86,8 +86,9 @@ bool
 IsSystemClass(Oid relid, Form_pg_class reltuple)
 {
        /* IsCatalogRelationOid is a bit faster, so test that first */
-       return (IsCatalogRelationOid(relid) || IsToastClass(reltuple)
-                       || IsConflictClass(reltuple));
+       return (IsCatalogRelationOid(relid) ||
+                       IsToastClass(reltuple) ||
+                       IsConflictClass(reltuple));
 }
 
 /*
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index fc3abb57e7e..c35fcf57fd4 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3523,9 +3523,8 @@ LookupCreationNamespace(const char *nspname)
 /*
  * Common checks on switching namespaces.
  *
- * We complain if either the old or new namespaces is a temporary schema
- * (or temporary toast schema), or if either the old or new namespaces is the
- * TOAST schema or the CONFLICT schema.
+ * We complain if either the old or new namespaces is a temporary schema,
+ * temporary toast schema, the TOAST schema, or the CONFLICT schema.
  */
 void
 CheckSetNamespace(Oid oldNspOid, Oid nspOid)

v29_amit_1.txt.patch
Description: Binary data