On Fri, Sep 28, 2018 at 11:09 AM Andres Freund <andres@anarazel.de> wrote:
> On 2018-09-28 10:55:13 +1200, Thomas Munro wrote:
> > Matteo Beccati reported a 5-10% performance drop on a
> > low-end Celeron NetBSD box which we have no explanation for, and we
> > have no reports from server-class machines on that OS -- so perhaps we
> > (or the NetBSD port?) should consider building with WAIT_USE_POLL on
> > NetBSD until someone can figure out what needs to be fixed there
> > (possibly on the NetBSD side)?
>
> Yea, I'm not too worried about that. It'd be great to test that, but
> otherwise I'm also ok to just plonk that into the template.
Thanks for the review! Ok, if we don't get a better idea I'll put
this in src/template/netbsd:
CPPFLAGS="$CPPFLAGS -DWAIT_USE_POLL"
> > @@ -576,6 +592,10 @@ CreateWaitEventSet(MemoryContext context, int nevents)
> > if (fcntl(set->epoll_fd, F_SETFD, FD_CLOEXEC) == -1)
> > elog(ERROR, "fcntl(F_SETFD) failed on epoll descriptor: %m");
> > #endif /* EPOLL_CLOEXEC */
> > +#elif defined(WAIT_USE_KQUEUE)
> > + set->kqueue_fd = kqueue();
> > + if (set->kqueue_fd < 0)
> > + elog(ERROR, "kqueue failed: %m");
> > #elif defined(WAIT_USE_WIN32)
>
> Is this automatically opened with some FD_CLOEXEC equivalent?
No. Hmm, I thought it wasn't necessary because kqueue descriptors are
not inherited and backends don't execve() directly without forking,
but I guess it can't hurt to add a fcntl() call. Done.
> > + *((WaitEvent **)(&k_ev->udata)) = event;
>
> I'm mildly inclined to hide that behind a macro, so the other places
> have a reference, via the macro definition, to this too.
Done.
> > + if (rc < 0 && event->events == WL_POSTMASTER_DEATH && errno == ESRCH)
> > + {
> > + /*
> > + * The postmaster is already dead. Defer reporting this to the caller
> > + * until wait time, for compatibility with the other implementations.
> > + * To do that we will now add the regular alive pipe.
> > + */
> > + WaitEventAdjustKqueueAdd(&k_ev[0], EVFILT_READ, EV_ADD, event);
> > + rc = kevent(set->kqueue_fd, &k_ev[0], count, NULL, 0, NULL);
> > + }
>
> That's, ... not particulary pretty. Kinda wonder if we shouldn't instead
> just add a 'pending_events' field, that we can check at wait time.
Done.
> > +/* Define to 1 if you have the `kqueue' function. */
> > +#undef HAVE_KQUEUE
> > +
> Should adjust pg_config.win32.h too.
Done.
--
Thomas Munro
http://www.enterprisedb.com