Thread

Commits

Use ppoll(2), if available, to wait for input in pgbench.
- 60e612b60299 12.0 landed
Use pselect(2) not select(2), if available, to wait in postmaster's loop.
- 81069a9efc5a 10.0 cited

PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-01-22T08:34:06Z

This version of the patch attempts to address the feedback for the previous
submission on 28-Nov-2017

This patch enables building pgbench to use ppoll() instead of select()
to allow for more than (FD_SETSIZE - 10) connections.  As implemented,
when using ppoll(), the only connection limitation is system resources.

“… ppoll() is to poll() as pselect() is to select().  Since the
existing code uses select(), why not convert to poll() rather than
ppoll()?”

Time in pgbench is measured in microseconds.
The select() uses microseconds.
The ppoll() and pselect() call use nanoseconds
The poll() call uses milliseconds.
In order to not downgrade time resolution, ppoll() is used instead of poll().

Without this patch, one is limited to '(FD_SETSIZE - 10)’ number of connections.
Example of something that fails without this patch but works with the patch:

Without the patch:

$ pgbench -j 1500 -c 1500
invalid number of clients: "1500"


With the patch:

$ pgbench -j 1500 -c 1500
starting vacuum...end.
progress: 12.0 s, 782.3 tps, lat 917.507 ms stddev 846.929
transaction type: <builtin: TPC-B (sort of)>
scaling factor: 2000
query mode: simple
number of clients: 1500
number of threads: 1500
number of transactions per client: 10
number of transactions actually processed: 15000/15000
latency average = 1180.216 ms
latency stddev = 855.126 ms
tps = 658.674816 (including connections establishing)
tps = 765.289160 (excluding connections establishing)


--
Doug Rady
Amazon Aurora, RDS PostgreSQL
radydoug@amazon.com

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-01-24T08:00:07Z

Hello Doug,

> This version of the patch attempts to address the feedback for the 
> previous submission on 28-Nov-2017

Please avoid recreating a thread, but rather respond to the previous one, 
I missed this post.

The overall function-based implementation with limited ifdefs seems 
readable and maintainable. I think that it rather improves the code in 
places by hiding details.

Patch applies with one warning:

   pgbench11-ppoll-v6.patch:141: trailing whitespace.
      set_socket(sockets, PQsocket(state[i].con), i);<SPACE>
   warning: 1 line adds whitespace errors.

The patch implies to run "autoconf" to regenerate the configure script.

Compilation with "ppoll" ok, globale & local make check ok. I do not have 
hardware which allows to run with over 1024 clients, so I cannot say that 
I tested the case.

Compilation without "ppoll" gets a warning:

   pgbench.c:5645:1: warning: ‘clear_socket’ defined but not used...
     clear_socket(socket_array *sa, int fd, int idx)

The "clear_socket" function is not used in this case. I'd suggest to 
remove it from the prototypes, remove or comment the unused implementation 
in the select section, and keep the one in the ppoll section. Or use it if 
it is needed. Or inline it in the "init_socket_array" function where it is 
used just once.

I'm not sure of the name "socket_array", because it is an array only in 
the ppoll case. Maybe "sockets_t" or "socket_set"? Or something else?

Maybe "init_socket_array" should be named "clear_...".

I would suggest not to include sys/select.h when using ppoll, as it is a useless
include this case. I.e. move includes in the ifdef USE_PPOLL section?

Please do not remove comments, eg:

   -  /* identify which client sockets should be checked for input */

On #endif or #else, especially large scope ones, I would have a comment to 
say about what it is, eg at the minimum:
   #else /* select(2) implementation */
   #endif /* USE_PPOLL */

On:
  +#if defined(USE_PPOLL)
  +#ifdef POLLRDHUP

Use just one "ifdef" style?

There should be a comment about what this sections are providing, eg:
   /* ppoll(2) implementation for "socket_array" functions */

There should be an empty line and possibly a comment explaining why
POLLRDHUP may not be there and/or why this define is necessary.

With select you always initialize the timeout, but not with ppoll.
Use a common approach in the implementations?

The "finishCon" function addition seems totally unrelated to this patch. 
Although I agree that this function improves the code, it is refactoring 
and does not really belong here.

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-01-25T15:05:52Z

On 1/24/18, 00:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:
    
    Hello Doug,

Hello Fabien,
   
    > This version of the patch attempts to address the feedback for the 
    > previous submission on 28-Nov-2017
    
    Please avoid recreating a thread, but rather respond to the previous one, 
    I missed this post.

Got it.
    
    The overall function-based implementation with limited ifdefs seems 
    readable and maintainable. I think that it rather improves the code in 
    places by hiding details.
    
    Patch applies with one warning:
    
       pgbench11-ppoll-v6.patch:141: trailing whitespace.
          set_socket(sockets, PQsocket(state[i].con), i);<SPACE>
       warning: 1 line adds whitespace errors.

