Date: Wed, 21 Aug 2019 10:58:54 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@2ndquadrant.com>,
	Dilip Kumar <dilipbalaut@gmail.com>, jungleboogie0@gmail.com,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15964: vacuumdb.c:187:10: error: use of undeclared
 identifier 'FD_SETSIZE'
Message-ID: <20190821015854.GD1684@paquier.xyz>
References: <20190818001858.ho3ev4z57fqhs7a5@alap3.anarazel.de>
 <20190819025953.GA15110@alvherre.pgsql>
 <20190819042832.GA1702@paquier.xyz>
 <23514.1566189171@sss.pgh.pa.us>
 <20190819051251.GB18166@paquier.xyz>
 <20190820160553.37nd5jxs4dk2qujy@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="imjhCm/Pyz7Rq5F2"
Content-Disposition: inline
In-Reply-To: <20190820160553.37nd5jxs4dk2qujy@alap3.anarazel.de>
Precedence: bulk


--imjhCm/Pyz7Rq5F2
Content-Type: multipart/mixed; boundary="zbGR4y+acU1DwHSi"
Content-Disposition: inline


--zbGR4y+acU1DwHSi
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Aug 20, 2019 at 09:05:53AM -0700, Andres Freund wrote:
> On 2019-08-19 14:12:51 +0900, Michael Paquier wrote:
>> - 0002 makes vacuumdb and reindexdb fail when trying to assign a
>> socket with an unsupported range.  Should this bit be backpatched?  We
>> are doing that for vacuumdb for some time now, and the error is
>> confusing so I would prefer fixing it on older branches as well.
>=20
> Yea, I think we clearly need to. The code right now essentially is a
> wild write, albeit with a somewhat limited range of what it can impact.

Okay.

> I think the error message ought be reformulated, so users have a chance
> to actually understand what they need to change to avoid the error. At
> least something roughly like "too many jobs for this platform's select()".

pgbench needs an extra fix then?

> ISTM that we should fail in ParallelSlotsSetup(), rather than
> ParallelSlotsGetIdle() though? That's always going to be earlier, and
> there's no way to get into the problematic situation at a later point,
> no?

Okay, done this way.  What do you think about the attached?
--
Michael

--zbGR4y+acU1DwHSi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="parallel-slot-fdlimit-v3.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 570cf3306a..fbf1370d61 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -6420,7 +6420,7 @@ add_socket_to_set(socket_set *sa, int fd, int idx)
 		 * Doing a hard exit here is a bit grotty, but it doesn't seem worth
 		 * complicating the API to make it less grotty.
 		 */
-		fprintf(stderr, "too many client connections for select()\n");
+		fprintf(stderr, "too many jobs for this platform's select()\n");
 		exit(1);
 	}
 	FD_SET(fd, &sa->fds);
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index f93f7dd5fe..f00aec15de 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -153,12 +153,6 @@ main(int argc, char *argv[])
 					pg_log_error("number of parallel jobs must be at least 1");
 					exit(1);
 				}
-				if (concurrentCons > ParallelSlotsMax())
-				{
-					pg_log_error("too many parallel jobs requested (maximum: %d)",
-								 ParallelSlotsMax());
-					exit(1);
-				}
 				break;
 			case 'v':
 				verbose =3D true;
diff --git a/src/bin/scripts/scripts_parallel.c b/src/bin/scripts/scripts_p=
arallel.c
index 10379a1f99..e0643bc914 100644
--- a/src/bin/scripts/scripts_parallel.c
+++ b/src/bin/scripts/scripts_parallel.c
@@ -94,20 +94,6 @@ select_loop(int maxFd, fd_set *workerset, bool *aborting)
 	return i;
 }
