vacuumdb --missing-stats-only and permission issue
Hi,
While following the discussion on vacuumdb --missing-stats-only [1]/messages/by-id/20250820104226.8ba51e43164cd590b863ce41@sraoss.co.jp,
I noticed that this option queries pg_statistic and pg_statistic_ext_data.
As a result, non-superusers like pg_maintain cannot use it because
by default they lack permission to access those catalogs.
I'm not sure whether --missing-stats-only was intended to work for
non-superusers, but if so, this restriction is inconvenient. Would it
make sense to use the views pg_stats and pg_stats_ext instead?
Since the catalogs are only consulted to check whether statistics exist,
the views should be sufficient. Thought?
Regards,
[1]: /messages/by-id/20250820104226.8ba51e43164cd590b863ce41@sraoss.co.jp
--
Fujii Masao
On Wed, Aug 20, 2025 at 11:06 PM Fujii Masao <masao.fujii@gmail.com> wrote:
I'm not sure whether --missing-stats-only was intended to work for
non-superusers, but if so, this restriction is inconvenient. Would it
make sense to use the views pg_stats and pg_stats_ext instead?
Since the catalogs are only consulted to check whether statistics exist,
the views should be sufficient. Thought?
It seems like it should be possible.
Initially, I was afraid that index statistics don't make it into pg_stats,
but experimentation shows that they do as far back as I looked (10-stable).
A similar issue might happen with tables themselves, but if the user
doesn't have permission to see the stats for that table, they weren't going
to get far trying to vacuum the table.
Any issue with expressions columns in an index seems to be resolved as
well, as the generated pg_attribute.attname is carried forward. I suppose
there might be a collision with old stats and renamed columns, but that's a
rare case, and it would still mean that the table had stats, just not for
the new column.
The main problem would be if has_column_privilege() works on indexes for
non-owners all the way back, if it doesn't, we're stuck.
One issue that may come up is that because pg_stats and pg_stats_ext are
security barrier views, the planner is prone to hash joins (and thus full
scans) of pg_stats, ignoring otherwise indexes that are ideal for
single-row lookups like you get with EXISTS and NOT EXISTS clauses. See
comment about "pg_class_relname_nsp_index" and associated query block in
pg_dump.c for an example of what we had to do to make the planner avoid a
full-scan.
Assuming that I'm not missing something, the fix seems straightforward.
I'll set about coding it up tomorrow if nobody has done so by then.
On Thu, Aug 21, 2025 at 03:19:40AM -0400, Corey Huinker wrote:
Assuming that I'm not missing something, the fix seems straightforward.
I'll set about coding it up tomorrow if nobody has done so by then.
I've added an open item for this.
--
nathan
On Thu, Aug 21, 2025 at 9:44 AM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Thu, Aug 21, 2025 at 03:19:40AM -0400, Corey Huinker wrote:
Assuming that I'm not missing something, the fix seems straightforward.
I'll set about coding it up tomorrow if nobody has done so by then.I've added an open item for this.
Here's the query changes, no regression test just yet.
Attachments:
v1-0001-Have-missing-stats-query-use-security-barrier-vie.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Have-missing-stats-query-use-security-barrier-vie.patchDownload
From 62d41900ff0938472e808bf2a87e35d294717698 Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huinker@gmail.com>
Date: Thu, 21 Aug 2025 12:30:17 -0400
Subject: [PATCH v1] Have missing-stats query use security barrier views.
Previously, the missing-stats query used pg_statistic and
pg_statistic_ext_data, which meant that the queries would fail for
non-superusers like pg_maintain as reported by Fujii Masao.
---
src/bin/scripts/vacuumdb.c | 47 +++++++++++++++++++++++---------------
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 22093e50aa5..919c77035a1 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -973,35 +973,42 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
" AND a.attnum OPERATOR(pg_catalog.>) 0::pg_catalog.int2\n"
" AND NOT a.attisdropped\n"
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) c.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* extended stats */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext e\n"
+ " JOIN pg_catalog.pg_namespace en"
+ " ON en.oid OPERATOR(pg_catalog.=) e.stxnamespace\n"
" WHERE e.stxrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND e.stxstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext_data d\n"
- " WHERE d.stxoid OPERATOR(pg_catalog.=) e.oid\n"
- " AND d.stxdinherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats_ext d\n"
+ " WHERE d.statistics_schemaname OPERATOR(pg_catalog.=) en.nspname\n"
+ " AND d.statistics_name OPERATOR(pg_catalog.=) e.stxname\n"
+ " AND d.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* expression indexes */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_attribute a\n"
" JOIN pg_catalog.pg_index i"
" ON i.indexrelid OPERATOR(pg_catalog.=) a.attrelid\n"
+ " JOIN pg_catalog.pg_class ic"
+ " ON ic.oid OPERATOR(pg_catalog.=) i.indexrelid\n"
" WHERE i.indrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND i.indkey[a.attnum OPERATOR(pg_catalog.-) 1::pg_catalog.int2]"
" OPERATOR(pg_catalog.=) 0::pg_catalog.int2\n"
" AND a.attnum OPERATOR(pg_catalog.>) 0::pg_catalog.int2\n"
" AND NOT a.attisdropped\n"
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) ic.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* inheritance and regular stats */
appendPQExpBufferStr(&catalog_query,
@@ -1014,23 +1021,27 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
" AND NOT p.inherited\n"
" AND EXISTS (SELECT NULL FROM pg_catalog.pg_inherits h\n"
" WHERE h.inhparent OPERATOR(pg_catalog.=) c.oid)\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) c.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* inheritance and extended stats */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext e\n"
+ " JOIN pg_catalog.pg_namespace en"
+ " ON en.oid OPERATOR(pg_catalog.=) e.stxnamespace\n"
" WHERE e.stxrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND e.stxstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
" AND c.relhassubclass\n"
" AND NOT p.inherited\n"
" AND EXISTS (SELECT NULL FROM pg_catalog.pg_inherits h\n"
" WHERE h.inhparent OPERATOR(pg_catalog.=) c.oid)\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext_data d\n"
- " WHERE d.stxoid OPERATOR(pg_catalog.=) e.oid\n"
- " AND d.stxdinherit))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats_ext d\n"
+ " WHERE d.statistics_schemaname OPERATOR(pg_catalog.=) en.nspname\n"
+ " AND d.statistics_name OPERATOR(pg_catalog.=) e.stxname\n"
+ " AND d.inherited))\n");
appendPQExpBufferStr(&catalog_query, " )\n");
}
base-commit: 12da45742cfd15d9fab151b25400d96a1febcbde
--
2.50.1
On Thu, Aug 21, 2025 at 12:52:17PM -0400, Corey Huinker wrote:
Here's the query changes, no regression test just yet.
I think there's a problem with the privilege checks for pg_stats (and
friends) versus ANALYZE. pg_stats checks for SELECT privileges on the
column, while ANALYZE checks for MAINTAIN privileges. If a role lacks
SELECT on the columns, it will always try to analyze the table. If a role
lacks MAINTAIN on the table, it will fail if it tries to analyze the table.
create role myrole login;
create table no_stats_select as select generate_series(1, 10);
create table no_stats_maintain as select generate_series(1, 10);
create table stats_select as select generate_series(1, 10);
create table stats_maintain as select generate_series(1, 10);
grant maintain on no_stats_maintain to myrole;
grant maintain on stats_maintain to myrole;
grant select on no_stats_select to myrole;
grant select on stats_select to myrole;
analyze stats_select, stats_maintain;
$ vacuumdb ... --missing-stats-only -Z -t stats_select
vacuumdb: vacuuming database "postgres"
$ vacuumdb ... --missing-stats-only -Z -t stats_maintain
vacuumdb: vacuuming database "postgres"
INFO: analyzing "public.stats_maintain"
...
$ vacuumdb ... --missing-stats-only -Z -t no_stats_select
vacuumdb: vacuuming database "postgres"
WARNING: permission denied to analyze "no_stats_select", skipping it
$ vacuumdb ... --missing-stats-only -Z -t no_stats_maintain
vacuumdb: vacuuming database "postgres"
INFO: analyzing "public.no_stats_maintain"
...
Perhaps we should also filter for only columns for which the user has
SELECT and MAINTAIN. Another option is just to look for SELECT and let the
attempt to ANALYZE it fail (since it just produces a WARNING). Whatever we
choose should probably be noted in the vacuumdb docs.
--
nathan
On Thu, Aug 21, 2025 at 12:59:52PM -0500, Nathan Bossart wrote:
I think there's a problem with the privilege checks for pg_stats (and
friends) versus ANALYZE. pg_stats checks for SELECT privileges on the
column, while ANALYZE checks for MAINTAIN privileges. If a role lacks
SELECT on the columns, it will always try to analyze the table. If a role
lacks MAINTAIN on the table, it will fail if it tries to analyze the table.
Unfortunately, pg_stats_ext is also different. The data for that view is
restricted to table owners (or roles that inherit privileges of the table
owner).
--
nathan
Unfortunately, pg_stats_ext is also different. The data for that view is
restricted to table owners (or roles that inherit privileges of the table
owner).
Ok, I took the RLS and permissions quals from pg_stats and pg_stats_ext
and put them in the corresponding EXISTs tests. The queries could be
written a bit more succinctly (ex. we only need to do the RLS checks once)
but putting them in each EXISTS clause drives home the point that we're
duplicating the filters in pg_stats/pg_stats_ext.
So now, we avoid probing attributes for stats that we know were going to be
filtered out, likewise for extended statistics objects.
The queries don't consult pg_stats_exprs because that's just exposing
different stats from the same pg_statistic_ext_data row, and we already
know that the row is there or not.
Attachments:
v2-0001-Have-missing-stats-query-use-security-barrier-vie.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Have-missing-stats-query-use-security-barrier-vie.patchDownload
From a217491ce3f4fc8e0bba2a743e8c2a2833749505 Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huinker@gmail.com>
Date: Thu, 21 Aug 2025 12:30:17 -0400
Subject: [PATCH v2] Have missing-stats query use security barrier views.
Previously, the missing-stats query used pg_statistic and
pg_statistic_ext_data, which meant that the queries would fail for
non-superusers like pg_maintain as reported by Fujii Masao.
Because the security barrier views will obscure certain statistics from
the user, it is important that each EXISTS() test also apply the same
filter in generating the list of attributes and extended stats to avoid
false positives.
This unfortunately means that we do not know if the columns the user
can't see have stats or not, but the alternative is false positives.
---
src/bin/scripts/vacuumdb.c | 62 +++++++++++++++++++++++++++-----------
1 file changed, 44 insertions(+), 18 deletions(-)
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 22093e50aa5..2b5e8ac91e8 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -972,36 +972,52 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
" WHERE a.attrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND a.attnum OPERATOR(pg_catalog.>) 0::pg_catalog.int2\n"
" AND NOT a.attisdropped\n"
+ " AND pg_catalog.has_column_privilege(c.oid, a.attnum, 'select'::pg_catalog.text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) c.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* extended stats */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext e\n"
+ " JOIN pg_catalog.pg_namespace en"
+ " ON en.oid OPERATOR(pg_catalog.=) e.stxnamespace\n"
" WHERE e.stxrelid OPERATOR(pg_catalog.=) c.oid\n"
+ " AND pg_catalog.pg_has_role(c.relowner, 'USAGE'::text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND e.stxstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext_data d\n"
- " WHERE d.stxoid OPERATOR(pg_catalog.=) e.oid\n"
- " AND d.stxdinherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats_ext d\n"
+ " WHERE d.statistics_schemaname OPERATOR(pg_catalog.=) en.nspname\n"
+ " AND d.statistics_name OPERATOR(pg_catalog.=) e.stxname\n"
+ " AND d.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* expression indexes */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_attribute a\n"
" JOIN pg_catalog.pg_index i"
" ON i.indexrelid OPERATOR(pg_catalog.=) a.attrelid\n"
+ " JOIN pg_catalog.pg_class ic"
+ " ON ic.oid OPERATOR(pg_catalog.=) i.indexrelid\n"
" WHERE i.indrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND i.indkey[a.attnum OPERATOR(pg_catalog.-) 1::pg_catalog.int2]"
" OPERATOR(pg_catalog.=) 0::pg_catalog.int2\n"
" AND a.attnum OPERATOR(pg_catalog.>) 0::pg_catalog.int2\n"
" AND NOT a.attisdropped\n"
+ " AND pg_catalog.has_column_privilege(ic.oid, a.attnum, 'select'::pg_catalog.text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) ic.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* inheritance and regular stats */
appendPQExpBufferStr(&catalog_query,
@@ -1012,25 +1028,35 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
" AND c.relhassubclass\n"
" AND NOT p.inherited\n"
+ " AND pg_catalog.has_column_privilege(c.oid, a.attnum, 'select'::pg_catalog.text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND EXISTS (SELECT NULL FROM pg_catalog.pg_inherits h\n"
" WHERE h.inhparent OPERATOR(pg_catalog.=) c.oid)\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) c.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* inheritance and extended stats */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext e\n"
+ " JOIN pg_catalog.pg_namespace en"
+ " ON en.oid OPERATOR(pg_catalog.=) e.stxnamespace\n"
" WHERE e.stxrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND e.stxstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
" AND c.relhassubclass\n"
" AND NOT p.inherited\n"
+ " AND pg_catalog.pg_has_role(c.relowner, 'USAGE'::text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND EXISTS (SELECT NULL FROM pg_catalog.pg_inherits h\n"
" WHERE h.inhparent OPERATOR(pg_catalog.=) c.oid)\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext_data d\n"
- " WHERE d.stxoid OPERATOR(pg_catalog.=) e.oid\n"
- " AND d.stxdinherit))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats_ext d\n"
+ " WHERE d.statistics_schemaname OPERATOR(pg_catalog.=) en.nspname\n"
+ " AND d.statistics_name OPERATOR(pg_catalog.=) e.stxname\n"
+ " AND d.inherited))\n");
appendPQExpBufferStr(&catalog_query, " )\n");
}
base-commit: 12da45742cfd15d9fab151b25400d96a1febcbde
--
2.50.1
On Thu, Aug 21, 2025 at 07:27:23PM -0400, Corey Huinker wrote:
Ok, I took the RLS and permissions quals from pg_stats and pg_stats_ext
and put them in the corresponding EXISTs tests. The queries could be
written a bit more succinctly (ex. we only need to do the RLS checks once)
but putting them in each EXISTS clause drives home the point that we're
duplicating the filters in pg_stats/pg_stats_ext.
Looks reasonable to me. It's unfortunate that we have to be so
restrictive, but since this is primarily intended for administrators to use
post-upgrade, I think it's okay for v18. For v19, perhaps we should
provide a view or function that returns whether stats are missing. That
could require MAINTAIN on the relation to match what's needed for ANALYZE.
We'll also need documentation and test updates, of course. For the docs,
I'd try to borrow some of the wording from pg_stats and pg_stats_ext:
This view allows access only to rows of pg_statistic that correspond to
tables the user has permission to read...
This view allows access only to rows of pg_statistic_ext and
pg_statistic_ext_data that correspond to tables the user owns...
--
nathan
On Fri, Aug 22, 2025 at 09:01:36AM -0500, Nathan Bossart wrote:
We'll also need documentation and test updates, of course.
Here is an attempt at the docs/tests. I also fixed a couple of small
issues in the query.
--
nathan
Attachments:
v3-0001-Have-missing-stats-query-use-security-barrier-vie.patchtext/plain; charset=us-asciiDownload
From e75f534d4662a4c0ba3ee0518c08b3692cdc0255 Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huinker@gmail.com>
Date: Thu, 21 Aug 2025 12:30:17 -0400
Subject: [PATCH v3 1/1] Have missing-stats query use security barrier views.
Previously, the missing-stats query used pg_statistic and
pg_statistic_ext_data, which meant that the queries would fail for
non-superusers like pg_maintain as reported by Fujii Masao.
Because the security barrier views will obscure certain statistics from
the user, it is important that each EXISTS() test also apply the same
filter in generating the list of attributes and extended stats to avoid
false positives.
This unfortunately means that we do not know if the columns the user
can't see have stats or not, but the alternative is false positives.
---
doc/src/sgml/ref/vacuumdb.sgml | 7 +++
src/bin/scripts/t/100_vacuumdb.pl | 96 +++++++++++++++++++++++++++++++
src/bin/scripts/vacuumdb.c | 62 ++++++++++++++------
3 files changed, 147 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 53147480515..f797abc273a 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -292,6 +292,13 @@ PostgreSQL documentation
This option can only be used in conjunction with
<option>--analyze-only</option> or <option>--analyze-in-stages</option>.
</para>
+ <para>
+ Note that <option>--missing-stats-only</option> only considers
+ statistics for tables the user has permission to read and extended
+ statistics for tables the user owns; if the user lacks those
+ privileges, <application>vacuumdb</application> may choose not to
+ analyze a relation even if it is missing some statistics.
+ </para>
</listitem>
</varlistentry>
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index 945c30df156..3ee089f8a9f 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -240,7 +240,29 @@ $node->command_fails_like(
$node->safe_psql('postgres', q|
CREATE TABLE regression_vacuumdb_test AS select generate_series(1, 10) a, generate_series(2, 11) b;
ALTER TABLE regression_vacuumdb_test ADD COLUMN c INT GENERATED ALWAYS AS (a + b);
+ ALTER TABLE regression_vacuumdb_test ENABLE ROW LEVEL SECURITY;
+ CREATE ROLE regression_noprivs LOGIN BYPASSRLS;
+ CREATE ROLE regression_rls LOGIN;
+ GRANT SELECT ON regression_vacuumdb_test TO regression_rls;
|);
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-only',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_noprivs'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing stats and unprivileged role');
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-only',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_rls'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing stats and role subject to RLS');
$node->issues_sql_like(
[
'vacuumdb', '--analyze-only',
@@ -261,6 +283,24 @@ $node->issues_sql_unlike(
$node->safe_psql('postgres',
'CREATE INDEX regression_vacuumdb_test_idx ON regression_vacuumdb_test (mod(a, 2));'
);
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-in-stages',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_noprivs'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing index expression stats and unprivileged role');
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-in-stages',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_rls'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing index expression stats and role subject to RLS');
$node->issues_sql_like(
[
'vacuumdb', '--analyze-in-stages',
@@ -281,6 +321,24 @@ $node->issues_sql_unlike(
$node->safe_psql('postgres',
'CREATE STATISTICS regression_vacuumdb_test_stat ON a, b FROM regression_vacuumdb_test;'
);
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-only',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_noprivs'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing extended stats and unprivileged role');
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-only',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_rls'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing extended stats and role subject to RLS');
$node->issues_sql_like(
[
'vacuumdb', '--analyze-only',
@@ -302,6 +360,24 @@ $node->safe_psql('postgres',
"CREATE TABLE regression_vacuumdb_child (a INT) INHERITS (regression_vacuumdb_test);\n"
. "INSERT INTO regression_vacuumdb_child VALUES (1, 2);\n"
. "ANALYZE regression_vacuumdb_child;\n");
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-in-stages',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_noprivs'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing inherited stats and unprivileged role');
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-in-stages',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_test', 'postgres',
+ '--username', 'regression_rls'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing inherited stats and role subject to RLS');
$node->issues_sql_like(
[
'vacuumdb', '--analyze-in-stages',
@@ -321,9 +397,29 @@ $node->issues_sql_unlike(
$node->safe_psql('postgres',
"CREATE TABLE regression_vacuumdb_parted (a INT) PARTITION BY LIST (a);\n"
+ . "ALTER TABLE regression_vacuumdb_parted ENABLE ROW LEVEL SECURITY;\n"
+ . "GRANT SELECT ON regression_vacuumdb_parted TO regression_rls;\n"
. "CREATE TABLE regression_vacuumdb_part1 PARTITION OF regression_vacuumdb_parted FOR VALUES IN (1);\n"
. "INSERT INTO regression_vacuumdb_parted VALUES (1);\n"
. "ANALYZE regression_vacuumdb_part1;\n");
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-only',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_parted', 'postgres',
+ '--username', 'regression_noprivs'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing partition stats and unprivileged role');
+$node->issues_sql_unlike(
+ [
+ 'vacuumdb', '--analyze-only',
+ '--missing-stats-only', '-t',
+ 'regression_vacuumdb_parted', 'postgres',
+ '--username', 'regression_rls'
+ ],
+ qr/statement:\ ANALYZE/sx,
+ '--missing-stats-only with missing partition stats and row subject to RLS');
$node->issues_sql_like(
[
'vacuumdb', '--analyze-only',
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index fd236087e90..244c0a885e9 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -973,38 +973,54 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
" WHERE a.attrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND a.attnum OPERATOR(pg_catalog.>) 0::pg_catalog.int2\n"
" AND NOT a.attisdropped\n"
+ " AND pg_catalog.has_column_privilege(c.oid, a.attnum, 'select'::pg_catalog.text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
" AND a.attgenerated OPERATOR(pg_catalog.<>) "
CppAsString2(ATTRIBUTE_GENERATED_VIRTUAL) "\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) c.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* extended stats */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext e\n"
+ " JOIN pg_catalog.pg_namespace en"
+ " ON en.oid OPERATOR(pg_catalog.=) e.stxnamespace\n"
" WHERE e.stxrelid OPERATOR(pg_catalog.=) c.oid\n"
+ " AND pg_catalog.pg_has_role(c.relowner, 'USAGE'::text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND e.stxstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext_data d\n"
- " WHERE d.stxoid OPERATOR(pg_catalog.=) e.oid\n"
- " AND d.stxdinherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats_ext d\n"
+ " WHERE d.statistics_schemaname OPERATOR(pg_catalog.=) en.nspname\n"
+ " AND d.statistics_name OPERATOR(pg_catalog.=) e.stxname\n"
+ " AND d.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* expression indexes */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_attribute a\n"
" JOIN pg_catalog.pg_index i"
" ON i.indexrelid OPERATOR(pg_catalog.=) a.attrelid\n"
+ " JOIN pg_catalog.pg_class ic"
+ " ON ic.oid OPERATOR(pg_catalog.=) i.indexrelid\n"
" WHERE i.indrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND i.indkey[a.attnum OPERATOR(pg_catalog.-) 1::pg_catalog.int2]"
" OPERATOR(pg_catalog.=) 0::pg_catalog.int2\n"
" AND a.attnum OPERATOR(pg_catalog.>) 0::pg_catalog.int2\n"
" AND NOT a.attisdropped\n"
+ " AND pg_catalog.has_column_privilege(ic.oid, a.attnum, 'select'::pg_catalog.text)\n"
+ " AND (ic.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(ic.oid))\n"
" AND a.attstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit OPERATOR(pg_catalog.=) p.inherited))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) ic.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited OPERATOR(pg_catalog.=) p.inherited))\n");
/* inheritance and regular stats */
appendPQExpBufferStr(&catalog_query,
@@ -1017,25 +1033,35 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
CppAsString2(ATTRIBUTE_GENERATED_VIRTUAL) "\n"
" AND c.relhassubclass\n"
" AND NOT p.inherited\n"
+ " AND pg_catalog.has_column_privilege(c.oid, a.attnum, 'select'::pg_catalog.text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND EXISTS (SELECT NULL FROM pg_catalog.pg_inherits h\n"
" WHERE h.inhparent OPERATOR(pg_catalog.=) c.oid)\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic s\n"
- " WHERE s.starelid OPERATOR(pg_catalog.=) a.attrelid\n"
- " AND s.staattnum OPERATOR(pg_catalog.=) a.attnum\n"
- " AND s.stainherit))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats s\n"
+ " WHERE s.schemaname OPERATOR(pg_catalog.=) ns.nspname\n"
+ " AND s.tablename OPERATOR(pg_catalog.=) c.relname\n"
+ " AND s.attname OPERATOR(pg_catalog.=) a.attname\n"
+ " AND s.inherited))\n");
/* inheritance and extended stats */
appendPQExpBufferStr(&catalog_query,
" OR EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext e\n"
+ " JOIN pg_catalog.pg_namespace en"
+ " ON en.oid OPERATOR(pg_catalog.=) e.stxnamespace\n"
" WHERE e.stxrelid OPERATOR(pg_catalog.=) c.oid\n"
" AND e.stxstattarget IS DISTINCT FROM 0::pg_catalog.int2\n"
" AND c.relhassubclass\n"
" AND NOT p.inherited\n"
+ " AND pg_catalog.pg_has_role(c.relowner, 'USAGE'::text)\n"
+ " AND (c.relrowsecurity OPERATOR(pg_catalog.=) false\n"
+ " OR NOT pg_catalog.row_security_active(c.oid))\n"
" AND EXISTS (SELECT NULL FROM pg_catalog.pg_inherits h\n"
" WHERE h.inhparent OPERATOR(pg_catalog.=) c.oid)\n"
- " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_statistic_ext_data d\n"
- " WHERE d.stxoid OPERATOR(pg_catalog.=) e.oid\n"
- " AND d.stxdinherit))\n");
+ " AND NOT EXISTS (SELECT NULL FROM pg_catalog.pg_stats_ext d\n"
+ " WHERE d.statistics_schemaname OPERATOR(pg_catalog.=) en.nspname\n"
+ " AND d.statistics_name OPERATOR(pg_catalog.=) e.stxname\n"
+ " AND d.inherited))\n");
appendPQExpBufferStr(&catalog_query, " )\n");
}
--
2.39.5 (Apple Git-154)
On Fri, Aug 22, 2025 at 3:34 PM Nathan Bossart <nathandbossart@gmail.com>
wrote:
On Fri, Aug 22, 2025 at 09:01:36AM -0500, Nathan Bossart wrote:
We'll also need documentation and test updates, of course.
Here is an attempt at the docs/tests. I also fixed a couple of small
issues in the query.--
nathan
Good catch on the query change. I like the docs change.
Tests look pretty straightforward. Was expecting one of them to test on a
WARNING or not, but that might not be reliable enough to capture.
+1
On Sat, Aug 23, 2025 at 8:15 AM Corey Huinker <corey.huinker@gmail.com> wrote:
On Fri, Aug 22, 2025 at 3:34 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Fri, Aug 22, 2025 at 09:01:36AM -0500, Nathan Bossart wrote:
We'll also need documentation and test updates, of course.
Here is an attempt at the docs/tests. I also fixed a couple of small
issues in the query.--
nathanGood catch on the query change. I like the docs change.
Tests look pretty straightforward. Was expecting one of them to test on a WARNING or not, but that might not be reliable enough to capture.
+1
Thanks for working on this!
I tested by creating many tables with make installcheck and running
vacuumdb --missing-stats-only on the regression database.
Without the patch, the query to find tables to analyze took about 60 ms,
but with the patch it took 18 seconds. That seems too slow,
so probably we'll need to tune the query?
Regards,
--
Fujii Masao
On Sat, Aug 23, 2025 at 10:59:43AM +0900, Fujii Masao wrote:
I tested by creating many tables with make installcheck and running
vacuumdb --missing-stats-only on the regression database.
Without the patch, the query to find tables to analyze took about 60 ms,
but with the patch it took 18 seconds. That seems too slow,
so probably we'll need to tune the query?
Hm. Maybe we should just document that the option requires SELECT
privileges on pg_statistic and pg_statistic_ext_data (which are restricted
to superusers by default). I suspect we have relatively limited
opportunities for tuning the query, and I'd like to avoid invasive changes
to v18 at this point.
--
nathan
On Sat, Aug 23, 2025 at 12:00 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
On Sat, Aug 23, 2025 at 10:59:43AM +0900, Fujii Masao wrote:
I tested by creating many tables with make installcheck and running
vacuumdb --missing-stats-only on the regression database.
Without the patch, the query to find tables to analyze took about 60 ms,
but with the patch it took 18 seconds. That seems too slow,
so probably we'll need to tune the query?Hm. Maybe we should just document that the option requires SELECT
privileges on pg_statistic and pg_statistic_ext_data (which are restricted
to superusers by default). I suspect we have relatively limited
opportunities for tuning the query, and I'd like to avoid invasive changes
to v18 at this point.
Yeah, adding a note about the permissions required for --missing-stats-only,
leaving the query unchanged in v18, and revisiting the issue in v19 seems
reasonable given the limited time before the v18 release.
Regards,
--
Fujii Masao
On Fri, Aug 22, 2025 at 11:20 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 23, 2025 at 12:00 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:On Sat, Aug 23, 2025 at 10:59:43AM +0900, Fujii Masao wrote:
I tested by creating many tables with make installcheck and running
vacuumdb --missing-stats-only on the regression database.
Without the patch, the query to find tables to analyze took about 60ms,
but with the patch it took 18 seconds. That seems too slow,
so probably we'll need to tune the query?
Sounds exactly like the issue we encountered with queries that used
pg_stats in pg_dump.
Hm. Maybe we should just document that the option requires SELECT
privileges on pg_statistic and pg_statistic_ext_data (which arerestricted
to superusers by default). I suspect we have relatively limited
opportunities for tuning the query, and I'd like to avoid invasivechanges
to v18 at this point.
Yeah, adding a note about the permissions required for
--missing-stats-only,
leaving the query unchanged in v18, and revisiting the issue in v19 seems
reasonable given the limited time before the v18 release.
Rather than resorting to the redundant where-clause trick that we did in
pg_dump.
Looking to v19, I think the problem could be solved if we exposed starelid
in pg_stats and stxrelid in pg_stats_ext and joined on those. pg_dump would
benefit from that as well, eventually.
On Sat, Aug 23, 2025 at 05:32:30AM -0400, Corey Huinker wrote:
On Fri, Aug 22, 2025 at 11:20 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 23, 2025 at 12:00 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
Hm. Maybe we should just document that the option requires SELECT
privileges on pg_statistic and pg_statistic_ext_data (which are restricted
to superusers by default). I suspect we have relatively limited
opportunities for tuning the query, and I'd like to avoid invasive changes
to v18 at this point.Yeah, adding a note about the permissions required for --missing-stats-only,
leaving the query unchanged in v18, and revisiting the issue in v19 seems
reasonable given the limited time before the v18 release.Rather than resorting to the redundant where-clause trick that we did in
pg_dump.
Here's a patch for the documentation update.
--
nathan
Attachments:
v4-0001-doc-Note-privileges-required-for-vacuumdb-missing.patchtext/plain; charset=us-asciiDownload
From 9996ef5e8e875cfbea641545bd615deb98654816 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Sat, 23 Aug 2025 09:33:53 -0500
Subject: [PATCH v4 1/1] doc: Note privileges required for vacuumdb
--missing-stats-only.
---
doc/src/sgml/ref/vacuumdb.sgml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 53147480515..84c76d7350c 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -292,6 +292,14 @@ PostgreSQL documentation
This option can only be used in conjunction with
<option>--analyze-only</option> or <option>--analyze-in-stages</option>.
</para>
+ <para>
+ Note that <option>--missing-stats-only</option> requires
+ <literal>SELECT</literal> privileges on
+ <link linkend="catalog-pg-statistic"><structname>pg_statistic</structname></link>
+ and
+ <link linkend="catalog-pg-statistic-ext-data"><structname>pg_statistic_ext_data</structname></link>,
+ which are restricted to superusers by default.
+ </para>
</listitem>
</varlistentry>
--
2.39.5 (Apple Git-154)
On Sat, 23 Aug 2025 09:36:07 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:
On Sat, Aug 23, 2025 at 05:32:30AM -0400, Corey Huinker wrote:
On Fri, Aug 22, 2025 at 11:20 PM Fujii Masao <masao.fujii@gmail.com> wrote:
On Sat, Aug 23, 2025 at 12:00 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
Hm. Maybe we should just document that the option requires SELECT
privileges on pg_statistic and pg_statistic_ext_data (which are restricted
to superusers by default). I suspect we have relatively limited
opportunities for tuning the query, and I'd like to avoid invasive changes
to v18 at this point.Yeah, adding a note about the permissions required for --missing-stats-only,
leaving the query unchanged in v18, and revisiting the issue in v19 seems
reasonable given the limited time before the v18 release.Rather than resorting to the redundant where-clause trick that we did in
pg_dump.Here's a patch for the documentation update.
The documentation fix looks good to me. However, it’s not very user-friendly that,
when the user lacks the required privileges, an error from the internal query is
raised. Instead, how about checking whether the user has the necessary privileges
and printing an appropriate message if any privilege is missing?
I've added the 0002 patch to address this; the 0001 is the same as the previous.
The check is performed per database, so it may be a bit slower when an enormous
number of databases are processed, but the overhead should be relatively small compared
with the other queries executed together.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
Attachments:
v5-0002-vacuumdb-check-SELECT-privilege-on-pg_statitis-or.patchtext/x-diff; name=v5-0002-vacuumdb-check-SELECT-privilege-on-pg_statitis-or.patchDownload
From 7e86930ede6ccc9294c14c748adba068e6e88ecf Mon Sep 17 00:00:00 2001
From: Yugo Nagata <nagata@sraoss.co.jp>
Date: Mon, 25 Aug 2025 06:18:04 +0900
Subject: [PATCH v5 2/2] vacuumdb: check SELECT privilege on pg_statitis or
pg_statistic_ext_data
---
src/bin/scripts/vacuumdb.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index fd236087e90..2087b4131a2 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -801,6 +801,30 @@ retrieve_objects(PGconn *conn, vacuumingOptions *vacopts,
SimpleStringList *found_objs = palloc0(sizeof(SimpleStringList));
bool objects_listed = false;
+ if (vacopts->missing_stats_only)
+ {
+ PQExpBufferData aclcheck;
+
+ initPQExpBuffer(&aclcheck);
+ appendPQExpBufferStr(&aclcheck,
+ "SELECT has_table_privilege('pg_catalog.pg_statistic', 'select'), "
+ "has_table_privilege('pg_catalog.pg_statistic_ext_data', 'select')");
+ res = executeQuery(conn, aclcheck.data, echo);
+ termPQExpBuffer(&aclcheck);
+ if (strcmp(PQgetvalue(res, 0, 0), "t") != 0)
+ {
+ PQfinish(conn);
+ pg_fatal("--missing-stats-only requires SELECT privileges on pg_statistic");
+ }
+ else if (strcmp(PQgetvalue(res, 0, 1), "t") != 0)
+ {
+ PQfinish(conn);
+ pg_fatal("--missing-stats-only requires SELECT privileges on pg_statistic_ext_data");
+ }
+
+ termPQExpBuffer(&aclcheck);
+ }
+
initPQExpBuffer(&catalog_query);
for (cell = objects ? objects->head : NULL; cell; cell = cell->next)
{
--
2.43.0
v5-0001-doc-Note-privileges-required-for-vacuumdb-missing.patchtext/x-diff; name=v5-0001-doc-Note-privileges-required-for-vacuumdb-missing.patchDownload
From 3ce399cb775e7f3a6f0b0a46959f9a3bc67ad7f2 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Sat, 23 Aug 2025 09:33:53 -0500
Subject: [PATCH v5 1/2] doc: Note privileges required for vacuumdb
--missing-stats-only.
---
doc/src/sgml/ref/vacuumdb.sgml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/doc/src/sgml/ref/vacuumdb.sgml b/doc/src/sgml/ref/vacuumdb.sgml
index 53147480515..84c76d7350c 100644
--- a/doc/src/sgml/ref/vacuumdb.sgml
+++ b/doc/src/sgml/ref/vacuumdb.sgml
@@ -292,6 +292,14 @@ PostgreSQL documentation
This option can only be used in conjunction with
<option>--analyze-only</option> or <option>--analyze-in-stages</option>.
</para>
+ <para>
+ Note that <option>--missing-stats-only</option> requires
+ <literal>SELECT</literal> privileges on
+ <link linkend="catalog-pg-statistic"><structname>pg_statistic</structname></link>
+ and
+ <link linkend="catalog-pg-statistic-ext-data"><structname>pg_statistic_ext_data</structname></link>,
+ which are restricted to superusers by default.
+ </para>
</listitem>
</varlistentry>
--
2.43.0
On Mon, Aug 25, 2025 at 10:30:32AM +0900, Yugo Nagata wrote:
The documentation fix looks good to me. However, it’s not very user-friendly that,
when the user lacks the required privileges, an error from the internal query is
raised. Instead, how about checking whether the user has the necessary privileges
and printing an appropriate message if any privilege is missing?
Thanks for taking a look. I appreciate your suggestion, but I'm finding
myself leaning -1 for a couple of reasons:
* There's no precedent for this in vacuumdb. In fact, if the user
specifies a nonexistent table, we return the error from the internal
query, and if the user lacks privileges to VACUUM or ANALYZE a table, we
let the command emit a WARNING and skip it. Intentionally or not, we
seem to let the server do most of the work when it comes to this sort of
thing. vacuumdb is just responsible for building and sending the
commands.
* I'm a little worried that warning about SELECT privileges on these
catalogs will encourage people to grant privileges on them, which we
probably don't want them to do. My proposed documentation note already
carries some risk of this, but it at least specifically calls out that
superusers should have the necessary privileges.
* While probably rare, checking the privileges beforehand introduces race
conditions that would cause the internal query error, anyway. I think
there are already a few such race conditions already (e.g., if the table
is dropped between the catalog query and the VACUUM or ANALYZE command),
which AFAICT we just live with, but I'm wary of pressing our luck too
much in this area.
I'd be grateful for others' thoughts on this. I'm hoping to resolve this
open item within the next couple of days, given 18rc1 is planned for next
week.
--
nathan
On Mon, 25 Aug 2025 09:29:15 -0500
Nathan Bossart <nathandbossart@gmail.com> wrote:
On Mon, Aug 25, 2025 at 10:30:32AM +0900, Yugo Nagata wrote:
The documentation fix looks good to me. However, it’s not very user-friendly that,
when the user lacks the required privileges, an error from the internal query is
raised. Instead, how about checking whether the user has the necessary privileges
and printing an appropriate message if any privilege is missing?Thanks for taking a look. I appreciate your suggestion, but I'm finding
myself leaning -1 for a couple of reasons:* There's no precedent for this in vacuumdb. In fact, if the user
specifies a nonexistent table, we return the error from the internal
query, and if the user lacks privileges to VACUUM or ANALYZE a table, we
let the command emit a WARNING and skip it. Intentionally or not, we
seem to let the server do most of the work when it comes to this sort of
thing. vacuumdb is just responsible for building and sending the
commands.* I'm a little worried that warning about SELECT privileges on these
catalogs will encourage people to grant privileges on them, which we
probably don't want them to do. My proposed documentation note already
carries some risk of this, but it at least specifically calls out that
superusers should have the necessary privileges.* While probably rare, checking the privileges beforehand introduces race
conditions that would cause the internal query error, anyway. I think
there are already a few such race conditions already (e.g., if the table
is dropped between the catalog query and the VACUUM or ANALYZE command),
which AFAICT we just live with, but I'm wary of pressing our luck too
much in this area.
Thank you for the clarification. I understand your points now,
so I'll withdraw my proposal.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata@sraoss.co.jp>
On Tue, Aug 26, 2025 at 12:24:27AM +0900, Yugo Nagata wrote:
Thank you for the clarification. I understand your points now,
so I'll withdraw my proposal.
Okay. I'll plan on committing the documentation update in the next 24-48
hours, provided no additional feedback materializes.
--
nathan
On Tue, Aug 26, 2025 at 1:20 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Aug 26, 2025 at 12:24:27AM +0900, Yugo Nagata wrote:
Thank you for the clarification. I understand your points now,
so I'll withdraw my proposal.Okay. I'll plan on committing the documentation update in the next 24-48
hours, provided no additional feedback materializes.
The patch looks good to me. Thanks!
Regards,
--
Fujii Masao
On Tue, Aug 26, 2025 at 09:00:53PM +0900, Fujii Masao wrote:
On Tue, Aug 26, 2025 at 1:20 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
Okay. I'll plan on committing the documentation update in the next 24-48
hours, provided no additional feedback materializes.The patch looks good to me. Thanks!
Committed.
--
nathan