Fixed.
    
    The patch implies to run "autoconf" to regenerate the configure script.

Yes. I should have included this in the description.
    
    Compilation with "ppoll" ok, globale & local make check ok. I do not have 
    hardware which allows to run with over 1024 clients, so I cannot say that 
    I tested the case.
    
    Compilation without "ppoll" gets a warning:
    
       pgbench.c:5645:1: warning: ‘clear_socket’ defined but not used...
         clear_socket(socket_array *sa, int fd, int idx)
    
    The "clear_socket" function is not used in this case. I'd suggest to 
    remove it from the prototypes, remove or comment the unused implementation 
    in the select section, and keep the one in the ppoll section. Or use it if 
    it is needed. Or inline it in the "init_socket_array" function where it is 
    used just once.

"clear_socket" removed. Functionality inlined into "init_socket_array"
    
    I'm not sure of the name "socket_array", because it is an array only in 
    the ppoll case. Maybe "sockets_t" or "socket_set"? Or something else?

Changed "socket_array" to "socket_set"
    
    Maybe "init_socket_array" should be named "clear_...".

Renamed "init_socket_array" to "clear_socket_set"
    
    I would suggest not to include sys/select.h when using ppoll, as it is a useless
    include this case. I.e. move includes in the ifdef USE_PPOLL section?

Done. Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.
    
    Please do not remove comments, eg:
    
       -  /* identify which client sockets should be checked for input */

Restored.
    
    On #endif or #else, especially large scope ones, I would have a comment to 
    say about what it is, eg at the minimum:
       #else /* select(2) implementation */
       #endif /* USE_PPOLL */
    
    On:
      +#if defined(USE_PPOLL)
      +#ifdef POLLRDHUP
    
    Use just one "ifdef" style?
    
    There should be a comment about what this sections are providing, eg:
       /* ppoll(2) implementation for "socket_array" functions */
    
    There should be an empty line and possibly a comment explaining why
    POLLRDHUP may not be there and/or why this define is necessary.

Removed unneeded #ifdef around POLLRDHUP
    
    With select you always initialize the timeout, but not with ppoll.
    Use a common approach in the implementations?

Fixed init & passing of timeout.
    
    The "finishCon" function addition seems totally unrelated to this patch. 
    Although I agree that this function improves the code, it is refactoring 
    and does not really belong here.

Removed.
    
    -- 
    Fabien.

Thank you!!
doug

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-01-25T15:16:41Z
```
ISTM that the v7 patch version you sent is identical to v6.

-- 
Fabien.
```

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-01-25T15:34:07Z

This time with the revised patch file:  pgbench11-ppoll-v8.patch

On 1/24/18, 00:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:
    
    Hello Doug,

Hello Fabien,
  
    > This version of the patch attempts to address the feedback for the
    > previous submission on 28-Nov-2017
    
    Please avoid recreating a thread, but rather respond to the previous one,
    I missed this post.

Got it.
    
    The overall function-based implementation with limited ifdefs seems
    readable and maintainable. I think that it rather improves the code in
    places by hiding details.
    
    Patch applies with one warning:
    
       pgbench11-ppoll-v6.patch:141: trailing whitespace.
          set_socket(sockets, PQsocket(state[i].con), i);<SPACE>
       warning: 1 line adds whitespace errors.

Fixed.
    
    The patch implies to run "autoconf" to regenerate the configure script.

Yes. I should have included this in the description.
    
    Compilation with "ppoll" ok, globale & local make check ok. I do not have
    hardware which allows to run with over 1024 clients, so I cannot say that
    I tested the case.
    
    Compilation without "ppoll" gets a warning:
    
       pgbench.c:5645:1: warning: ‘clear_socket’ defined but not used...
         clear_socket(socket_array *sa, int fd, int idx)
    
    The "clear_socket" function is not used in this case. I'd suggest to
    remove it from the prototypes, remove or comment the unused implementation
    in the select section, and keep the one in the ppoll section. Or use it if
    it is needed. Or inline it in the "init_socket_array" function where it is
    used just once.

"clear_socket" removed. Functionality inlined into "init_socket_array"
    
    I'm not sure of the name "socket_array", because it is an array only in
    the ppoll case. Maybe "sockets_t" or "socket_set"? Or something else?

Changed "socket_array" to "socket_set"
    
    Maybe "init_socket_array" should be named "clear_...".

Renamed "init_socket_array" to "clear_socket_set"
    
    I would suggest not to include sys/select.h when using ppoll, as it is a useless
    include this case. I.e. move includes in the ifdef USE_PPOLL section?

Done. Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.
    
    Please do not remove comments, eg:
    
       -  /* identify which client sockets should be checked for input */

Restored.
    
    On #endif or #else, especially large scope ones, I would have a comment to
    say about what it is, eg at the minimum:
       #else /* select(2) implementation */
       #endif /* USE_PPOLL */
    
    On:
      +#if defined(USE_PPOLL)
      +#ifdef POLLRDHUP
    
    Use just one "ifdef" style?
    
    There should be a comment about what this sections are providing, eg:
       /* ppoll(2) implementation for "socket_array" functions */
    
    There should be an empty line and possibly a comment explaining why
    POLLRDHUP may not be there and/or why this define is necessary.

Removed unneeded #ifdef around POLLRDHUP
    
    With select you always initialize the timeout, but not with ppoll.
    Use a common approach in the implementations?

Fixed init & passing of timeout.
    
    The "finishCon" function addition seems totally unrelated to this patch.
    Although I agree that this function improves the code, it is refactoring
    and does not really belong here.

Removed.
    
    --
    Fabien.

Thank you!!
doug

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-01-25T22:46:24Z

Hello Doug,

> This time with the revised patch file:  pgbench11-ppoll-v8.patch

Patch applies cleanly. Compiles cleanly and runs fine in both ppoll & 
select cases.

I'm okay with having a preferred ppoll implementation because of its improved
capability.

A few minor additional comments/suggestions:

Cpp has an #elif that could be used to manage the ppoll/select alternative.
It is already used elsewhere in the file. Or not.

I must admit that I'm not fond of the alloc_socket_set trick with MAXCLIENTS,
especially without any comment. I'd suggest to just have two distinct functions
in their corresponding sections.

I would add a comment that free_socket_set code is common to both 
versions, and maybe consider moving it afterwards. Or maybe just duplicate 
if in each section for homogeneity.

It looks like error_on_socket and ignore_socket should return a boolean instead
of an int. Also, maybe simplify the implementation of the later by avoiding
the ?: expression.

ISTM that the error_on_socket function in the ppoll case deserves some 
comments, especially on the condition.


> [...] Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.

I'm okay with that. I'm wondering whether there should be a way to force 
using one or the other when both are available. Not sure.

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-01-26T22:27:47Z

On 1/25/18, 14:46, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:

    
    Hello Doug,

Hello Fabien,
    
    > This time with the revised patch file:  pgbench11-ppoll-v8.patch
    
    Patch applies cleanly. Compiles cleanly and runs fine in both ppoll & 
    select cases.
    
    I'm okay with having a preferred ppoll implementation because of its improved
    capability.
    
    A few minor additional comments/suggestions:
    
    Cpp has an #elif that could be used to manage the ppoll/select alternative.
    It is already used elsewhere in the file. Or not.
    
    I must admit that I'm not fond of the alloc_socket_set trick with MAXCLIENTS,
    especially without any comment. I'd suggest to just have two distinct functions
    in their corresponding sections.

Made a distinct function for each section.
  
    I would add a comment that free_socket_set code is common to both 
    versions, and maybe consider moving it afterwards. Or maybe just duplicate 
    if in each section for homogeneity.

Duplicated.
    
    It looks like error_on_socket and ignore_socket should return a boolean instead
    of an int. Also, maybe simplify the implementation of the later by avoiding
    the ?: expression.
    
    ISTM that the error_on_socket function in the ppoll case deserves some 
    comments, especially on the condition.

Added comment. Changed output to be more compatible with socket() error check.
Extra ppoll() specific error info only output when debug flag set ... not sure if this is OK or should just remove the extra info.
    
    
    > [...] Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.
    
    I'm okay with that. I'm wondering whether there should be a way to force 
    using one or the other when both are available. Not sure.

Added option to force use of select(2) via:  -DUSE_SELECT
    
    -- 
    Fabien.
    
Thank you!!
doug

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-01-26T23:00:48Z

Hello Doug,

Patch applies, compiles, tests ok.

>    > [...] Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.
>
>    I'm okay with that. I'm wondering whether there should be a way to force
>    using one or the other when both are available. Not sure.
>
> Added option to force use of select(2) via:  -DUSE_SELECT

USE_SELECT could mean something somewhere. Maybe use something more 
specific like PGBENCH_USE_SELECT? Having this macro available simplifies 
testing.

I'm not sure why you do the following trick, could you explain?
   +#undef USE_SELECT
   +#define USE_SELECT

In the select implementation you do:

  return (socket_set *) pg_malloc0(sizeof(socket_set) * MAXCLIENTS);

but ISTM that socket_set is already an fd_set which represents a set of 
clients, so allocating a number of it is needed. The initial 
implementation just does "fs_set input_mask", whetever the number of 
clients, and it works fine.

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-01-28T23:02:57Z

On 1/26/18, 15:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:
        Hello Doug,

