Обсуждение: BUG #15964: vacuumdb.c:187:10: error: use of undeclared identifier 'FD_SETSIZE'

The following bug has been logged on the website:

Bug reference:      15964
Logged by:          sean
Email address:      jungleboogie0@gmail.com
PostgreSQL version: 12beta3
Operating system:   kern.version=OpenBSD 6.6-beta (GENERIC.MP) #221: F
Description:

Hi All,

I'm trying to compile Postgresql from master on my OpenBSD machine, but I'm
getting a build failure here:
vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
                                if (concurrentCons > FD_SETSIZE - 1)
                                                     ^
vacuumdb.c:187:10: error: use of undeclared identifier 'FD_SETSIZE'
                                                                 FD_SETSIZE
- 1);
                                                                 ^
2 errors generated.
gmake[3]: *** [<builtin>: vacuumdb.o] Error 1
gmake[3]: Leaving directory '/home/sean/bin/postgresql/src/bin/scripts'
gmake[2]: *** [Makefile:41: all-scripts-recurse] Error 2
gmake[2]: Leaving directory '/home/sean/bin/postgresql/src/bin'
gmake[1]: *** [Makefile:42: all-bin-recurse] Error 2
gmake[1]: Leaving directory '/home/sean/bin/postgresql/src'
gmake: *** [GNUmakefile:11: all-src-recurse] Error 2
sean@beginning:postgresql$ sysctk kern.version

OS information:
$ sysctl kern.version
kern.version=OpenBSD 6.6-beta (GENERIC.MP) #221: Fri Aug 16 16:00:01 MDT
2019
    deraadt@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP

gmake information:
$ gmake -v
GNU Make 4.2.1
Built for x86_64-unknown-openbsd6.6
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

clang information:
$ cc -v 
OpenBSD clang version 8.0.0 (tags/RELEASE_800/final) (based on LLVM 8.0.0)
Target: amd64-unknown-openbsd6.6
Thread model: posix
InstalledDir: /usr/bin

Posgresql commit: d78d452bc5ac46a970e4fca2b31f3d533815c39a

config options:
./configure --enable-cassert --enable-debug  --with-perl \
        --with-python --with-tcl --with-tclconfig=/usr/local/lib/tcl/tcl8.6/
\
        --with-includes=/usr/local/include

What am I doing wrong? What more information would you like?

https://www.postgresql.org/docs/current/bug-reporting.html
5.1 states:
PostgreSQL fails to compile, build, or install according to the instructions
on supported platforms.


Thanks!

P.S. The submit bug page does not list master/current/trunk as a version,
and I needed to make a selection, which doesn't match the commit I
referenced above. Fear not, I am using the git repo here:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=summary and cloned it
locally.

PG Bug reporting form <noreply@postgresql.org> writes:
> I'm trying to compile Postgresql from master on my OpenBSD machine, but I'm
> getting a build failure here:
> vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
>                                 if (concurrentCons > FD_SETSIZE - 1)
>                                                      ^

Hmm, it seems somebody removed the "#include <sys/select.h>" from
that file, which was a pretty not-bright idea.  But I wonder why
the OpenBSD machines in the buildfarm aren't complaining.

            regards, tom lane

Hi,

On 2019-08-17 19:06:33 +0000, PG Bug reporting form wrote:
> I'm trying to compile Postgresql from master on my OpenBSD machine, but I'm
> getting a build failure here:
> vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
>                                 if (concurrentCons > FD_SETSIZE - 1)
>                                                      ^
> vacuumdb.c:187:10: error: use of undeclared identifier 'FD_SETSIZE'
>                                                                  FD_SETSIZE
> - 1);
>                                                                  ^
> 2 errors generated.

Yea, that file is clearly missing an include for #include
<sys/select.h>. I don't immediately see how that file is included on
other platforms, but it's obviously not enough for your version of
openbsd.

I assume adding

#ifdef HAVE_SYS_SELECT_H
#include <sys/select.h>
#endif

after

#include "postgres_fe.h"

in vacuumdb.c fixes the problem?


Michael, it looks like this is an oversight in

commit 5f3840370b63fdf17f704a285623ccc233fa8d4f
Author: Michael Paquier <michael@paquier.xyz>
Date:   2019-07-19 09:31:58 +0900

    Refactor parallelization processing code in src/bin/scripts/

Greetings,

Andres Freund

Hi,

On 2019-08-17 16:31:01 -0400, Tom Lane wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > I'm trying to compile Postgresql from master on my OpenBSD machine, but I'm
> > getting a build failure here:
> > vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
> >                                 if (concurrentCons > FD_SETSIZE - 1)
> >                                                      ^
> 
> Hmm, it seems somebody removed the "#include <sys/select.h>" from
> that file, which was a pretty not-bright idea.

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?


