Re: Fix wrong error message from pg_get_tablespace_ddl()

From: Andrew Dunstan <andrew(at)dunslane(dot)net>
To: Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Fix wrong error message from pg_get_tablespace_ddl()
Date: 2026-05-08 12:07:38
Message-ID: 91b25e9b-5761-4046-84ef-cb73b5db17e6@dunslane.net
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2026-05-08 12:09:53 Re: Proposal: Conflict log history table for Logical Replication
Previous Message John Naylor 2026-05-08 10:04:15 Re: Broken build on macOS (Universal / Intel): cpuid instruction not available