Hello Fabien,
    
    Patch applies, compiles, tests ok.
    
    >    > [...] Replaced USE_PPOLL with HAVE_PPOLL as having both seems redundant.
    >
    >    I'm okay with that. I'm wondering whether there should be a way to force
    >    using one or the other when both are available. Not sure.
    >
    > Added option to force use of select(2) via:  -DUSE_SELECT
    
    USE_SELECT could mean something somewhere. Maybe use something more 
    specific like PGBENCH_USE_SELECT? Having this macro available simplifies 
    testing.

Changed to PGBENCH_USE_SELECT
    
    I'm not sure why you do the following trick, could you explain?
       +#undef USE_SELECT
       +#define USE_SELECT

This was due to compiler complaint about USE_SELECT being redefined.
Have replaced that "trick" with a new #define POLL_USING_SELECT which is used elsewhere in pgbench instead.
    
    In the select implementation you do:
    
      return (socket_set *) pg_malloc0(sizeof(socket_set) * MAXCLIENTS);
    
    but ISTM that socket_set is already an fd_set which represents a set of 
    clients, so allocating a number of it is needed. The initial 
    implementation just does "fs_set input_mask", whetever the number of 
    clients, and it works fine.

Ugh. Yes, for socket() only one (1) fd_set is needed.
Fixed.
    
    -- 
    Fabien.

Thank you, again!!
doug

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-01-29T08:58:59Z

Hello Doug,

>    I'm not sure why you do the following trick, could you explain?
>       +#undef USE_SELECT
>       +#define USE_SELECT
>
> This was due to compiler complaint about USE_SELECT being redefined.
> Have replaced that "trick" with a new #define POLL_USING_SELECT which is used elsewhere in pgbench instead.

Ok, why not.

Another option to avoid the warning and a new name could have been to 
"#ifndef X #define X #endif /* !X */"

Patch applies cleanly, compiles cleanly for both options, local & global 
"make check" ok.

Switched to "Ready".

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andres Freund <andres@anarazel.de> — 2018-03-01T08:34:04Z

On 2018-01-28 23:02:57 +0000, Rady, Doug wrote:
> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index 31ea6ca06e..689f15a772 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -44,7 +44,13 @@
>  #include <signal.h>
>  #include <time.h>
>  #include <sys/time.h>
> -#ifdef HAVE_SYS_SELECT_H
> +#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
> +#undef HAVE_PPOLL
> +#endif
> +#ifdef HAVE_PPOLL
> +#include <poll.h>
> +#elif defined(HAVE_SYS_SELECT_H)
> +#define POLL_USING_SELECT

(random thing noticed while going through patches)

It strikes me as a bad idea to undefine configure selected
symbols. Postgres header might rely on them. It also strikes me as
entirely unnecessary here.

- Andres

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-03-01T10:30:39Z

>> -#ifdef HAVE_SYS_SELECT_H
>> +#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
>> +#undef HAVE_PPOLL
>> +#endif
>> +#ifdef HAVE_PPOLL
>> +#include <poll.h>
>> +#elif defined(HAVE_SYS_SELECT_H)
>> +#define POLL_USING_SELECT
>
> (random thing noticed while going through patches)
>
> It strikes me as a bad idea to undefine configure selected
> symbols. Postgres header might rely on them. It also strikes me as
> entirely unnecessary here.

Yes, I though about this one but let it pass. Indeed, it would be 
sufficient to not load "poll.h" when select is forced, without undefining 
the configure setting.

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andres Freund <andres@anarazel.de> — 2018-03-04T00:14:09Z

On 2018-03-01 11:30:39 +0100, Fabien COELHO wrote:
> 
> > > -#ifdef HAVE_SYS_SELECT_H
> > > +#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
> > > +#undef HAVE_PPOLL
> > > +#endif
> > > +#ifdef HAVE_PPOLL
> > > +#include <poll.h>
> > > +#elif defined(HAVE_SYS_SELECT_H)
> > > +#define POLL_USING_SELECT
> > 
> > (random thing noticed while going through patches)
> > 
> > It strikes me as a bad idea to undefine configure selected
> > symbols. Postgres header might rely on them. It also strikes me as
> > entirely unnecessary here.
> 
> Yes, I though about this one but let it pass. Indeed, it would be sufficient
> to not load "poll.h" when select is forced, without undefining the configure
> setting.

I've marked the CF entry waiting on author.

Greetings,

Andres Freund

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-03-21T05:42:33Z

Updated the patch to not do the #undef

pgbench11-ppoll-v11.patch attached.

Thanks,
doug



