Message-ID: <00d98a9f-bec1-4cd0-b59e-1043ae04c8bc@oss.nttdata.com>
Date: Tue, 24 Sep 2024 23:20:43 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: vacuumdb: permission denied for schema "pg_temp_7"
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>,
 Christophe Pettus <xof@thebuild.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
 vaibhave postgres <postgresvaibhave@gmail.com>, Tom Lane
 <tgl@sss.pgh.pa.us>, Noah Misch <noah@leadboat.com>,
 pgsql-bugs@lists.postgresql.org, vsekar@microsoft.com
References: 
 <CAM_eQjwfAR=y3G1fGyS1U9FTmc+FyJm9amNfY2QCZBnDDbNPZg@mail.gmail.com>
 <20240920190731.37.nmisch@google.com> <Zu3iMzfiGBTbg3iy@nathan>
 <20240920215030.8d.nmisch@google.com> <2603142.1726873660@sss.pgh.pa.us>
 <5f37ee56-14fb-44fe-9150-9eb97e10538b@oss.nttdata.com>
 <2649165.1726898595@sss.pgh.pa.us>
 <CAM_eQjyWnu_LUt6JWjh3boNFHAT9DBrcbamhG3Xy39Q4Ph=2rg@mail.gmail.com>
 <ZvGye19tHrhLCpFl@nathan> <E9660D68-5B26-488E-9AB2-2CA1E8EDDDA1@thebuild.com>
 <ZvIRG6282T33q3Ku@paquier.xyz>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <ZvIRG6282T33q3Ku@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/00d98a9f-bec1-4cd0-b59e-1043ae04c8bc%40oss.nttdata.com>
Precedence: bulk


On 2024/09/24 10:08, Michael Paquier wrote:
> On Mon, Sep 23, 2024 at 11:48:21AM -0700, Christophe Pettus wrote:
>> I'm happy to pick it up iff the current patch submitter doesn't want
>> to continue with it.
> 
> Somewhat missed this thread, thanks for the latest activity.
> 
> If we apply a restriction on the temporary persistence, then we know
> that vacuumdb will always have a WHERE clause so we can simplify the
> code and remove the business with has_where like in the attached.

LGTM.


> About the permission restrictions depending on the objects listed, the
> filtering query uses currently a list of VALUES in a CTE.  Perhaps it
> would be more elegant to switch that to a SELECT with some
> has_schema_privilege() for the cases where OBJFILTER_SCHEMA is
> used?
> 
> There permission checks with USAGE and MAINTAIN are broader, so I'd
> choose to add a skip on the temp persistence first and backpatch it
> down to 12 as there is also a performance argument.  Then tackle the
> rest by reworking the VALUES part in the CTE.

Are you suggesting that any objects a user lacks sufficient privileges for
should be silently excluded from vacuuming? This could make vacuumdb appear
successful because no errors occur, but some tables the user intended to
vacuum might be skipped without notice. That seems more problematic to me.

Sorry if I misunderstood your point.


> The REINDEX one is really something that we need to care about?  One
> needs database ownership for a database-level REINDEX, or schema-level
> ownership for the schema-level one.  Stricter restrictions apply
> compared to vacuum_rel().

Is ownership really necessary in these cases? A similar issue can easily
happen with reindexdb as follows, so I think that should be fixed as well.

=# CREATE ROLE admin WITH LOGIN;
=# GRANT pg_maintain TO admin;
=# CREATE TEMP TABLE tt (i int primary key);
=# \! bin/reindexdb -U admin -j 2 postgres
reindexdb: error: query failed: ERROR:  permission denied for schema pg_temp_0
LINE 4:   AND c.oid OPERATOR(pg_catalog.=) 'pg_temp_0.tt'::pg_catalo...
                                            ^
reindexdb: detail: Query was: SELECT c.relname, ns.nspname
  FROM pg_catalog.pg_class c, pg_catalog.pg_namespace ns
  WHERE c.relnamespace OPERATOR(pg_catalog.=) ns.oid
   AND c.oid OPERATOR(pg_catalog.=) 'pg_temp_0.tt'::pg_catalog.regclass;

We should probably add a condition like "relpersistence != CppAsString2(RELPERSISTENCE_TEMP)"
to the queries in get_parallel_object_list().

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION