Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 24 Sep 2024 10:08:43 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Christophe Pettus <xof@thebuild.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	vaibhave postgres <postgresvaibhave@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	Fujii Masao <masao.fujii@oss.nttdata.com>,
	Noah Misch <noah@leadboat.com>, pgsql-bugs@lists.postgresql.org,
	vsekar@microsoft.com
Subject: Re: vacuumdb: permission denied for schema "pg_temp_7"
Message-ID: <ZvIRG6282T33q3Ku@paquier.xyz>
References: 
 <CAM_eQjwfAR=y3G1fGyS1U9FTmc+FyJm9amNfY2QCZBnDDbNPZg@mail.gmail.com>
 <20240920190731.37.nmisch@google.com>
 <Zu3iMzfiGBTbg3iy@nathan>
 <20240920215030.8d.nmisch@google.com>
 <2603142.1726873660@sss.pgh.pa.us>
 <5f37ee56-14fb-44fe-9150-9eb97e10538b@oss.nttdata.com>
 <2649165.1726898595@sss.pgh.pa.us>
 <CAM_eQjyWnu_LUt6JWjh3boNFHAT9DBrcbamhG3Xy39Q4Ph=2rg@mail.gmail.com>
 <ZvGye19tHrhLCpFl@nathan>
 <E9660D68-5B26-488E-9AB2-2CA1E8EDDDA1@thebuild.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Fdh9MWMOUSwLFW/E"
Content-Disposition: inline
In-Reply-To: <E9660D68-5B26-488E-9AB2-2CA1E8EDDDA1@thebuild.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZvIRG6282T33q3Ku%40paquier.xyz>
Precedence: bulk


--Fdh9MWMOUSwLFW/E
Content-Type: multipart/mixed; boundary="d4nbwX7/OWSDUVul"
Content-Disposition: inline


--d4nbwX7/OWSDUVul
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--d4nbwX7/OWSDUVul
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-vacuumdb-Skip-temporary-tables-in-filtering-query.patch"
Content-Transfer-Encoding: quoted-printable

=46rom a234eb7a0ad24a9d5641168346ce608515f484c2 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Tue, 24 Sep 2024 10:00:29 +0900
Subject: [PATCH v2] vacuumdb: Skip temporary tables in filtering query

Running vacuumdb with a non-superuser when another user has created a
temporary table would lead to a mid-flight failure.

There are other issues related to schema-level USAGE, which will be
tackled later.

