Re: vacuumdb: permission denied for schema "pg_temp_7"

On Tue, Sep 24, 2024 at 11:20:43PM +0900, Fujii Masao wrote:
> On 2024/09/24 10:08, Michael Paquier wrote:
>> 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.

I don't really have a strong opinion in favor of skipping the objects
or fail hard, because both have arguments. My point was just that, if
we were to skip the objects, then the query needs a bit of rework and
a SELECT with some extra filters in the CTE felt kind of right. If
the consensus is that errors are better, my point about rewriting the
query is most probably moot.

> 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().

Missed your point. An extra filter based on relpersistence can indeed
make sense for this path.
--
Michael