vacuumdb: permission denied for schema "pg_temp_7"

On Sat, Jul 06, 2024 at 05:19:39PM +0530, vaibhave postgres wrote:

Repo steps

1. Create a temporary table

sample => CREATE TEMPORARY TABLE temp_employees (

id SERIAL PRIMARY KEY,
name VARCHAR(100),
position VARCHAR(50),
salary NUMERIC(10, 2)
);
CREATE TABLE
sample => \dt pg_temp_*.*
List of relations
Schema | Name | Type | Owner
-----------+----------------+-------+----------
pg_temp_7 | temp_employees | table | vaibhave
(1 row)

2. Run vacuumdb

vacuumdb: vacuuming database "sample"

vacuumdb: error: processing of database " sample " failed: ERROR:
permission denied for schema pg_temp_7

Temporary tables can only be accessed within the session which created
them. They should be skipped during vacuumdb.

This happens when a non-superuser runs vacuumdb while a different user has a
temp table. This isn't specific to temp tables; it arises for any schema on
which the vacuumdb user lacks USAGE privilege.

v12 introduced this regression. I suspect it started when commit e0c2933 "Use
catalog query to discover tables to process in vacuumdb" switched vacuumdb
from a simple "VACUUM;" command to per-table commands. Non-superuser vacuumdb
must be rare indeed for this to go unnoticed long enough to leave all
supported branches affected.

Suggested Patch is attached

From ca78eb35b59cc398a37d36c27373dd64eb3a8f77 Mon Sep 17 00:00:00 2001
From: VaibhaveS <vaibhavedavey@gmail.com>
Date: Sat, 6 Jul 2024 17:15:33 +0530
Subject: [PATCH] Skip temporary tables in vacuumdb.

---
src/bin/scripts/vacuumdb.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7138c6e97e..3dbda53b72 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -733,6 +733,11 @@ vacuum_one_database(ConnParams *cparams,
has_where = true;
}

+	/*
+	 * Exclude temporary tables
+	 */
+	appendPQExpBufferStr(&catalog_query, " AND c.relpersistence <> 't'");

That helps, but we'd probably want to do something more general about vacuumdb
and schema USAGE permission.

Thanks for the report.

On Fri, Sep 20, 2024 at 12:07:31PM -0700, Noah Misch wrote:

v12 introduced this regression. I suspect it started when commit e0c2933 "Use
catalog query to discover tables to process in vacuumdb" switched vacuumdb
from a simple "VACUUM;" command to per-table commands. Non-superuser vacuumdb
must be rare indeed for this to go unnoticed long enough to leave all
supported branches affected.

I think the bug actually predates that commit, but it was only broken when
--jobs > 1. Commit e0c2933 just broke the --jobs == 1 case, too.

That helps, but we'd probably want to do something more general about vacuumdb
and schema USAGE permission.

Hm. I think filtering out schemas for which you lack USAGE makes sense
when neither --schema nor --table are specified, but if the user lists an
object they can't vacuum, we should probably fail. My current thinking is
that we could still filter when --exclude-schema is used, but I'm curious
what others think.

You might also be interested in this thread about VACUUM and USAGE [0]/messages/by-id/flat/56596b81-088f-4c0c-9a88-b5f27a7a62e9@oss.nttdata.com.

Thanks for the report.

+1

[0]: /messages/by-id/flat/56596b81-088f-4c0c-9a88-b5f27a7a62e9@oss.nttdata.com

--
nathan

On Fri, Sep 20, 2024 at 03:59:32PM -0500, Nathan Bossart wrote:

On Fri, Sep 20, 2024 at 12:07:31PM -0700, Noah Misch wrote:

v12 introduced this regression. I suspect it started when commit e0c2933 "Use
catalog query to discover tables to process in vacuumdb" switched vacuumdb
from a simple "VACUUM;" command to per-table commands. Non-superuser vacuumdb
must be rare indeed for this to go unnoticed long enough to leave all
supported branches affected.

I think the bug actually predates that commit, but it was only broken when
--jobs > 1. Commit e0c2933 just broke the --jobs == 1 case, too.

Agreed.

That helps, but we'd probably want to do something more general about vacuumdb
and schema USAGE permission.

Hm. I think filtering out schemas for which you lack USAGE makes sense
when neither --schema nor --table are specified, but if the user lists an
object they can't vacuum, we should probably fail. My current thinking is
that we could still filter when --exclude-schema is used, but I'm curious
what others think.

That all sounds good to me.

You might also be interested in this thread about VACUUM and USAGE [0].

[0] /messages/by-id/flat/56596b81-088f-4c0c-9a88-b5f27a7a62e9@oss.nttdata.com

The outcome is odd, but I'm not worried about it.

Noah Misch <noah@leadboat.com> writes:

That helps, but we'd probably want to do something more general about vacuumdb
and schema USAGE permission.

I agree a more general fix is needed, but I think excluding temp
tables as suggested is a good idea for performance, independently of
permissions concerns. vacuum_rel() will ignore requests to vacuum
such tables, which is why we've not heard complaints before, but
nonetheless we're wasting server round trips by issuing those
requests.

regards, tom lane

On 2024/09/21 4:07, Noah Misch wrote:

Non-superuser vacuumdb
must be rare indeed for this to go unnoticed long enough to leave all
supported branches affected.

Yes.
And more users might notice this in v17 or later, since v17 supports
the maintain privilege. Some users may want to use a role with
the maintain privilege to run vacuumdb. If they forget to grant USAGE
privilege on the temp table's schema to that role, they'll encounter
the same issue.