On 3/3/18, 16:14, "Andres Freund" <andres@anarazel.de> wrote:

    On 2018-03-01 11:30:39 +0100, Fabien COELHO wrote:
    > 
    > > > -#ifdef HAVE_SYS_SELECT_H
    > > > +#ifdef PGBENCH_USE_SELECT			/* force use of select(2)? */
    > > > +#undef HAVE_PPOLL
    > > > +#endif
    > > > +#ifdef HAVE_PPOLL
    > > > +#include <poll.h>
    > > > +#elif defined(HAVE_SYS_SELECT_H)
    > > > +#define POLL_USING_SELECT
    > > 
    > > (random thing noticed while going through patches)
    > > 
    > > It strikes me as a bad idea to undefine configure selected
    > > symbols. Postgres header might rely on them. It also strikes me as
    > > entirely unnecessary here.
    > 
    > Yes, I though about this one but let it pass. Indeed, it would be sufficient
    > to not load "poll.h" when select is forced, without undefining the configure
    > setting.
    
    I've marked the CF entry waiting on author.
    
    Greetings,
    
    Andres Freund

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-03-25T11:00:20Z

Hello Doug,

> Updated the patch to not do the #undef
> pgbench11-ppoll-v11.patch attached.

Patch applies. Do not forget to regenerate configure to test...

I've compiled and run with both ppoll & select options without issues.

Two quite minor style comment (at least 2 instances):

   if (cond) return false; else return true;

ISTM that the simpler the better:

   return !cond;

Also ISTM that the following does not comply with pg C style expectations 
(idem, 2 instances):

   } else {


-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Rady, Doug <radydoug@amazon.com> — 2018-03-26T20:23:48Z

On 3/25/18, 04:00, "Fabien COELHO" <coelho@cri.ensmp.fr> wrote:

Hello Fabien,
    
    Hello Doug,
    
    > Updated the patch to not do the #undef
    > pgbench11-ppoll-v11.patch attached.
    
    Patch applies. Do not forget to regenerate configure to test...
    
    I've compiled and run with both ppoll & select options without issues.
    
    Two quite minor style comment (at least 2 instances):
    
       if (cond) return false; else return true;
    
    ISTM that the simpler the better:
    
       return !cond;
 Fixed.
   
    Also ISTM that the following does not comply with pg C style expectations 
    (idem, 2 instances):
    
       } else {
 Fixed.

Also fixed issue with 'timeout' not being passed as NULL when no timeout time.
    
    -- 
    Fabien.

Thanks!
doug

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-03-27T09:06:33Z

Hello Doug,

>> I've compiled and run with both ppoll & select options without issues. 
>> Two quite minor style comment (at least 2 instances):

> Fixed. Fixed. Also fixed issue with 'timeout' not being passed as NULL 
> when no timeout time.

v12 compiled and tested with both ppoll & select (by forcing). All seems 
ok to me.

Switched back to "ready".

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andres Freund <andres@anarazel.de> — 2018-04-06T21:15:28Z

Hi,

I'm still not particularly happy with this.  Checking whether I can
polish it up.

a) the new function names are partially non-descriptive and their
   meaning is undocumented.  As an extreme example:

-				if (!FD_ISSET(sock, &input_mask))
+				if (ignore_socket(sockets, i, st->con))
					continue;

   reading the new code it's entirely unclear what that could mean. Are
   you marking the socket as ignored? What does ignored even mean?

   There's not a single comment on what the new functions mean. It's not
   that bad if there's no docs on what FD_ISSET implies, because that's a
   well known and documented interface. But introducing an abstraction
   without any comments on it?

b) Does this actually improve the situation all that much? We still loop
   over every socket:

		/* ok, advance the state machine of each connection */
		for (i = 0; i < nstate; i++)
		{
			CState	   *st = &state[i];

			if (st->state == CSTATE_WAIT_RESULT)
			{
				/* don't call doCustom unless data is available */

				if (error_on_socket(sockets, i, st->con))
					goto done;

				if (ignore_socket(sockets, i, st->con))
					continue;
			}
			else if (st->state == CSTATE_FINISHED ||
					 st->state == CSTATE_ABORTED)
			{
				/* this client is done, no need to consider it anymore */
				continue;
			}

			doCustom(thread, st, &aggs);

			/* If doCustom changed client to finished state, reduce remains */
			if (st->state == CSTATE_FINISHED || st->state == CSTATE_ABORTED)
				remains--;
		}

   if the goal is to make this more scalable, wouldn't this require
   using a proper polling mechanism that supports signalling the
   sockets with relevant changes, rather than busy-looping through every
   socket if there's just a single change?

   I kinda wonder if the proper fix wouldn't be to have one patch making
   WaitEventSets usable from frontend code, and then make this code use
   them.  Not a small project though.

Greetings,

Andres Freund

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Tom Lane <tgl@sss.pgh.pa.us> — 2018-04-06T21:49:19Z

Andres Freund <andres@anarazel.de> writes:
> I'm still not particularly happy with this.

