Thread: vacuumdb: permission denied for schema "pg_temp_7"
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
Suggested Patch is attached
Attachment
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.
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
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.
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
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
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
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
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
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
> 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.
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.
> 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 anydatabase that it finds in the initial query. This can happen if pg_hba.conf denies the user that is running vacuumdbaccess to a database that comes up during --all. Some hosting providers (in particular, Google) create restricteddatabases 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 awarning if further connections fail. If it seems reasonable, I'm happy to do it in a separate patch.
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.
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
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
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
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
> 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 determinewhich databases are in the instance, it stops with a fatal error just as it does now.
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
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");
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
On Mon, Oct 07, 2024 at 02:54:18PM -0500, Nathan Bossart wrote: > I'll just go apply the patch now, then. Committed. -- nathan