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

On Tue, Nov 28, 2017 at 10:38 AM, Rady, Doug <radydoug@amazon.com> wrote:
> 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.

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

+#ifdef USE_PPOLL
+#define SCKTWTMTHD "ppoll"
+#undef MAXCLIENTS
+#define POLL_EVENTS (POLLIN|POLLPRI|POLLRDHUP)
+#define POLL_FAIL (POLLERR|POLLHUP|POLLNVAL|POLLRDHUP)
+#define PFD_RESETEV(s) { do { if ((s)->pfdp) { (s)->pfdp->events =
POLL_EVENTS; (s)->pfdp->revents = 0; } } while(0); }
+#define PFD_ZERO(s) { do { if ((s)->pfdp) { (s)->pfdp->fd = -1;
(s)->pfdp->events = POLL_EVENTS; (s)->pfdp->revents = 0; } } while(0);
}
+#define PFD_SETFD(s) { do { (s)->pfdp->fd = PQsocket((s)->con); } while(0); }
+#define PFD_STRUCT_POLLFD(p)    struct pollfd   (p);
+#define PFD_THREAD_FREE(t) { do { if ((t)->pfds) {
pg_free((t)->pfds); (t)->pfds = NULL; } } while (0); }
+#define PFD_THREAD_INIT(t,s,n) { do { int _i; (t)->pfds = (struct
pollfd *) pg_malloc0(sizeof(struct pollfd) * (n)); for (_i = 0; _i <
(n); _i++) { (s)[_i].pfdp = &(t)->pfds[_i]; (s)[_i].pfdp->fd = -1; } }
while (0); }
+#else
+#define SCKTWTMTHD "select"
+#define PFD_RESETEV(s)
+#define PFD_ZERO(s)
+#define PFD_SETFD(s)
+#define PFD_STRUCT_POLLFD(p)
+#define PFD_THREAD_FREE(t)
+#define PFD_THREAD_INIT(t,s,n)
+#endif

I find this an impenetrable mess.  I am not going to commit a macro
with a ten-letter name that contains neither punctuation marks nor
vowels, nor one that is a single 200+ character that embeds a malloc
and a loop call but no comments.  While I agree with the general goal
of avoiding introducing lots of #ifdefs, and while I think that the
introduction of finishCon() seems like a possibly-useful step in that
direction, I don't think that having a bunch of long, complex,
comment-free macros like this actually improves code clarity.  Better
to have some embedded #ifdefs than this.

+                    if (debug)
+                        fprintf(stderr, "client %d connecting ...\n",
+                            st->id);
+

This is unrelated to the purpose of the patch.  Please don't mix in
unrelated changes.

+            if (debug)
+                fprintf(stderr, "client %d connecting\n",
+                    state[i].id);

Ditto.

IMHO, this looks like a broadly reasonable thing to do.  I had no idea
that FD_SETSIZE was ever set to a value much less than the number of
FDs the system could handle -- but if it is, then this seems like a
reasonable fix, once we agree on exactly how the details should look.

I don't remember off-hand whether you've done testing to see how
poll()/ppoll() performs compares to select(), but that might also be
something to check.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company