I'm a bit confused as to what the point is.  It seems unlikely that one
pgbench process can effectively drive enough backends for select's
limitations to really be an issue.

			regards, tom lane

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andres Freund <andres@anarazel.de> — 2018-04-06T21:54:14Z

On 2018-04-06 17:49:19 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I'm still not particularly happy with this.
>
> I'm a bit confused as to what the point is.  It seems unlikely that one
> pgbench process can effectively drive enough backends for select's
> limitations to really be an issue.

It's not that crazy to use more than 1024 connections (common FD_SETSIZE
valu), especially over a higher latency connection.

As I wrote, I think using a poll API that doesn't require looping over
all sockets, even if they didn't get an event, would be a better plan.

- Andres

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Konstantin Knizhnik <k.knizhnik@postgrespro.ru> — 2018-04-07T05:20:35Z

On Apr 7, 2018, at 12:49 AM, Tom Lane wrote:

> Andres Freund <andres@anarazel.de> writes:
>> I'm still not particularly happy with this.
> 
> I'm a bit confused as to what the point is.  It seems unlikely that one
> pgbench process can effectively drive enough backends for select's
> limitations to really be an issue.

pgbench is multithreaded application, so in theory it can drive almost arbitrary number of connections.
It is limited only by network throughput, but if pgbench is launched at the same host and connected  to the server through loopback or unix sockets,
then network is also not a limit.
We quite often have to spawn more than 1k connections and SMP systems with hundreds of CPU.
So there are two choices: either use patched version of pgbench which is using poll, either spawn several instances of pgbench (which is not always convenient).

> 
> 			regards, tom lane
>

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Thomas Munro <thomas.munro@enterprisedb.com> — 2018-05-17T05:23:37Z

On Tue, Mar 27, 2018 at 9:23 AM, Rady, Doug <radydoug@amazon.com> wrote:
> pgbench11-ppoll-v12.patch

Hi Doug,

FYI this patch is trying and failing to use ppoll() on Windows:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.30

-- 
Thomas Munro
http://www.enterprisedb.com

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-07-03T23:52:48Z


On 05/17/2018 01:23 AM, Thomas Munro wrote:
> On Tue, Mar 27, 2018 at 9:23 AM, Rady, Doug <radydoug@amazon.com> wrote:
>> pgbench11-ppoll-v12.patch
> Hi Doug,
>
> FYI this patch is trying and failing to use ppoll() on Windows:
>
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.30
>


It's still failing -  see 
<https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4098>

I'm setting this back to "Waiting on Author" until that's fixed.

cheers

andrew

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

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-08-09T16:45:09Z


On 07/03/2018 07:52 PM, Andrew Dunstan wrote:
>
>
> On 05/17/2018 01:23 AM, Thomas Munro wrote:
>> On Tue, Mar 27, 2018 at 9:23 AM, Rady, Doug <radydoug@amazon.com> wrote:
>>> pgbench11-ppoll-v12.patch
>> Hi Doug,
>>
>> FYI this patch is trying and failing to use ppoll() on Windows:
>>
>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.30
>>
>
>
> It's still failing -  see 
> <https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4098>
>
> I'm setting this back to "Waiting on Author" until that's fixed.
>


The author hasn't replied, but the attached seems to have cured the 
bitrot so that it at least applies. Let's see what the cfbot makes of it 
and then possibly fix any Windows issues.


cheers

andrew

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

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-08-09T21:46:10Z


On 08/09/2018 12:45 PM, Andrew Dunstan wrote:
>
>
> On 07/03/2018 07:52 PM, Andrew Dunstan wrote:
>>
>>
>> On 05/17/2018 01:23 AM, Thomas Munro wrote:
>>> On Tue, Mar 27, 2018 at 9:23 AM, Rady, Doug <radydoug@amazon.com> 
>>> wrote:
>>>> pgbench11-ppoll-v12.patch
>>> Hi Doug,
>>>
>>> FYI this patch is trying and failing to use ppoll() on Windows:
>>>
>>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.30 
>>>
>>>
>>
>>
>> It's still failing -  see 
>> <https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4098>
>>
>> I'm setting this back to "Waiting on Author" until that's fixed.
>>
>
>
> The author hasn't replied, but the attached seems to have cured the 
> bitrot so that it at least applies. Let's see what the cfbot makes of 
> it and then possibly fix any Windows issues.
>
>
>


And there's still a Windows problem which I think is cured in the 
attached patch

cheers

andrew


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

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2018-08-10T01:32:58Z