=20
-/*
- * ParallelSlotsMax
- *		Returns the maximum number of parallel slots supported.
- *
- * Note that this is included here as FD_SETSIZE is declared in sys/select=
=2Eh
- * per POSIX.
- */
-int
-ParallelSlotsMax(void)
-{
-	/* leave some room for pre-existing fds */
-	return FD_SETSIZE - 10;
-}
-
 /*
  * ParallelSlotsGetIdle
  *		Return a connection slot that is ready to execute a command.
@@ -246,6 +232,18 @@ ParallelSlotsSetup(const char *dbname, const char *hos=
t, const char *port,
 		{
 			conn =3D connectDatabase(dbname, host, port, username, prompt_password,
 								   progname, echo, false, true);
+
+			/*
+			 * Fail immediately if trying to use an index in an unsupported
+			 * range.  Doing a hard exit here is not beautiful, but that's
+			 * not worth complicating the logic.
+			 */
+			if (PQsocket(conn) >=3D FD_SETSIZE)
+			{
+				fprintf(stderr, "too many jobs for this platform's select()\n");
+				exit(1);
+			}
+
 			init_slot(slots + i, conn);
 		}
 	}
diff --git a/src/bin/scripts/scripts_parallel.h b/src/bin/scripts/scripts_p=
arallel.h
index 8042345072..ab82c5e6a9 100644
--- a/src/bin/scripts/scripts_parallel.h
+++ b/src/bin/scripts/scripts_parallel.h
@@ -21,8 +21,6 @@ typedef struct ParallelSlot
 	bool		isFree;			/* Is it known to be idle? */
 } ParallelSlot;
=20
-extern int	ParallelSlotsMax(void);
-
 extern ParallelSlot *ParallelSlotsGetIdle(ParallelSlot *slots, int numslot=
s);
=20
 extern ParallelSlot *ParallelSlotsSetup(const char *dbname, const char *ho=
st,
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index c5c38692ed..2c7219239f 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -181,12 +181,6 @@ main(int argc, char *argv[])
 					pg_log_error("number of parallel jobs must be at least 1");
 					exit(1);
 				}
-				if (concurrentCons > ParallelSlotsMax())
-				{
-					pg_log_error("too many parallel jobs requested (maximum: %d)",
-								 ParallelSlotsMax());
-					exit(1);
-				}
 				break;
 			case 2:
 				maintenance_db =3D pg_strdup(optarg);

--zbGR4y+acU1DwHSi--

--imjhCm/Pyz7Rq5F2
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl1cpV4ACgkQnvQgOdby
QH3ERg/9HYnGmDPcbpVTKv6F3hW/XQLi2UxeKUYB/vB1HvEcKLPfj850U5iYNslt
94SCtkYU5yzk7caq+KQGdxIh0WF4y6olv6uVsowOMJOMe/yKt+gN89+RnkOUbXO1
X4n0dAjZb72jKDp7j9Yz+dRkb9MA3dR3KrHxSNTIRZf/OveA3U9g378IuHsVZCaC
zbpCMU/FcNkAvI34SuosGJvJaF57dC3MuoetDduNgyYk1YebOF9CzbG/feT6n18q
DoOnJ6eUbc0dTMrbaYSIGOybMTs1ndskty3zKK3IpmeRW5PVpZ06nNIacZMHMWKO
QrOwnT+wiVBqgMmw46z3BMkNguaysqdEz8RMNHzBMqUHIfLGAQs3xVGIRrAv3f3N
XT1mhMWovc7smBl3lkVo0xHNAteolqIvwLfsEZI8HhP4KIxxv+u7SQqgHqEHsnIt
U24gcmftwCgzhMgeC8FpAyK3SX5cqQ6U39d6C1sqJgF07QfCl4nc7DdQ0oiu+mRq
XITAK2mqPic8mUve/WKguYxjEGqYxdinWKJWaSHth8bPQnvScU//3DNwRESGKX34
hJIM32YNqiLmLQ0hi/JATB/nmGpmCCov8fwhd3kpLHxS6qvzHf5x3ZxdP1k1b+QQ
iQtNKdPD+hlFQYpXrXM1DduCuF1ibg4JQJQWLu28oVuGaIcWJM0=
=29ig
-----END PGP SIGNATURE-----

--imjhCm/Pyz7Rq5F2--