Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. vacuumdb: Schema-qualify operator in catalog query's WHERE clause.

  2. reindexdb: Skip reindexing temporary tables and indexes.

  3. vacuumdb: Skip temporary tables in query to build list of relations

  4. Use catalog query to discover tables to process in vacuumdb

  1. vacuumdb: permission denied for schema "pg_temp_7"

    vaibhave postgres <postgresvaibhave@gmail.com> — 2024-07-06T11:49:39Z

    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.

Suggested Patch is attached

  2. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Noah Misch <noah@leadboat.com> — 2024-09-20T19:07:31Z

    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.

  3. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-09-20T20:59:32Z

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

> Thanks for the report.

+1

[0] https://postgr.es/m/flat/56596b81-088f-4c0c-9a88-b5f27a7a62e9%40oss.nttdata.com

-- 
nathan

  4. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Noah Misch <noah@leadboat.com> — 2024-09-20T21:50:30Z

    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] https://postgr.es/m/flat/56596b81-088f-4c0c-9a88-b5f27a7a62e9%40oss.nttdata.com

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

  5. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-20T23:07:40Z

    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

  6. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-09-21T05:56:22Z

    
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

  7. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-09-21T05:59:51Z

    
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

  8. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-21T06:03:15Z

    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

  9. Re: vacuumdb: permission denied for schema "pg_temp_7"

    vaibhave postgres <postgresvaibhave@gmail.com> — 2024-09-21T06:12:19Z

    Thanks for the review and feedback.

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

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

  10. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-09-23T18:24:59Z

    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

  11. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Christophe Pettus <xof@thebuild.com> — 2024-09-23T18:48:21Z

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

  12. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-09-24T01:08:43Z

    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

  13. Re: vacuumdb: permission denied for schema "pg_temp_7"

    vaibhave postgres <postgresvaibhave@gmail.com> — 2024-09-24T01:09:53Z

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

  14. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Christophe Pettus <xof@thebuild.com> — 2024-09-24T01:13:23Z

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

  15. Re: vacuumdb: permission denied for schema "pg_temp_7"

    vaibhave postgres <postgresvaibhave@gmail.com> — 2024-09-24T01:35:41Z

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

  16. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-09-24T01:56:27Z

    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

  17. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-09-24T14:20:43Z

    
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

  18. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-09-24T14:26:21Z

    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].  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] https://postgr.es/m/Zu3iMzfiGBTbg3iy%40nathan

-- 
nathan

  19. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-09-24T14:30:21Z

    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

  20. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-09-24T14:49:12Z

    
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

  21. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-09-24T14:51:14Z

    On Tue, Sep 24, 2024 at 11:49:12PM +0900, Fujii Masao wrote:
> 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.

I'm okay with that approach.

-- 
nathan

  22. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Christophe Pettus <xof@thebuild.com> — 2024-09-24T15:22:32Z

    
> On Sep 24, 2024, at 07:30, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 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.

Yes, that's essentially my proposal.  If --all is not specified, or if it cannot make the initial connection to determine which databases are in the instance, it stops with a fatal error just as it does now.

  23. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-09-24T23:10:16Z

    On Tue, Sep 24, 2024 at 11:20:43PM +0900, Fujii Masao wrote:
> On 2024/09/24 10:08, Michael Paquier wrote:
>> 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.

Thanks.  As I am kind of behind this one, I'll go fix it first.  Let's
sort out the permission bits after that one is sorted out.  REL_17_0
is out, so this can happen across all branches.
--
Michael

  24. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-09-24T23:20:22Z

    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.
> 
> Sorry if I misunderstood your point.

I don't really have a strong opinion in favor of skipping the objects
or fail hard, because both have arguments.  My point was just that, if
we were to skip the objects, then the query needs a bit of rework and
a SELECT with some extra filters in the CTE felt kind of right.  If
the consensus is that errors are better, my point about rewriting the
query is most probably moot.

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

Missed your point.  An extra filter based on relpersistence can indeed
make sense for this path.
--
Michael

  25. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-09-24T23:29:32Z

    On Tue, Sep 24, 2024 at 09:51:14AM -0500, Nathan Bossart wrote:
> On Tue, Sep 24, 2024 at 11:49:12PM +0900, Fujii Masao wrote:
>> 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.
> 
> I'm okay with that approach.

Okay.  I'd be on board with a new option to control the filtering.
The lack of complaints regarding vacuumdb failing with a non-superuser
makes it rather OK to still fail by default.
--
Michael

  26. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-09-26T14:19:33Z

    
On 2024/09/25 8:20, Michael Paquier wrote:
>> We should probably add a condition like "relpersistence != CppAsString2(RELPERSISTENCE_TEMP)"
>> to the queries in get_parallel_object_list().
> 
> Missed your point.  An extra filter based on relpersistence can indeed
> make sense for this path.

Patch attached.