On 08/09/2018 05:46 PM, Andrew Dunstan wrote:
>
>
> On 08/09/2018 12:45 PM, Andrew Dunstan wrote:
>>
>>
>> On 07/03/2018 07:52 PM, Andrew Dunstan wrote:
>>>
>>>
>>> On 05/17/2018 01:23 AM, Thomas Munro wrote:
>>>> On Tue, Mar 27, 2018 at 9:23 AM, Rady, Doug <radydoug@amazon.com> 
>>>> wrote:
>>>>> pgbench11-ppoll-v12.patch
>>>> Hi Doug,
>>>>
>>>> FYI this patch is trying and failing to use ppoll() on Windows:
>>>>
>>>> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.30 
>>>>
>>>>
>>>
>>>
>>> It's still failing -  see 
>>> <https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.4098>
>>>
>>> I'm setting this back to "Waiting on Author" until that's fixed.
>>>
>>
>>
>> The author hasn't replied, but the attached seems to have cured the 
>> bitrot so that it at least applies. Let's see what the cfbot makes of 
>> it and then possibly fix any Windows issues.
>>
>>
>>
>
>
> And there's still a Windows problem which I think is cured in the 
> attached patch
>


and the CFBot agrees.

cheers

andrew

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

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Fabien COELHO <coelho@cri.ensmp.fr> — 2018-09-15T13:59:08Z

>> The author hasn't replied, but the attached seems to have cured the 
>> bitrot so that it at least applies. Let's see what the cfbot makes of 
>> it and then possibly fix any Windows issues.

The patch was not applying cleanly anymore for me, so here is a rebase of 
your latest version.

Morever, ISTM that Tom's "why?" question has been answered: there are very 
large systems out there with many processors, which are tested against 
many connections, exceeding select limit.

I have turned back this patch to ready.

-- 
Fabien.

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-19T21:46:02Z

"Rady, Doug" <radydoug@amazon.com> writes:
> This patch enables building pgbench to use ppoll() instead of select()
> to allow for more than (FD_SETSIZE - 10) connections.  As implemented,
> when using ppoll(), the only connection limitation is system resources.

So ... why exactly is this patch insisting on ppoll() rather than just
plain poll()?  AFAICS, all you're doing with that is being able to
specify the timeout in microsec not millisec, which does not really
justify taking much of a hit in portability, to my mind.

> “… ppoll() is to poll() as pselect() is to select().  Since the
> existing code uses select(), why not convert to poll() rather than
> ppoll()?”

A moment's glance at our git history will remind you that we attempted
to start using pselect() last year, and the attempt crashed and burned:

Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: master Release: REL_10_BR [64925603c] 2017-04-24 18:29:03 -0400

    Revert "Use pselect(2) not select(2), if available, to wait in postmaster's loop."

    This reverts commit 81069a9efc5a374dd39874a161f456f0fb3afba4.

    Buildfarm results suggest that some platforms have versions of pselect(2)
    that are not merely non-atomic, but flat out non-functional.  Revert the
    use-pselect patch to confirm this diagnosis (and exclude the no-SA_RESTART
    patch as the source of trouble).  If it's so, we should probably look into
    blacklisting specific platforms that have broken pselect.

    Discussion: https://postgr.es/m/9696.1493072081@sss.pgh.pa.us

Now, it might be that ppoll doesn't suffer from as many portability
problems as pselect, but I don't see a good reason to assume that.
So I'd rather see if we can't convert this to use poll(), and thereby
ensure that it works basically everywhere.

			regards, tom lane

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-21T15:44:10Z

I wrote:
> So ... why exactly is this patch insisting on ppoll() rather than just
> plain poll()?  AFAICS, all you're doing with that is being able to
> specify the timeout in microsec not millisec, which does not really
> justify taking much of a hit in portability, to my mind.

To check into my assumptions here, I did a bit of testing to find out
what ppoll can really do in terms of timeout accuracy.  As best I can
tell, its resolution is on the order of 100 usec on both fairly new
Linux kernels (4.17.19 on Fedora 28) and fairly old ones (2.6.32 on
RHEL6).  So there's not really that much daylight between what you can
do with ppoll() and with poll()'s millisecond resolution.  select(2)
seems to have about the same effective timeout resolution.

Interestingly, select(2)'s timeout resolution is noticeably better than
that, around 10 usec, on recent macOS.  Didn't try other platforms.

Also, I'd misunderstood the portability angle.  Unlike pselect(),
ppoll() is *not* in POSIX.  It started as a Linux-ism, although
I see it's appeared recently in FreeBSD.  That somewhat assuages
my fear of broken implementations on specific platforms, but it
also means that it's going to be far from universally available.

So the question here really boils down to whether you think that a
large set of pgbench connections is interesting on non-Linux platforms.
There's a case to be made that it isn't, perhaps, but I'm not exactly
sold.

