Date: Mon, 19 Aug 2019 13:46:25 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, 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: <20190819044625.GA18166@paquier.xyz>
References: <15964-c1753bdfed722e04@postgresql.org>
 <24757.1566073861@sss.pgh.pa.us>
 <20190817210025.v4bblh4lhsxajmlj@alap3.anarazel.de>
 <6402.1566079145@sss.pgh.pa.us>
 <20190817224142.gq57z3cwt7c76dtk@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ADZbWkCsHQ7r3kzd"
Content-Disposition: inline
In-Reply-To: <20190817224142.gq57z3cwt7c76dtk@alap3.anarazel.de>
Precedence: bulk


--ADZbWkCsHQ7r3kzd
Content-Type: multipart/mixed; boundary="Kj7319i9nmIyA2yE"
Content-Disposition: inline


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

On Sat, Aug 17, 2019 at 03:41:42PM -0700, Andres Freund wrote:
> Heh, just discovered
> https://www.postgresql.org/message-id/20160921171819.1357.29774%40wrigley=
s.postgresql.org
> from the same reporter, where we went through this before :/

Ugh.

> On 2019-08-17 17:59:05 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> Most of the parallel code was move into bin/scripts/scripts_parallel.c -
>>> but there's still the above error check. Seems like we ought to add a
>>> ParallelSlotsMax() or such, and use that in the error check, rather than
>>> check FD_SETSIZE directly?
>>=20
>> Yeah, that would likely be cleaner than just responding to this directly.
>=20
> I'll go and do that.

Hm.  I'd like to keep the dependency to select.h directly in
scripts_parallel.c, so the ParallelSlotsMax sounds like a good thing
to me so as FD_SETSIZE remains localized.  That would give the
attached which does not take care of pgbench, and there is an extra
proposal in another part of this thread.  Just looking at it now..

>> Indeed.  I've confirmed the bug report on a local OpenBSD 6.4 build
>> (clang 6.0.0), and with "make -k" I can see that reindexdb.c fails
>> likewise.  But this is unsurprising given that POSIX says that
>> FD_SETSIZE is declared by sys/select.h.
>=20
> Right.

Okay, then the current code is broken in this sense.  It was
surprising to not see the buildfarm complain about that though.
--
Michael

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

diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 14a4f4a91c..f93f7dd5fe 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -153,10 +153,10 @@ main(int argc, char *argv[])
 					pg_log_error("number of parallel jobs must be at least 1");
 					exit(1);
 				}
-				if (concurrentCons > FD_SETSIZE - 1)
+				if (concurrentCons > ParallelSlotsMax())
 				{
 					pg_log_error("too many parallel jobs requested (maximum: %d)",
-								 FD_SETSIZE - 1);
+								 ParallelSlotsMax());
 					exit(1);
 				}
 				break;
diff --git a/src/bin/scripts/scripts_parallel.c b/src/bin/scripts/scripts_p=
arallel.c
index ffdc4e49ef..2b571a470e 100644
--- a/src/bin/scripts/scripts_parallel.c
+++ b/src/bin/scripts/scripts_parallel.c
@@ -94,6 +94,20 @@ 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
+ * by POSIX.
+ */
+int
+ParallelSlotsMax(void)
+{
+	/* leave some room for existing fds */
+	return FD_SETSIZE - 10;
+}
+
 /*
  * ParallelSlotsGetIdle
  *		Return a connection slot that is ready to execute a command.
diff --git a/src/bin/scripts/scripts_parallel.h b/src/bin/scripts/scripts_p=
arallel.h
index ab82c5e6a9..dd625577c6 100644
--- a/src/bin/scripts/scripts_parallel.h
+++ b/src/bin/scripts/scripts_parallel.h
@@ -21,6 +21,8 @@ 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 d81bfa3a6b..c5c38692ed 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -181,10 +181,10 @@ main(int argc, char *argv[])
 					pg_log_error("number of parallel jobs must be at least 1");
 					exit(1);
 				}
-				if (concurrentCons > FD_SETSIZE - 1)
+				if (concurrentCons > ParallelSlotsMax())
 				{
 					pg_log_error("too many parallel jobs requested (maximum: %d)",
-								 FD_SETSIZE - 1);
+								 ParallelSlotsMax());
 					exit(1);
 				}
 				break;

--Kj7319i9nmIyA2yE--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl1aKaEACgkQnvQgOdby
QH1Znw//dJMt0xdcSSeRdQKAu6gmbjZjdOWt8C713x0YOu/UZwI52mxzcW5bMc/v
O4Ps1pCklEUe5l1ksp53I5/3v/6o4eMAq1t+5w8kMcRLbG2gxf2kOD4yfAevO3Kw
ggRDuwHk+1TzRIN94DM3PItx2ypECYIZxTwo2kEFUYQLeJ6UEHQZmufM3w5sdZpt
H1jYaCXzH87RRle6ahegyu4glIU8Q5uFT0dxOeJcOW6PbrD6eRkWzv3CjJthHOF0
yDWP4t9skeVUFOhr6V30PaGOrTmjgwlVimNs/aEpTglBQWt5MAycOOb73wpyWWPo
OIfR5zyk2CkBssfOG/QNrxMYjlwL6qYrGeMw/tfk3GuakD0paPQwcg4GekuM+/78
ZvIC4JidGsjbbp6F0rdgJ2MYtLviL4QOwKf/yKe6nPnXHNXuyczvLJyBTnfnIp6V
wuj+Pd6zFudW/uaDrrsw4fGOukXHqLPvl4+HjMdAS1wu6gGVEZu9xiDqG3gWyToF
UR/FE83KmEEBZqpl2z8MCfClFasjgCLSmEpjLd+TGgZ0Lnw5aJ8uRLfTlf9sS/fa
Rb2HBeQAxQY9a0UxY8dsImlp+u+mUc+ya5OdDWxtFJ75YnXSEAages/+PkTDQC4W
GfyP+kf02N9D1XomSHV8oNi1IkejVDLhWnqYiWli0OaAJlQM2DA=
=D4ZE
-----END PGP SIGNATURE-----

--ADZbWkCsHQ7r3kzd--