Should reindexdb skip temporary tables or indexes even when specified explicitly
with the -t or -i options? Currently, the patch doesn't change this behavior;
reindexdb will still not skip them if specified.

If we agree to back-patch, it should be applied to v13, as parallel mode was
introduced in that version.

Regards,

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

  27. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-09-27T00:48:56Z

    On Thu, Sep 26, 2024 at 11:19:33PM +0900, Fujii Masao wrote:
> Should reindexdb skip temporary tables or indexes even when specified explicitly
> with the -t or -i options? Currently, the patch doesn't change this behavior;
> reindexdb will still not skip them if specified.
> 
> If we agree to back-patch, it should be applied to v13, as parallel mode was
> introduced in that version.

Looks sensible to me to apply that to the database and schema queries.
Thanks for the patch.
--
Michael

  28. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2024-09-30T17:07:18Z

    
On 2024/09/27 9:48, Michael Paquier wrote:
> On Thu, Sep 26, 2024 at 11:19:33PM +0900, Fujii Masao wrote:
>> Should reindexdb skip temporary tables or indexes even when specified explicitly
>> with the -t or -i options? Currently, the patch doesn't change this behavior;
>> reindexdb will still not skip them if specified.
>>
>> If we agree to back-patch, it should be applied to v13, as parallel mode was
>> introduced in that version.
> 
> Looks sensible to me to apply that to the database and schema queries.
> Thanks for the patch.

Pushed. Thanks for the review!

Regards,

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

  29. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-10-07T16:16:33Z

    Regarding commit 1ab67c9...

On Wed, Sep 25, 2024 at 08:10:16AM +0900, Michael Paquier wrote:
> Thanks.  As I am kind of behind this one, I'll go fix it first.  Let's
> sort out the permission bits after that one is sorted out.  REL_17_0
> is out, so this can happen across all branches.

For consistency with the surrounding code, I think we should schema-qualify
the operator and add a newline after "WHERE relpersistence != 't'".  If
folks agree, I can handle committing the attached patch.

-- 
nathan

  30. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Noah Misch <noah@leadboat.com> — 2024-10-07T19:40:54Z

    On Mon, Oct 07, 2024 at 11:16:33AM -0500, Nathan Bossart wrote:
> Regarding commit 1ab67c9...
> 
> On Wed, Sep 25, 2024 at 08:10:16AM +0900, Michael Paquier wrote:
> > Thanks.  As I am kind of behind this one, I'll go fix it first.  Let's
> > sort out the permission bits after that one is sorted out.  REL_17_0
> > is out, so this can happen across all branches.
> 
> For consistency with the surrounding code, I think we should schema-qualify
> the operator and add a newline after "WHERE relpersistence != 't'".  If
> folks agree, I can handle committing the attached patch.

Not just code consistency.  A code comment requires the schema qualification:

	 * Since we execute the constructed query with the default search_path
	 * (which could be unsafe), everything in this query MUST be fully
	 * qualified.

> --- a/src/bin/scripts/vacuumdb.c
> +++ b/src/bin/scripts/vacuumdb.c
> @@ -684,7 +684,8 @@ vacuum_one_database(ConnParams *cparams,
>  	 * Exclude temporary tables, beginning the WHERE clause.
>  	 */
>  	appendPQExpBufferStr(&catalog_query,
> -						 " WHERE c.relpersistence != " CppAsString2(RELPERSISTENCE_TEMP));
> +						 " WHERE c.relpersistence OPERATOR(pg_catalog.!=) "
> +						 CppAsString2(RELPERSISTENCE_TEMP) "\n");

  31. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-10-07T19:54:18Z

    On Mon, Oct 07, 2024 at 12:40:54PM -0700, Noah Misch wrote:
> On Mon, Oct 07, 2024 at 11:16:33AM -0500, Nathan Bossart wrote:
>> For consistency with the surrounding code, I think we should schema-qualify
>> the operator and add a newline after "WHERE relpersistence != 't'".  If
>> folks agree, I can handle committing the attached patch.
> 
> Not just code consistency.  A code comment requires the schema qualification:
> 
> 	 * Since we execute the constructed query with the default search_path
> 	 * (which could be unsafe), everything in this query MUST be fully
> 	 * qualified.

D'oh.  I'm listed as the author of the commit that added that comment and
should've remembered it.  I'll just go apply the patch now, then.

-- 
nathan

  32. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Nathan Bossart <nathandbossart@gmail.com> — 2024-10-07T21:51:14Z

    On Mon, Oct 07, 2024 at 02:54:18PM -0500, Nathan Bossart wrote:
> I'll just go apply the patch now, then.

Committed.

-- 
nathan

  33. Re: vacuumdb: permission denied for schema "pg_temp_7"

    Michael Paquier <michael@paquier.xyz> — 2024-10-07T23:22:31Z

    On Mon, Oct 07, 2024 at 04:51:14PM -0500, Nathan Bossart wrote:
> Committed.

Oops.  Thanks!
--
Michael