On the other hand, while we could certainly make a poll-based code path
within this patch, I'm not quite sure what we'd do with it.  My results
for macOS indicate that using poll rather than select would create a
tradeoff: in return for possibly allowing more clients, there would be
a definite loss in timeout precision.  I don't think that limiting
\sleep commands to ms precision would be so awful, but it's easier
to believe that loss of precision in the transaction dispatch rate for
"--rate" tests could be a problem for some people.  So we might have
to expose the choice of which call to use to users, which seems like
a mess.

So maybe what we've got here --- make it better on Linux, with no
change elsewhere --- is about as good as we can hope for.

Also, I notice that the kevent syscall available on macOS and some
BSDen uses a struct-timespec timeout parameter, ie, theoretical nsec
resolution same as ppoll.  So there's a case to be made that where
we should go for those platforms is to build a kevent-based code
path not a poll-based one.  It'd be unreasonable to insist on this
patch providing that, though.

Anyway, bottom line: my objection on Wednesday was mainly based on
the assumption that we might have to contend with broken ppoll on
some platforms, which now appears to be fallacious.  So, objection
withdrawn.

			regards, tom lane

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-22T16:09:40Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> The patch was not applying cleanly anymore for me, so here is a rebase of 
> your latest version.

The cfbot doesn't like that patch, probably because of the Windows newlines.
Here's a version with regular newlines, and some cosmetic cleanup in the
configure infrastructure.

I haven't looked at the pgbench changes proper yet, but I did quickly
test building on FreeBSD 11, which has ppoll, and it falls over:

pgbench.c:6080:69: error: use of undeclared identifier 'POLLRDHUP'
  ...== -1 || (PQsocket(con) >= 0 && !(sa[idx].revents & POLL_UNWANTED)))
                                                         ^
pgbench.c:6059:24: note: expanded from macro 'POLL_UNWANTED'
#define POLL_UNWANTED (POLLRDHUP|POLLERR|POLLHUP|POLLNVAL)
                       ^
pgbench.c:6085:42: error: use of undeclared identifier 'POLLRDHUP'
                        errno, sa[idx].fd, (sa[idx].revents & POLL_UNWANTED));
                                                              ^
pgbench.c:6059:24: note: expanded from macro 'POLL_UNWANTED'
#define POLL_UNWANTED (POLLRDHUP|POLLERR|POLLHUP|POLLNVAL)
                       ^
pgbench.c:6107:19: error: use of undeclared identifier 'POLLRDHUP'
        sa[idx].events = POLL_EVENTS;
                         ^
pgbench.c:6057:22: note: expanded from macro 'POLL_EVENTS'
#define POLL_EVENTS (POLLRDHUP|POLLIN|POLLPRI)
                     ^
3 errors generated.
make[3]: *** [<builtin>: pgbench.o] Error 1

I'm strongly tempted to just remove the POLL_UNWANTED business
altogether, as it seems both pointless and unportable on its face.
Almost by definition, we can't know what "other" bits a given
implementation might set.

I'm not entirely following the point of including POLLRDHUP in
POLL_EVENTS, either.  What's wrong with the traditional solution
of detecting EOF?

			regards, tom lane

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-23T00:11:34Z

I wrote:
> I'm strongly tempted to just remove the POLL_UNWANTED business
> altogether, as it seems both pointless and unportable on its face.
> Almost by definition, we can't know what "other" bits a given
> implementation might set.
> I'm not entirely following the point of including POLLRDHUP in
> POLL_EVENTS, either.  What's wrong with the traditional solution
> of detecting EOF?

So after studying that a bit longer, I think it's just wrong.
It's not the business of this code to be checking for connection
errors at all; that is libpq's province.  The libpq API specifies
that callers should wait for read-ready on the socket, and nothing
else.  So the only bit we need concern ourselves with is POLLIN.

I also seriously disliked both the details of the abstraction API
and its lack of documentation.  (Other people complained about that
upthread, too.)  So attached is a rewrite attempt.  There's still a
couple of grotty things about it; in particular the ppoll variant of
socket_has_input() knows more than one could wish about how it's being
used.  But I couldn't see a way to make it cleaner without significant
changes to the logic in threadRun, and that didn't seem better.

I think that Andres' concern upthread about iterating over a whole
lot of sockets is somewhat misplaced.  We aren't going to be iterating
over the entire set of client connections, only those being run by a
particular pgbench thread.  So assuming you're using a reasonable ratio
of threads to clients, there won't be very many to look at in any one
thread.  In any case, I'm dubious that we could get much of a win from
some other abstraction for waiting: both of these code paths do work
pretty much proportional to the number of connections the current
thread is responsible for, and it's hard to see how to avoid that.

I've tested this on both Linux and FreeBSD, and it seems to work fine.

I'm reasonably happy with this version of the patch, and would be
ready to commit it, but I thought I'd throw it out for another round
of review if anyone wants to.

			regards, tom lane