From: Tom Lane <tgl@sss.pgh.pa.us>
To: Laurenz Albe <laurenz.albe@cybertec.at>
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: Commit 5a2fed911a broke parallel query
In-reply-to: <1349368.1734968136@sss.pgh.pa.us>
References: <8befc845430ba1ae3748af900af298788e579c89.camel@cybertec.at>
 <1349368.1734968136@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 23 Dec 2024 10:35:36 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1408434.1734973939.0@sss.pgh.pa.us>
Date: Mon, 23 Dec 2024 12:12:53 -0500
Message-ID: <1408486.1734973973@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1408486.1734973973%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1408434.1734973939.1@sss.pgh.pa.us>

I wrote:
> In the security-team discussion that led up to 73c9f91a1, I'd been
> wondering why parallel worker start enforces any connection
> restrictions at all.  If we'd like the use of parallelism to be
> more-or-less transparent, we shouldn't apply these checks,
> and not the !am_superuser ones in InitPostgres either.

That is, the way I'd prefer to attack this is something along the
lines of the attached, which just disables all those checks in
parallel workers (and reverts 73c9f91a1's hackery on am_superuser).

One big question this raises is whether any other sorts of background
processes should be treated similarly.

Another loose end is that I don't think the enforcement of
datconnlimit is very sane.  CountDBBackends skips processes that say
AmBackgroundWorkerProcess, so logically those should be precisely the
ones that escape enforcement of datconnlimit, but it doesn't quite
work like that today.  This patch at least puts parallel workers on
the right side of the line; but I think we have some cleanup to do
for other sorts of auxiliary processes.

I'm also wondering if we could revisit the need for
CheckMyDatabase's override_allow_connections parameter
and thereby remove INIT_PG_OVERRIDE_ALLOW_CONNS.

So all in all, this is just at the POC stage.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-parallel-worker-connection-checks-poc.patch";
	charset="us-ascii"
Content-ID: <1408434.1734973939.2@sss.pgh.pa.us>
Content-Description: fix-parallel-worker-connection-checks-poc.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/po=
stinit.c
index 770ab6906e..bd2b91bdde 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -342,9 +342,17 @@ CheckMyDatabase(const char *name, bool am_superuser, =
bool override_allow_connect
 	 * a way to recover from disabling all access to all databases, for
 	 * example "UPDATE pg_database SET datallowconn =3D false;".
 	 *
-	 * We do not enforce them for autovacuum worker processes either.
+	 * We do not enforce them for autovacuum worker processes either, so tha=
t
+	 * autovacuuming can still happen in all databases.
+	 *
+	 * We do not enforce them for parallel worker processes either: it's
+	 * assumed that the checks applied when the leader started are sufficien=
t.
+	 * (To do otherwise would make the use of parallel query less transparen=
t
+	 * than we want.)
 	 */
-	if (IsUnderPostmaster && !AmAutoVacuumWorkerProcess())
+	if (IsUnderPostmaster &&
+		!AmAutoVacuumWorkerProcess() &&
+		!InitializingParallelWorker)
 	{
 		/*
 		 * Check that the database is currently allowing connections.
@@ -865,23 +873,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
 		{
 			InitializeSessionUserId(username, useroid,
 									(flags & INIT_PG_OVERRIDE_ROLE_LOGIN) !=3D 0);
-
-			/*
-			 * In a parallel worker, set am_superuser based on the
-			 * authenticated user ID, not the current role.  This is pretty
-			 * dubious but it matches our historical behavior.  Note that this
-			 * value of am_superuser is used only for connection-privilege
-			 * checks here and in CheckMyDatabase (we won't reach
-			 * process_startup_options in a background worker).
-			 *
-			 * In other cases, there's been no opportunity for the current
-			 * role to diverge from the authenticated user ID yet, so we can
-			 * just rely on superuser() and avoid an extra catalog lookup.
-			 */
-			if (InitializingParallelWorker)
-				am_superuser =3D superuser_arg(GetAuthenticatedUserId());
-			else
-				am_superuser =3D superuser();
+			am_superuser =3D superuser();
 		}
 	}
 	else
@@ -909,16 +901,17 @@ InitPostgres(const char *in_dbname, Oid dboid,
 =

 	/*
 	 * The last few connection slots are reserved for superusers and roles
-	 * with privileges of pg_use_reserved_connections.  Replication
-	 * connections are drawn from slots reserved with max_wal_senders and ar=
e
-	 * not limited by max_connections, superuser_reserved_connections, or
-	 * reserved_connections.
+	 * with privileges of pg_use_reserved_connections.
+	 *
+	 * This does not apply to walsenders (which are instead limited by
+	 * max_wal_senders) nor parallel workers (which are instead limited by
+	 * max_parallel_workers).
 	 *
 	 * Note: At this point, the new backend has already claimed a proc struc=
t,
 	 * so we must check whether the number of free slots is strictly less th=
an
 	 * the reserved connection limits.
 	 */
-	if (!am_superuser && !am_walsender &&
+	if (!am_superuser && !am_walsender && !InitializingParallelWorker &&
 		(SuperuserReservedConnections + ReservedConnections) > 0 &&
 		!HaveNFreeProcs(SuperuserReservedConnections + ReservedConnections, &nf=
ree))
 	{

------- =_aaaaaaaaaa0--