Re: [HACKERS] kqueue

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