Avoid extra "skipping" messages from VACUUM/ANALYZE
Right now, if an unprivileged user issues VACUUM/ANALYZE (without
specifying a table), it will emit messages for each relation that it
skips, including indexes, views, and other objects that can't be a
direct target of VACUUM/ANALYZE anyway. Attached patch causes it to
check the type of object first, and then check privileges second.
Found while reviewing the MAINTAIN privilege patch. Implemented with
his suggested fix. I intend to commit soon.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
Attachments:
v1-0001-Clean-up-useless-skipping-messages-for-VACUUM-ANA.patchtext/x-patch; charset=UTF-8; name=v1-0001-Clean-up-useless-skipping-messages-for-VACUUM-ANA.patchDownload
From e1f0c310175285945ca0742b92fc70e2405cb831 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 13 Dec 2022 17:37:22 -0800
Subject: [PATCH v1] Clean up useless "skipping" messages for VACUUM/ANALYZE.
When VACUUM/ANALYZE are run on an entire database, it warns of
skipping relations for which the user doesn't have sufficient
privileges. That only makes sense for tables, so skip such messages
for indexes, etc.
---
src/backend/commands/vacuum.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 293b84bbca..bb4847dfa0 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -874,10 +874,6 @@ get_all_vacuum_rels(int options)
MemoryContext oldcontext;
Oid relid = classForm->oid;
- /* check permissions of relation */
- if (!vacuum_is_permitted_for_relation(relid, classForm, options))
- continue;
-
/*
* We include partitioned tables here; depending on which operation is
* to be performed, caller will decide whether to process or ignore
@@ -888,6 +884,10 @@ get_all_vacuum_rels(int options)
classForm->relkind != RELKIND_PARTITIONED_TABLE)
continue;
+ /* check permissions of relation */
+ if (!vacuum_is_permitted_for_relation(relid, classForm, options))
+ continue;
+
/*
* Build VacuumRelation(s) specifying the table OIDs to be processed.
* We omit a RangeVar since it wouldn't be appropriate to complain
--
2.34.1
On Tue, Dec 13, 2022 at 06:29:56PM -0800, Jeff Davis wrote:
Right now, if an unprivileged user issues VACUUM/ANALYZE (without
specifying a table), it will emit messages for each relation that it
skips, including indexes, views, and other objects that can't be a
direct target of VACUUM/ANALYZE anyway. Attached patch causes it to
check the type of object first, and then check privileges second.
This also seems to be the case when a table name is specified:
postgres=# CREATE TABLE test (a INT);
CREATE TABLE
postgres=# CREATE INDEX ON test (a);
CREATE INDEX
postgres=# CREATE ROLE myuser;
CREATE ROLE
postgres=# SET ROLE myuser;
SET
postgres=> VACUUM test_a_idx;
WARNING: permission denied to vacuum "test_a_idx", skipping it
VACUUM
Granted, this likely won't create as much noise as a database-wide VACUUM,
but perhaps we could add a relkind check in expand_vacuum_rel() and swap
the checks in vacuum_rel()/analyze_rel(), too. I don't know if it's worth
the trouble, though.
Found while reviewing the MAINTAIN privilege patch. Implemented with
his suggested fix. I intend to commit soon.
LGTM
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Dec 13, 2022 at 07:40:59PM -0800, Nathan Bossart wrote:
Granted, this likely won't create as much noise as a database-wide VACUUM,
but perhaps we could add a relkind check in expand_vacuum_rel() and swap
the checks in vacuum_rel()/analyze_rel(), too. I don't know if it's worth
the trouble, though.
I looked into this. I don't think adding a check in expand_vacuum_rel() is
worth much because we'd have to permit all relkinds that can be either
vacuumed or analyzed, and you have to check the relkind again in
vacuum_rel()/analyze_rel() anyway. It's easy enough to postpone the
permissions check in vacuum_rel() so that the relkind messages take
precedence, but if we do the same in analyze_rel(), FDWs'
AnalyzeForeignTable functions will be called prior to checking permissions,
which doesn't seem great. We could move the call to AnalyzeForeignTable
out of the relkind check to avoid this, but I'm having trouble believing
it's worth it to reorder the WARNING messages.
Ultimately, I think reversing the checks in get_all_vacuum_rels() (as your
patch does) should eliminate most of the noise, so I filed a commitfest
entry [0]https://commitfest.postgresql.org/41/4094/ and marked it as ready-for-committer.
[0]: https://commitfest.postgresql.org/41/4094/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com