Обсуждение: Fix wrong error message from pg_get_tablespace_ddl()

Hi,

I started testing pg_get_tablespace_ddl(). While tracing pg_get_tablespace_ddl_internal(), I noticed that this error
reportmust be wrong: 
```
    /* User must have SELECT privilege on pg_tablespace. */
    if (pg_class_aclcheck(TableSpaceRelationId, GetUserId(), ACL_SELECT) != ACLCHECK_OK)
    {
        ReleaseSysCache(tuple);
        aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLESPACE, spcname);
    }
```

The comment clearly says that SELECT privilege on pg_tablespace is required, but the error is reported against the
targettablespace instead. 

This is easy to reproduce:
```
evantest=# set allow_in_place_tablespaces = true;
SET
evantest=# create role r1;
CREATE ROLE
evantest=# create tablespace ts1 location '';
CREATE TABLESPACE
evantest=# revoke select on pg_tablespace from r1;
REVOKE
evantest=# set role r1;
SET
evantest=> select * from pg_get_tablespace_ddl('ts1');
ERROR:  permission denied for tablespace ts1
```

Attached is a simple one-line fix. Attached is a simple one-line fix. I did not add a new test, as we usually try to
avoidextending the test time for such a small fix. With the fix, the error message now looks like: 
```
evantest=> select * from pg_get_tablespace_ddl('ts1');
ERROR:  permission denied for table pg_tablespace
```

Oops, I was one of the reviewers of the original patch. Sorry for not finding this during review.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi Chao

On 08/05/2026 10:14, Chao Li wrote:
> This is easy to reproduce:
> ```
> evantest=# set allow_in_place_tablespaces = true;
> SET
> evantest=# create role r1;
> CREATE ROLE
> evantest=# create tablespace ts1 location '';
> CREATE TABLESPACE
> evantest=# revoke select on pg_tablespace from r1;
> REVOKE
> evantest=# set role r1;
> SET
> evantest=> select * from pg_get_tablespace_ddl('ts1');
> ERROR:  permission denied for tablespace ts1
> ```
> 
> Attached is a simple one-line fix. Attached is a simple one-line fix. I did not add a new test, as we usually try to
avoidextending the test time for such a small fix. With the fix, the error message now looks like:
 
> ```
> evantest=> select * from pg_get_tablespace_ddl('ts1');
> ERROR:  permission denied for table pg_tablespace
> ```

A few comments:

== hardcoded table name ==

Hardcoding "pg_tablespace" looks IMO a bit fragile. What about
get_rel_name(TableSpaceRelationId) instead?

See get_database_name(dbid) in pg_get_database_ddl_internal().

== similar issue in pg_get_role_ddl_internal ==

If we do this change, we should also address pg_authid to keep the code 
consistent:

/* User must have SELECT privilege on pg_authid. */
if (pg_class_aclcheck(AuthIdRelationId, GetUserId(), ACL_SELECT) != 
ACLCHECK_OK)
{
   ReleaseSysCache(tuple);
   ereport(ERROR,
   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
    errmsg("permission denied for role %s", rolname)));
}

Perhaps something like this instead of the ereport:

aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLE, 
get_rel_name(AuthIdRelationId));

Thanks!

Best, Jim

On 2026-05-08 Fr 5:20 AM, Jim Jones wrote:
> Hi Chao
>
> On 08/05/2026 10:14, Chao Li wrote:
>> This is easy to reproduce:
>> ```
>> evantest=# set allow_in_place_tablespaces = true;
>> SET
>> evantest=# create role r1;
>> CREATE ROLE
>> evantest=# create tablespace ts1 location '';
>> CREATE TABLESPACE
>> evantest=# revoke select on pg_tablespace from r1;
>> REVOKE
>> evantest=# set role r1;
>> SET
>> evantest=> select * from pg_get_tablespace_ddl('ts1');
>> ERROR:  permission denied for tablespace ts1
>> ```
>>
>> Attached is a simple one-line fix. Attached is a simple one-line fix. 
>> I did not add a new test, as we usually try to avoid extending the 
>> test time for such a small fix. With the fix, the error message now 
>> looks like:
>> ```
>> evantest=> select * from pg_get_tablespace_ddl('ts1');
>> ERROR:  permission denied for table pg_tablespace
>> ```
>
> A few comments:
>
> == hardcoded table name ==
>
> Hardcoding "pg_tablespace" looks IMO a bit fragile. What about
> get_rel_name(TableSpaceRelationId) instead?
>
> See get_database_name(dbid) in pg_get_database_ddl_internal().
>
> == similar issue in pg_get_role_ddl_internal ==
>
> If we do this change, we should also address pg_authid to keep the 
> code consistent:
>
> /* User must have SELECT privilege on pg_authid. */
> if (pg_class_aclcheck(AuthIdRelationId, GetUserId(), ACL_SELECT) != 
> ACLCHECK_OK)
> {
>   ReleaseSysCache(tuple);
>   ereport(ERROR,
>   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>     errmsg("permission denied for role %s", rolname)));
> }
>
> Perhaps something like this instead of the ereport:
>
> aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLE, 
> get_rel_name(AuthIdRelationId));
>
>

I'm not 100% convinced these are in fact wrong. The user asks for the 
DDL for a named role or tablespace and we tell them that they don't have 
the privilege to get the information for that object. If we tell them 
that the problem is that they don't have privilege on the underlying 
catalog table, they might think "Well, I didn't ask for that".


cheers


andrew



>
>
--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 08/05/2026 14:07, Andrew Dunstan wrote:
> I'm not 100% convinced these are in fact wrong. The user asks for the 
> DDL for a named role or tablespace and we tell them that they don't have 
> the privilege to get the information for that object. If we tell them 
> that the problem is that they don't have privilege on the underlying 
> catalog table, they might think "Well, I didn't ask for that".

I honestly don't have a strong opinion either way.

It depends on what we expect from the error message. If its purpose is 
simply to tell the user "you can't access this object," the current 
message is totally fine. If, however, the goal is to show the error's 
root cause, it could be a bit misleading.

Best, Jim