Horton

Thread

Commits

  1. Document privileges required for vacuumdb --missing-stats-only.

  1. vacuumdb --missing-stats-only and permission issue

    Fujii Masao <masao.fujii@gmail.com> — 2025-08-21T03:06:32Z

    Hi,

While following the discussion on vacuumdb --missing-stats-only [1],
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] https://postgr.es/m/20250820104226.8ba51e43164cd590b863ce41@sraoss.co.jp

-- 
Fujii Masao

  2. Re: vacuumdb --missing-stats-only and permission issue

    Corey Huinker <corey.huinker@gmail.com> — 2025-08-21T07:19:40Z

    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.

  3. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-21T13:44:01Z

    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

  4. Re: vacuumdb --missing-stats-only and permission issue

    Corey Huinker <corey.huinker@gmail.com> — 2025-08-21T16:52:17Z

    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.

  5. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-21T17:59:52Z

    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

  6. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-21T18:07:10Z

    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

  7. Re: vacuumdb --missing-stats-only and permission issue

    Corey Huinker <corey.huinker@gmail.com> — 2025-08-21T23:27:23Z

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

  8. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-22T14:01:36Z

    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

  9. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-22T19:34:00Z

    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

  10. Re: vacuumdb --missing-stats-only and permission issue

    Corey Huinker <corey.huinker@gmail.com> — 2025-08-22T23:15:35Z

    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

  11. Re: vacuumdb --missing-stats-only and permission issue

    Fujii Masao <masao.fujii@gmail.com> — 2025-08-23T01:59:43Z

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

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

  12. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-23T03:00:06Z

    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

  13. Re: vacuumdb --missing-stats-only and permission issue

    Fujii Masao <masao.fujii@gmail.com> — 2025-08-23T03:20:06Z

    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

  14. Re: vacuumdb --missing-stats-only and permission issue

    Corey Huinker <corey.huinker@gmail.com> — 2025-08-23T09:32:30Z

    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 60
> ms,
> > > 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 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.

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.

  15. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-23T14:36:07Z

    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

  16. Re: vacuumdb --missing-stats-only and permission issue

    Yugo Nagata <nagata@sraoss.co.jp> — 2025-08-25T01:30:32Z

    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>

  17. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-25T14:29:15Z

    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

  18. Re: vacuumdb --missing-stats-only and permission issue

    Yugo Nagata <nagata@sraoss.co.jp> — 2025-08-25T15:24:27Z

    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>

  19. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-25T16:20:50Z

    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

  20. Re: vacuumdb --missing-stats-only and permission issue

    Fujii Masao <masao.fujii@gmail.com> — 2025-08-26T12:00:53Z

    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

  21. Re: vacuumdb --missing-stats-only and permission issue

    Nathan Bossart <nathandbossart@gmail.com> — 2025-08-26T19:52:28Z

    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