Author: VaibhaveS
Discussion: https://postgr.es/m/CAM_eQjwfAR=3Dy3G1fGyS1U9FTmc+FyJm9amNfY2QC=
ZBnDDbNPZg@mail.gmail.com
Backpatch-through: 12
---
 src/bin/scripts/vacuumdb.c | 40 ++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7c33e13e1a..249622068a 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -491,7 +491,6 @@ vacuum_one_database(ConnParams *cparams,
 	int			ntups;
 	bool		failed =3D false;
 	bool		objects_listed =3D false;
-	bool		has_where =3D false;
 	const char *initcmd;
 	const char *stage_commands[] =3D {
 		"SET default_statistics_target=3D1; SET vacuum_cost_delay=3D0;",
@@ -665,7 +664,10 @@ vacuum_one_database(ConnParams *cparams,
 						 " LEFT JOIN pg_catalog.pg_class t"
 						 " ON c.reltoastrelid OPERATOR(pg_catalog.=3D) t.oid\n");
=20
-	/* Used to match the tables or schemas listed by the user */
+	/*
+	 * Used to match the tables or schemas listed by the user, completing the
+	 * JOIN clause.
+	 */
 	if (objects_listed)
 	{
 		appendPQExpBufferStr(&catalog_query, " LEFT JOIN listed_objects"
@@ -676,14 +678,26 @@ vacuum_one_database(ConnParams *cparams,
 			appendPQExpBufferStr(&catalog_query, "c.oid\n");
 		else
 			appendPQExpBufferStr(&catalog_query, "ns.oid\n");
+	}
=20
+	/*
+	 * Exclude temporary tables, beginning the WHERE clause.
+	 */
+	appendPQExpBufferStr(&catalog_query,
+						 " WHERE c.relpersistence !=3D " CppAsString2(RELPERSISTENCE_TEMP));
+
+	/*
+	 * Used to match the tables or schemas listed by the user, for the WHERE
+	 * clause.
+	 */
+	if (objects_listed)
+	{
 		if (objfilter & OBJFILTER_SCHEMA_EXCLUDE)
 			appendPQExpBuffer(&catalog_query,
-							  " WHERE listed_objects.object_oid IS NULL\n");
+							  " AND listed_objects.object_oid IS NULL\n");
 		else
 			appendPQExpBuffer(&catalog_query,
-							  " WHERE listed_objects.object_oid IS NOT NULL\n");
-		has_where =3D true;
+							  " AND listed_objects.object_oid IS NOT NULL\n");
 	}
=20
 	/*
@@ -695,11 +709,9 @@ vacuum_one_database(ConnParams *cparams,
 	if ((objfilter & OBJFILTER_TABLE) =3D=3D 0)
 	{
 		appendPQExpBuffer(&catalog_query,
-						  " %s c.relkind OPERATOR(pg_catalog.=3D) ANY (array["
+						  " AND c.relkind OPERATOR(pg_catalog.=3D) ANY (array["
 						  CppAsString2(RELKIND_RELATION) ", "
-						  CppAsString2(RELKIND_MATVIEW) "])\n",
-						  has_where ? "AND" : "WHERE");
-		has_where =3D true;
+						  CppAsString2(RELKIND_MATVIEW) "])\n");
 	}
=20
 	/*
@@ -712,25 +724,23 @@ vacuum_one_database(ConnParams *cparams,
 	if (vacopts->min_xid_age !=3D 0)
 	{
 		appendPQExpBuffer(&catalog_query,
-						  " %s GREATEST(pg_catalog.age(c.relfrozenxid),"
+						  " AND GREATEST(pg_catalog.age(c.relfrozenxid),"
 						  " pg_catalog.age(t.relfrozenxid)) "
 						  " OPERATOR(pg_catalog.>=3D) '%d'::pg_catalog.int4\n"
 						  " AND c.relfrozenxid OPERATOR(pg_catalog.!=3D)"
 						  " '0'::pg_catalog.xid\n",
-						  has_where ? "AND" : "WHERE", vacopts->min_xid_age);
-		has_where =3D true;
+						  vacopts->min_xid_age);
 	}
=20
 	if (vacopts->min_mxid_age !=3D 0)
 	{
 		appendPQExpBuffer(&catalog_query,
-						  " %s GREATEST(pg_catalog.mxid_age(c.relminmxid),"
+						  " AND GREATEST(pg_catalog.mxid_age(c.relminmxid),"
 						  " pg_catalog.mxid_age(t.relminmxid)) OPERATOR(pg_catalog.>=3D)"
 						  " '%d'::pg_catalog.int4\n"
 						  " AND c.relminmxid OPERATOR(pg_catalog.!=3D)"
 						  " '0'::pg_catalog.xid\n",
-						  has_where ? "AND" : "WHERE", vacopts->min_mxid_age);
-		has_where =3D true;
+						  vacopts->min_mxid_age);
 	}
=20
 	/*
--=20
2.45.2


--d4nbwX7/OWSDUVul--

--Fdh9MWMOUSwLFW/E
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbyERsACgkQnvQgOdby
QH02Ww/+Otwswoicguiy8MhfkWINN/tK8koavsN2nXjvi3haKRsBjjuvtrMxsOT7
xwVugJWqkdGDoc7KkKbvi/rXg8EIGl1GaZAsGds8VqmE7V765XKjZbL2mL3o9AgF
79VZ2Sqg72fnhXMD/PiF9/VJqpXrYrbyigOFsKjLkUEtNZYr1gEdroYvcO75ley2
rAoJil/1ZgRp/O1b2TlHIBOuimvfDkiNWMRq49iL+QhSa7i5fEus3/8/5bL4EgDn
w0WKz6yDk9t43hhI5e2AINvG+wvDYs0FhzVotPkejy90vMve/a6+Fg5Po892Iygp
FZKA8UhkfZD4cFsCqtaiVXPMcUMmS9OpL0puGI4grX5iHZzlPqJA2Otee6N09rqI
hDu69lGxu0DjA28K4DirjEBfhB4mT3r41sYbQZgYPrRTC1/kOzRrJ/QJBFTaqWZS
mT8gLDeKRm8h1NJp2hDihyEYy5pG0gb66QYfKottiyEpdPTfD1Ek5yN089BTS6E9
zJ4oJsRNuB6S52mK4FgoxncQQjH/BjNXi9bdJ/l8h2ddS0rtIdwMoTp7b575ueeo
lG+ImsUstLfqFkEcZ+FGezTw3plyQ6a0Z2eLlrb4wX6xh/a9iB00OwtLFXAo9T1B
07KR/m+1YjBnvqQ6h8cA6SUWGwDRR1fK1oyDjEHaIYjmGfn14QI=
=AhGb
-----END PGP SIGNATURE-----

--Fdh9MWMOUSwLFW/E--