Regards,

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

On 2024/09/21 8:07, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

That helps, but we'd probably want to do something more general about vacuumdb
and schema USAGE permission.

I agree a more general fix is needed, but I think excluding temp
tables as suggested is a good idea for performance, independently of
permissions concerns. vacuum_rel() will ignore requests to vacuum
such tables, which is why we've not heard complaints before, but
nonetheless we're wasting server round trips by issuing those
requests.

+1

It looks like reindexdb has the same issue. It would be good to
update reindexdb to skip temp tables as well to fix this.

+ appendPQExpBufferStr(&catalog_query, " AND c.relpersistence <> 't'");

For the proposed patch, it seems better to use CppAsString2(RELPERSISTENCE_TEMP)
instead of 't'.

Regards,

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

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

It looks like reindexdb has the same issue. It would be good to
update reindexdb to skip temp tables as well to fix this.

Agreed.

For the proposed patch, it seems better to use CppAsString2(RELPERSISTENCE_TEMP)
instead of 't'.

+1, if we can easily avoid hard-coding that value we should do so.
It's not that we're going to change the value; it's that it makes
it way easier to grep the source tree for relevant code.

regards, tom lane

Thanks for the review and feedback.

On Sat, Sep 21, 2024 at 11:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

On Sat, Sep 21, 2024 at 11:42:19AM +0530, vaibhave postgres wrote:

Thanks for the review and feedback.

Are you planning to submit an updated patch? I don't think there's a
tremendous amount of urgency to fix this since it's been broken for so
long, but it'd be good to know whether someone is planning to pick it up.

--
nathan

On Sep 23, 2024, at 11:24, Nathan Bossart <nathandbossart@gmail.com> wrote:
Are you planning to submit an updated patch? I don't think there's a
tremendous amount of urgency to fix this since it's been broken for so
long, but it'd be good to know whether someone is planning to pick it up.

I'm happy to pick it up iff the current patch submitter doesn't want to continue with it.

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.

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.

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

Side note: we could use more CppAsString2() for relpersistence in
src/bin/, like in pg_amcheck, if we go this way.
--
Michael

Hi,

Yes I plan on continuing with working this.

Thanks.

On Tue, 24 Sept 2024, 00:18 Christophe Pettus, <xof@thebuild.com> wrote:

On Sep 23, 2024, at 18:09, vaibhave postgres <postgresvaibhave@gmail.com> wrote:
Yes I plan on continuing with working this.

Great!

One related but not identical thing that has come up with vacuumdb is that it terminates if it's not able to connect to any database that it finds in the initial query. This can happen if pg_hba.conf denies the user that is running vacuumdb access to a database that comes up during --all. Some hosting providers (in particular, Google) create restricted databases in the cluster that a customer role can't get access to. This pretty much defeats --analyze-in-stages. My suggested fix was to terminate with an error if the initial connection fails, but continue with a warning if further connections fail.

If it seems reasonable, I'm happy to do it in a separate patch.

Looks like Michael has already sent the updated patch from the discussions
in this thread so far. Let me know if anything else needs to be done.

Thanks.

On Tue, Sep 24, 2024 at 6:43 AM Christophe Pettus <xof@thebuild.com> wrote:

On Tue, Sep 24, 2024 at 07:05:41AM +0530, vaibhave postgres wrote:

Looks like Michael has already sent the updated patch from the discussions
in this thread so far. Let me know if anything else needs to be done.

Yes, the timing of your reply was interesting ;)

The temporary persistence issue is one side of the coin, and that's
the only part I have sent now as there is also a performance argument
(while being entirely useless to do). The permission parts with USAGE
and MAINTAIN need a bit more thoughts.
--
Michael

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

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.

Yeah, this is what I mentioned upthread [0]/messages/by-id/Zu3iMzfiGBTbg3iy@nathan. If the user doesn't specify
anything in --table or --schema, then it's probably fine to silently skip
objects for which they lack privileges. But if they do explicitly specify
a table or schema that they cannot vacuum, then IMHO it'd be better to
fail.

[0]: /messages/by-id/Zu3iMzfiGBTbg3iy@nathan

--
nathan

On Mon, Sep 23, 2024 at 06:13:23PM -0700, Christophe Pettus wrote:

One related but not identical thing that has come up with vacuumdb is
that it terminates if it's not able to connect to any database that it
finds in the initial query. This can happen if pg_hba.conf denies the
user that is running vacuumdb access to a database that comes up during
--all. Some hosting providers (in particular, Google) create restricted
databases in the cluster that a customer role can't get access to. This
pretty much defeats --analyze-in-stages. My suggested fix was to
terminate with an error if the initial connection fails, but continue
with a warning if further connections fail.

I think it'd be fine to continue with a warning when --all is used, but if
you specify a --dbname that you cannot connect to, then I think it should
fail.

--
nathan

On 2024/09/24 23:26, Nathan Bossart wrote:

Yeah, this is what I mentioned upthread [0]. If the user doesn't specify
anything in --table or --schema, then it's probably fine to silently skip
objects for which they lack privileges. But if they do explicitly specify
a table or schema that they cannot vacuum, then IMHO it'd be better to
fail.

This could be debatable. To be honest, if I run something like vacuumdb mydb,
*I* expect all eligible tables in that database to be vacuumed. If I forget to
grant the necessary privileges to the role, I’d prefer to see errors from
vacuumdb so I can fix the permissions.

If we decide to skip tables without enough privilege, I’d prefer adding
an option like --skip-unprivileged-tables rather than changing the default behavior.

Regards,

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