> But I wonder why the OpenBSD machines in the buildfarm aren't complaining.

Or even why it works on other platforms.  On linux/glibc it looks like
sys/select.h is included by sys/types.h under certain conditions:

#ifdef  __USE_MISC
/* In BSD <sys/types.h> is expected to define BYTE_ORDER.  */
# include <endian.h>

/* It also defines `fd_set' and the FD_* macros for `select'.  */
# include <sys/select.h>
#endif /* Use misc.  */

which in turn is included by stddef.h under certain conditions:

#if defined __USE_MISC || defined __USE_XOPEN_EXTENDED
# include <sys/types.h> /* we need int32_t... */

stddef.h is included by c.h, so will obviously be included in any of our
.c files.

_USE_MISC and _XOPEN_SOURCE_EXTENDED are defined by default, unless
they're explicitly specified (which we don't).


I assume there's some compiler specific going on. Our animals use an old
gcc version, whereas Sean's uses a modern clang.  Not hard to imagine
that the compiler specific bits look different enough to cause such a discrepancy.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-08-17 16:31:01 -0400, Tom Lane wrote:
>> PG Bug reporting form <noreply@postgresql.org> writes:
>>> vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'

>> Hmm, it seems somebody removed the "#include <sys/select.h>" from
>> that file, which was a pretty not-bright idea.

> 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?

Yeah, that would likely be cleaner than just responding to this directly.

>> But I wonder why the OpenBSD machines in the buildfarm aren't complaining.

> Or even why it works on other platforms.

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.  And I'm not that astonished
by it not failing on Linux, either; the glibc headers are well known
for #including much more than POSIX says they must.  But it's
surprising and worrisome that none of our other buildfarm platforms
complained.  Seems like somebody should start running an animal with
a more modern OpenBSD, at least.

            regards, tom lane

Hi,

Heh, just discovered
https://www.postgresql.org/message-id/20160921171819.1357.29774%40wrigleys.postgresql.org
from the same reporter, where we went through this before :/


On 2019-08-17 17:59:05 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-08-17 16:31:01 -0400, Tom Lane wrote:
> >> PG Bug reporting form <noreply@postgresql.org> writes:
> >>> vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
> 
> >> Hmm, it seems somebody removed the "#include <sys/select.h>" from
> >> that file, which was a pretty not-bright idea.
> 
> > 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?
> 
> Yeah, that would likely be cleaner than just responding to this directly.

I'll go and do that.


> >> But I wonder why the OpenBSD machines in the buildfarm aren't complaining.
> 
> > Or even why it works on other platforms.
> 
> 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.

Right.


> And I'm not that astonished by it not failing on Linux, either; the
> glibc headers are well known for #including much more than POSIX says
> they must.  But it's surprising and worrisome that none of our other
> buildfarm platforms complained.  Seems like somebody should start
> running an animal with a more modern OpenBSD, at least.

I don't see an easy option for making glibc less aggressive on that
front, unfortunately. And I don't want to start running a vm with
openbsd or such.

I wonder if it'd be worth setting up a buildfarm animal on linux using
musl as the libc, based on a quick look it includes less. Doesn't appear
to find this issue however [1], so it's perhaps not worth it.  It fails
with src/bin/pg_upgrade/file.c including linux/fs.h without a proper
configure check:
#ifdef __linux__
#include <sys/ioctl.h>
#include <linux/fs.h>
#endif

Probably worth fixing, even if it can also fixed by just symlinking
/usr/include/linux into /usr/include/x86_64-linux-musl/ (or whatever is
appropriate for the current platform).


[1] The relevant commit's explanation isn't very helpful:
commit 2555fe1b6da21119f87d407ef3838648d5fd601d
Author: Rich Felker <dalias@aerifal.cx>
Date:   2011-04-10 22:47:43 -0400

    add some ugly legacy type names in sys/types.h (u_char etc.)

