Thread: 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

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] and marked it as ready-for-committer.

[0] https://commitfest.postgresql.org/41/4094/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com