diff --git a/include/sys/types.h b/include/sys/types.h
index 216574ad..5c6b2090 100644
--- a/include/sys/types.h
+++ b/include/sys/types.h
@@ -59,6 +59,14 @@ extern "C" {
 
 #ifdef _GNU_SOURCE
 typedef unsigned long caddr_t;
+typedef unsigned char u_char;
+typedef unsigned short u_short, ushort;
+typedef unsigned u_int, uint;
+typedef unsigned long u_long, ulong;
+typedef long long quad_t;
+typedef unsigned long long u_quad_t;
+#include <endian.h>
+#include <sys/select.h>
 #include <sys/sysmacros.h>
 #endif
 


Greetings,

Andres Freund

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

Oh, wow! Sorry I didn't remember that. Guess I didn't do a good enough job
searching through the archives.

>
>
> On 2019-08-17 17:59:05 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2019-08-17 16:31:01 -0400, Tom Lane wrote:
> > >> PG Bug reporting form <noreply@postgresql.org> writes:
> > >>> vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
> >
> > >> Hmm, it seems somebody removed the "#include <sys/select.h>" from
> > >> that file, which was a pretty not-bright idea.
> >
> > > 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?
> >
> > Yeah, that would likely be cleaner than just responding to this directly.
>
> I'll go and do that.
>
>
> > >> But I wonder why the OpenBSD machines in the buildfarm aren't complaining.
> >
> > > Or even why it works on other platforms.
> >
> > 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.
>
> Right.

I noticed all the machines in your buildfarm are running OpenBSD 5.9 from March
2016 and I believe before clang was the default compiler. I'll see what I can
find on local craigslist for inexpensive amd64 machines and then have it build
Postgres.

Thanks for the efforts you two have put into tracking this down.

Hi,

On 2019-08-17 15:41:42 -0700, Andres Freund wrote:
> On 2019-08-17 17:59:05 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2019-08-17 16:31:01 -0400, Tom Lane wrote:
> > >> PG Bug reporting form <noreply@postgresql.org> writes:
> > >>> vacuumdb.c:184:26: error: use of undeclared identifier 'FD_SETSIZE'
> >
> > >> Hmm, it seems somebody removed the "#include <sys/select.h>" from
> > >> that file, which was a pretty not-bright idea.
> >
> > > 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?
> >
> > Yeah, that would likely be cleaner than just responding to this directly.
>
> I'll go and do that.

Hm. This made me think: Why is

                if (concurrentCons > FD_SETSIZE - 1)
                {
                    pg_log_error("too many parallel jobs requested (maximum: %d)",
                                 FD_SETSIZE - 1);

a useful test / error message?  FD_SETSIZE is about the numerical value
of fds.  There will usually be at least three fds open, starting at 0 -
but there easily can be more, depending on what the reindexdb/vacuumdb
caller is doing.

I'm prone to off-by-one errors, but I think this will over-estimate the
number of allowed connections by 1 even if there's just
stdin/stdout/stderr open.

Looks like this has been copied forward from
commit a17923204736d8842eade3517d6a8ee81290fca4

Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date:   2015-01-23 15:02:45 -0300

    vacuumdb: enable parallel mode
    
    This mode allows vacuumdb to open several server connections to vacuum
    or analyze several tables simultaneously.
    
    Author: Dilip Kumar.  Some reworking by Álvaro Herrera
    Reviewed by: Jeff Janes, Amit Kapila, Magnus Hagander, Andres Freund

Alvaro, Dilip?

The pre 12 pgbench at least just subtracted 10 from FD_SETSIZE, to make
room for some pre-existing fds.

What is the reason that this doesn't use poll() in the first place? It
surely can't be - as it is the case for pgbench - that the minimum time
resolution is 1ms?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Hm. This made me think: Why is
>                 if (concurrentCons > FD_SETSIZE - 1)
> a useful test / error message?

Good point, it's not.  Subtracting off 10 or so might be reasonable.

> What is the reason that this doesn't use poll() in the first place?

We still support platforms without that, no?  Windows, for one.

            regards, tom lane

Hi,

On 2019-08-17 20:59:56 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Hm. This made me think: Why is
> >                 if (concurrentCons > FD_SETSIZE - 1)
> > a useful test / error message?
> 
> Good point, it's not.  Subtracting off 10 or so might be reasonable.

I wonder if we shouldn't just do the same as pgbench now does, and just
only error when adding a too large fd. That does reduce the number of
detected cases, true, but it also adds robustness, because larger fds
are properly handled.

> > What is the reason that this doesn't use poll() in the first place?
> 
> We still support platforms without that, no?  Windows, for one.

Ah, right. I forgot that because we do rely on poll() in latch.c - but
we do have an alternative windows implementation there...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-08-17 20:59:56 -0400, Tom Lane wrote:
>> Good point, it's not.  Subtracting off 10 or so might be reasonable.

> I wonder if we shouldn't just do the same as pgbench now does, and just
> only error when adding a too large fd.

Works for me.

            regards, tom lane

On 2019-Aug-17, Andres Freund wrote:

> Hm. This made me think: Why is
> 
>                 if (concurrentCons > FD_SETSIZE - 1)
>                 {
>                     pg_log_error("too many parallel jobs requested (maximum: %d)",
>                                  FD_SETSIZE - 1);
> 
> a useful test / error message?  FD_SETSIZE is about the numerical value
> of fds.  There will usually be at least three fds open, starting at 0 -
> but there easily can be more, depending on what the reindexdb/vacuumdb
> caller is doing.

Hmm ... yeah, this is clearly not perfect.  In my laptop, vacuumdb -j 1021
works; 1022 and 1023 fail like this after opening a number of conns:

vacuumdb: vacuuming database "alvherre"
vacuumdb: error: could not connect to database alvherre: could not look up local user ID 1000: Too many open files

and 1024 fails like this immediately on start:
vacuumdb: error: too many parallel jobs requested (maximum: 1023)

After 'ulimit -n 1200', vacuumdb -j1023 fails like this:

vacuumdb: vacuuming database "alvherre"
*** buffer overflow detected ***: vacuumdb terminated
Aborted

So I agree that we need a fix.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, Aug 18, 2019 at 10:59:54PM -0400, Alvaro Herrera wrote:
> Hmm ... yeah, this is clearly not perfect.  In my laptop, vacuumdb -j 1021
> works; 1022 and 1023 fail like this after opening a number of conns:
>
> So I agree that we need a fix.

How would you detect how many fds can be opened by a user in this
case?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sun, Aug 18, 2019 at 10:59:54PM -0400, Alvaro Herrera wrote:
>> So I agree that we need a fix.

> How would you detect how many fds can be opened by a user in this
> case?

I think Andres' suggestion is probably fine: don't try to detect
it in advance.  Just open the files, and error out if we need to
put an fd index >= FD_SETSIZE into an fd_set.  It'll be a shade
less user-friendly, in that the program might run for a bit before
failing; but I doubt that such cases arise often enough to be worth
working harder.

            regards, tom lane

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%40wrigleys.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?
>>
>> Yeah, that would likely be cleaner than just responding to this directly.
>
> 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.
>
> Right.

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

On Mon, Aug 19, 2019 at 12:32:51AM -0400, Tom Lane wrote:
> I think Andres' suggestion is probably fine: don't try to detect
> it in advance.  Just open the files, and error out if we need to
> put an fd index >= FD_SETSIZE into an fd_set.  It'll be a shade
> less user-friendly, in that the program might run for a bit before
> failing; but I doubt that such cases arise often enough to be worth
> working harder.

Thanks.  I have somewhat not catched what Andres was suggesting here.
So attached are two patches:
- 0001 should take care of the compilation failure, by moving
FD_SETSIZE into scripts_parallel.c.
- 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.

Thoughts?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Aug 17, 2019 at 03:41:42PM -0700, Andres Freund wrote:
>>> 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.

>> Right.

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

Yeah --- see 51c3e9fad.  We should have set up a modern-OpenBSD
buildfarm member back then, but failed to.  Let's do that this
time.

            regards, tom lane

On 2019-08-19 07:14, Tom Lane wrote:

>> Okay, then the current code is broken in this sense.  It was
>> surprising to not see the buildfarm complain about that though.
> 
> Yeah --- see 51c3e9fad.  We should have set up a modern-OpenBSD
> buildfarm member back then, but failed to.  Let's do that this
> time.

I could set up a OpenBSD 6.5 animal.  I have resources available.

Will do it the next coming days.

/Mikael

On Mon, Aug 19, 2019 at 07:32:21AM +0200, Mikael Kjellström wrote:
> I could set up a OpenBSD 6.5 animal.  I have resources available.
>
> Will do it the next coming days.

Thanks!  That's great.
--
Michael

On Mon, Aug 19, 2019 at 02:12:51PM +0900, Michael Paquier wrote:
> Thanks.  I have somewhat not catched what Andres was suggesting here.
> So attached are two patches:
> - 0001 should take care of the compilation failure, by moving
> FD_SETSIZE into scripts_parallel.c.

I have been able to work on this one more, and applied a fix that
should address the compilation failure.  While on it, I have reduced
the maximum number of parallel slots allowed to give some room for
pre-existing fds as pgbench did before moving to its poll()
implementation.

> - 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.

For this one, I am still not completely sure which way we would want
to go.  The issue exists down to 9.6 for vacuumdb, so could a fix like
the one I previously proposed be acceptable for a back-patch as this
is not worth the complexity?  Should we try to move to a poll()-based
implementation like pgbench on HEAD, which most likely would result in
having pgbench also use parallel slots with a bit of refactoring, no?
--
Michael

Hi,

On 2019-08-19 14:12:51 +0900, Michael Paquier wrote:
> On Mon, Aug 19, 2019 at 12:32:51AM -0400, Tom Lane wrote:
> > I think Andres' suggestion is probably fine: don't try to detect
> > it in advance.  Just open the files, and error out if we need to
> > put an fd index >= FD_SETSIZE into an fd_set.  It'll be a shade
> > less user-friendly, in that the program might run for a bit before
> > failing; but I doubt that such cases arise often enough to be worth
> > working harder.
> 
> Thanks.  I have somewhat not catched what Andres was suggesting here.
> So attached are two patches:
> - 0001 should take care of the compilation failure, by moving
> FD_SETSIZE into scripts_parallel.c.

I don't understand why we would still want to keep the check? The check
is pretty much useless, imo. And it adds code to multiple different
files?


> - 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.

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.


> diff --git a/src/bin/scripts/scripts_parallel.c b/src/bin/scripts/scripts_parallel.c
> index 2b571a470e..b124db0d48 100644
> --- a/src/bin/scripts/scripts_parallel.c
> +++ b/src/bin/scripts/scripts_parallel.c
> @@ -160,6 +160,17 @@ ParallelSlotsGetIdle(ParallelSlot *slots, int numslots)
>              if (sock < 0)
>                  continue;
>  
> +            /*
> +             * 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 (sock >= FD_SETSIZE)
> +            {
> +                fprintf(stderr, "too many client connections for select()\n");
> +                exit(1);
> +            }
> +
>              FD_SET(sock, &slotset);
>              if (sock > maxFd)
>                  maxFd = sock;
> -- 
> 2.23.0.rc1
> 

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()".

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?

Greetings,

Andres Freund

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.
>
> 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

Hi,

On 2019-08-21 10:58:54 +0900, Michael Paquier wrote:
> On Tue, Aug 20, 2019 at 09:05:53AM -0700, Andres Freund wrote:
> > 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?

Well, there the commandline option talks about clients? Whereas
vacuumdb/reindexdb talk about jobs, but the old message talked about
clients.  So no, I don't think it makes sense to adapt it the same way.


> > 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

> @@ -246,6 +232,18 @@ ParallelSlotsSetup(const char *dbname, const char *host, const char *port,
>          {
>              conn = 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.
> +             */

"that's not worth" sounds odd to me in the above sentence. I don't quite
get the half-hearted rewrite of the comment from pgbench.c.


Greetings,

Andres Freund

On 2019-Aug-21, Michael Paquier wrote:

> +            if (PQsocket(conn) >= FD_SETSIZE)
> +            {
> +                fprintf(stderr, "too many jobs for this platform's select()\n");
> +                exit(1);
> +            }

BTW why fprintf?  Since you can get into this by careless use of -j,
ISTM that this should be a translatable string.  I'd consider
pg_log_fatal().  Maybe something like "Argument of -j too large for this
platform -- try %d", numslots - i.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Aug 20, 2019 at 07:07:49PM -0700, Andres Freund wrote:
> On 2019-08-21 10:58:54 +0900, Michael Paquier wrote:
>> @@ -246,6 +232,18 @@ ParallelSlotsSetup(const char *dbname, const char *host, const char *port,
>>          {
>>              conn = 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.
>> +             */
>
> "that's not worth" sounds odd to me in the above sentence. I don't quite
> get the half-hearted rewrite of the comment from pgbench.c.

What about just keeping the first sentence then?  And this gives:
"Fail immediately if trying to use an index in an unsupported range."
--
Michael

On Tue, Aug 20, 2019 at 10:40:05PM -0400, Alvaro Herrera wrote:
> On 2019-Aug-21, Michael Paquier wrote:
>
> > +            if (PQsocket(conn) >= FD_SETSIZE)
> > +            {
> > +                fprintf(stderr, "too many jobs for this platform's select()\n");
> > +                exit(1);
> > +            }
>
> BTW why fprintf?  Since you can get into this by careless use of -j,
> ISTM that this should be a translatable string.  I'd consider
> pg_log_fatal().  Maybe something like "Argument of -j too large for this
> platform -- try %d", numslots - i.

If giving a recommendation, shouldn't that be linked only to "i"
instead?  If for example numslots = 1024, but that we get an error
after allocating 128 slots because of a lack of fds in the range, then
we would recommend 896, which would still result in an error.  I quite
like the suggestion from Andres to keep the message simple with "too
many jobs for this platform's select()".  You are indeed right about
the pg_log_fatal() part here, I have changed the patch to do that on
my local branch.
--
Michael

On 2019-Aug-21, Michael Paquier wrote:

> On Tue, Aug 20, 2019 at 10:40:05PM -0400, Alvaro Herrera wrote:
> > 
> > BTW why fprintf?  Since you can get into this by careless use of -j,
> > ISTM that this should be a translatable string.  I'd consider
> > pg_log_fatal().  Maybe something like "Argument of -j too large for this
> > platform -- try %d", numslots - i.
> 
> If giving a recommendation, shouldn't that be linked only to "i"
> instead?  If for example numslots = 1024, but that we get an error
> after allocating 128 slots because of a lack of fds in the range, then
> we would recommend 896, which would still result in an error.

I was too tired last night to think this through properly ... the first
few times I wrote that were even more wrong.  "i" seems right.

> I quite like the suggestion from Andres to keep the message simple
> with "too many jobs for this platform's select()".  You are indeed
> right about the pg_log_fatal() part here, I have changed the patch to
> do that on my local branch.

Well, it's a user-facing error, so I'd rather make it user-friendly.
It doesn't seem difficult, or unreliable enough not to try.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Aug 21, 2019 at 09:49:54AM -0400, Alvaro Herrera wrote:
> On 2019-Aug-21, Michael Paquier wrote:
>> I quite like the suggestion from Andres to keep the message simple
>> with "too many jobs for this platform's select()".  You are indeed
>> right about the pg_log_fatal() part here, I have changed the patch to
>> do that on my local branch.
>
> Well, it's a user-facing error, so I'd rather make it user-friendly.
> It doesn't seem difficult, or unreliable enough not to try.

Still, in this case, because of the nature of FD_SETSIZE the hint may
finish by being wrong, no?  I am not sure that it is worth going this
way.
--
Michael

On 2019-Aug-21, Michael Paquier wrote:

> On Wed, Aug 21, 2019 at 09:49:54AM -0400, Alvaro Herrera wrote:

> > Well, it's a user-facing error, so I'd rather make it user-friendly.
> > It doesn't seem difficult, or unreliable enough not to try.
> 
> Still, in this case, because of the nature of FD_SETSIZE the hint may
> finish by being wrong, no?  I am not sure that it is worth going this
> way.

Theoretically it is possible that we give a wrong hint, but I think it's
hardly a practical reality.  I'd rather give the hint and tell the user
what a reasonable parameter might be.  If they run under a different
environment (I dunno, they use shell invocation line with other
redirections I guess), then they might get a different hint next time.
So what?

I guess the other possibility is that there exists an operating system
that returns file descriptors higher than its FD_SETSIZE, when some fds
below FD_SETSIZE are still available.  Doesn't seem realistic.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I guess the other possibility is that there exists an operating system
> that returns file descriptors higher than its FD_SETSIZE, when some fds
> below FD_SETSIZE are still available.  Doesn't seem realistic.

I think POSIX requires open() to select the lowest unused FD.  Otherwise
tricks like closing and reopening stdout wouldn't work reliably.

            regards, tom lane

On 2019-Aug-21, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > I guess the other possibility is that there exists an operating system
> > that returns file descriptors higher than its FD_SETSIZE, when some fds
> > below FD_SETSIZE are still available.  Doesn't seem realistic.
> 
> I think POSIX requires open() to select the lowest unused FD.  Otherwise
> tricks like closing and reopening stdout wouldn't work reliably.

Ah, yes, that's right -- my system's open(3p) manpage says:

       The  open()  function  shall return a file descriptor for the named file
       that is the lowest file descriptor not currently open for that process.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Aug 21, 2019 at 10:34:49AM -0400, Alvaro Herrera wrote:
> Ah, yes, that's right -- my system's open(3p) manpage says:
>
>        The  open()  function  shall return a file descriptor for the named file
>        that is the lowest file descriptor not currently open for that process.

Ah, well.  In this case, what about that?
("too many jobs for this platform's select() -- try %d", i-1)

I don't think that it is a good idea to use the option name -j/--jobs
directly in the error string as scripts_parallel.c should not have any
knowledge of how the option layer is in the tools using its APIs.
Attached is an updated patch, with refined comments.
--
Michael

Hi,

On 2019-08-22 10:47:39 +0900, Michael Paquier wrote:
> On Wed, Aug 21, 2019 at 10:34:49AM -0400, Alvaro Herrera wrote:
> > Ah, yes, that's right -- my system's open(3p) manpage says:
> > 
> >        The  open()  function  shall return a file descriptor for the named file
> >        that is the lowest file descriptor not currently open for that process.
> 
> Ah, well.  In this case, what about that?
> ("too many jobs for this platform's select() -- try %d", i-1)

I don't think the - 1 bit is likely to be helpful, as it's too likely to
be inaccurate.

> I don't think that it is a good idea to use the option name -j/--jobs
> directly in the error string as scripts_parallel.c should not have any
> knowledge of how the option layer is in the tools using its APIs.

This isn't a particularly general API - it's in
src/bin/scripts/scripts_parallel.h. So I think a better error message is
more than worth the slight decrease in generality.

Greetings,

Andres Freund

On Thu, Aug 22, 2019 at 08:09:45AM -0700, Andres Freund wrote:
> This isn't a particularly general API - it's in
> src/bin/scripts/scripts_parallel.h. So I think a better error message is
> more than worth the slight decrease in generality.

The latest patch does that:
+    pg_log_fatal("too many jobs for this platform's select() -- try %d"
Do you have a better suggestion to offer, like a s#jobs#-j/--jobs#?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> The latest patch does that:
> +    pg_log_fatal("too many jobs for this platform's select() -- try %d"
> Do you have a better suggestion to offer, like a s#jobs#-j/--jobs#?

That seems over-punctuated, plus it burdens the reader with unnecessary
detail.  Most SQL folk would be confused by the reference to SELECT,
I bet.  (In fact, don't we have a style guideline against mentioning
particular syscalls?)

How about just "too many jobs for this platform -- try %d" ?

            regards, tom lane

On Fri, Aug 23, 2019 at 10:58:14AM -0400, Tom Lane wrote:
> That seems over-punctuated, plus it burdens the reader with unnecessary
> detail.  Most SQL folk would be confused by the reference to SELECT,
> I bet.  (In fact, don't we have a style guideline against mentioning
> particular syscalls?)
>
> How about just "too many jobs for this platform -- try %d" ?

WFM.  Do others have any more suggestions?
--
Michael

On Sat, Aug 24, 2019 at 11:23:19AM +0900, Michael Paquier wrote:
> WFM.  Do others have any more suggestions?

Okay, done this way down to 9.5.
--
Michael

Thus said Michael Paquier <michael@paquier.xyz> on Mon, 26 Aug 2019 
11:17:23 +0900
> On Sat, Aug 24, 2019 at 11:23:19AM +0900, Michael Paquier wrote:
>> WFM.  Do others have any more suggestions?
> 
> Okay, done this way down to 9.5.

I haven't yet tested on amd64, but good news with ARM64 with OpenBSD 6.6 
-current:
$ sysctl kern.version
kern.version=OpenBSD 6.6-beta (GENERIC.MP) #198: Sun Aug 25 18:38:28 MDT 
2019
     deraadt@arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP


gmake[1]: Nothing to be done for 'all'.
gmake[1]: Leaving directory '/home/sean/bin/postgresql/config'
All of PostgreSQL successfully made. Ready to install.




> --
> Michael
> 

On Sun, Aug 25, 2019 at 11:46:05PM -0700, jungle boogie wrote:
> I haven't yet tested on amd64, but good news with ARM64 with OpenBSD 6.6
> -current:
> $ sysctl kern.version
> kern.version=OpenBSD 6.6-beta (GENERIC.MP) #198: Sun Aug 25 18:38:28 MDT
> 2019
>     deraadt@arm64.openbsd.org:/usr/src/sys/arch/arm64/compile/GENERIC.MP
>
> gmake[1]: Nothing to be done for 'all'.
> gmake[1]: Leaving directory '/home/sean/bin/postgresql/config'
> All of PostgreSQL successfully made. Ready to install.

Thanks for testing out.  And my apologies for introducing the issue in
the first place.
--
Michael

On 2019-Aug-27, Michael Paquier wrote:

> Thanks for testing out.  And my apologies for introducing the issue in
> the first place.

No need to apologize. Thanks for fixing,

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Aug-27, Michael Paquier wrote:

> Thanks for testing out.  And my apologies for introducing the issue in
> the first place.

BTW one nit: in deference to src/tools/git_changelog, it would be better
to make all branch commit messages exactly the same, and if you want to
make per-branch distinctions make them in the unified text ("On branches
so and so, also add a newline", etc) rather than writing per branch
slightly different messages.  They way you did it, git_changelog creates
three entries when it could have been a single one:

Author: Michael Paquier <michael@paquier.xyz>
Branch: REL_11_STABLE [128e9b2cc] 2019-08-27 09:11:43 +0900
Branch: REL_10_STABLE [c90096009] 2019-08-27 09:11:48 +0900
Branch: REL9_6_STABLE [c4d75313e] 2019-08-27 09:12:10 +0900
Branch: REL9_5_STABLE [4ed3bda49] 2019-08-27 09:12:14 +0900

    Fix failure of --jobs with vacuumdb on Windows
    
    FD_SETSIZE needs to be declared before winsock2.h, or it is possible to
    run into buffer overflow issues when using --jobs.  This is similar to
    pgbench's solution done in a23c641.
    
    This has been introduced by 71d84ef, and older versions have been using
    the default value of FD_SETSIZE, defined at 64.  While on it, add a
    missing newline to the previously-added error message.
    
    Per buildfarm member jacana, but this impacts all Windows animals
    running the TAP tests.  I have reproduced the failure locally to check
    the patch.
    
    Author: Michael Paquier
    Reviewed-by: Andrew Dunstan
    Discussion: https://postgr.es/m/20190826054000.GE7005@paquier.xyz
    Backpatch-through: 9.5

Author: Michael Paquier <michael@paquier.xyz>
Branch: REL_12_STABLE [b783a38d4] 2019-08-27 09:11:38 +0900

    Fix failure of --jobs with vacuumdb on Windows
    
    FD_SETSIZE needs to be declared before winsock2.h, or it is possible to
    run into buffer overflow issues when using --jobs.  This is similar to
    pgbench's solution done in a23c641.
    
    This has been introduced by 71d84ef, and older versions have been using
    the default value of FD_SETSIZE, defined at 64.
    
    Per buildfarm member jacana, but this impacts all Windows animals
    running the TAP tests.  I have reproduced the failure locally to check
    the patch.
    
    Author: Michael Paquier
    Reviewed-by: Andrew Dunstan
    Discussion: https://postgr.es/m/20190826054000.GE7005@paquier.xyz
    Backpatch-through: 9.5

Author: Michael Paquier <michael@paquier.xyz>
Branch: master [9acda7311] 2019-08-27 09:11:31 +0900

    Fix failure of --jobs with reindexdb and vacuumdb on Windows
    
    FD_SETSIZE needs to be declared before winsock2.h, or it is possible to
    run into buffer overflow issues when using --jobs.  This is similar to
    pgbench's solution done in a23c641.
    
    This has been introduced by 71d84ef, and older versions have been using
    the default value of FD_SETSIZE, defined at 64.
    
    Per buildfarm member jacana, but this impacts all Windows animals
    running the TAP tests.  I have reproduced the failure locally to check
    the patch.
    
    Author: Michael Paquier
    Reviewed-by: Andrew Dunstan
    Discussion: https://postgr.es/m/20190826054000.GE7005@paquier.xyz
    Backpatch-through: 9.5


Thanks!


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Aug 27, 2019 at 08:29:39AM -0400, Alvaro Herrera wrote:
> BTW one nit: in deference to src/tools/git_changelog, it would be better
> to make all branch commit messages exactly the same, and if you want to
> make per-branch distinctions make them in the unified text ("On branches
> so and so, also add a newline", etc) rather than writing per branch
> slightly different messages.

I see.  Thanks for letting me know.
--
Michael

On Mon Aug 19, 2019 at 2:52 PM Michael Paquier wrote:
> On Mon, Aug 19, 2019 at 07:32:21AM +0200, Mikael Kjellstr=F6m wrote:
> > I could set up a OpenBSD 6.5 animal.  I have resources available.
> > Will do it the next coming days.
>
> Thanks!  That's great.

I've setup an OpenBSD -current machine and it will build every 12 hours.
Animal name: scorpionfly

If you all think the config options should be modified, just let me know.

> --
> Michael

"Jungle Boogie" <jungleboogie0@gmail.com> writes:
> I've setup an OpenBSD -current machine and it will build every 12 hours.
> Animal name: scorpionfly

Cool, thanks!

Looks like it's only building HEAD though?

            regards, tom lane

On 2019-08-28 22:27, Tom Lane wrote:
> "Jungle Boogie" <jungleboogie0@gmail.com> writes:
>> I've setup an OpenBSD -current machine and it will build every 12 hours.
>> Animal name: scorpionfly
> 
> Cool, thanks!
> 
> Looks like it's only building HEAD though?

I have one ready to go also with 6.5 and clang but no name yet as nobody 
seems to be checking the email for that.  Hint hint!!!

I can set it up to build all active branches.

/Mikael

On Wed, Aug 28, 2019 at 10:48:42PM +0200, Mikael Kjellström wrote:
> I have one ready to go also with 6.5 and clang but no name yet as nobody
> seems to be checking the email for that.  Hint hint!!!
>
> I can set it up to build all active branches.

Thanks a lot for doing that.
--
Michael

On 2019-08-29 02:00, Michael Paquier wrote:

>> I can set it up to build all active branches.
> 
> Thanks a lot for doing that.

OK, now it's set up and it should report in for each branch during the 
next 8+ of hours.

/Mikael