On Tue, Dec 14, 2021 at 7:53 AM Andres Freund <andres@anarazel.de> wrote:
> On 2021-12-13 17:51:00 +1300, Thomas Munro wrote:
> > I tried that. It seems OK, and gets rid of the PG_FINALLY(), which is
> > nice. Latches still have higher priority, and still have the fast
> > return if already set and you asked for only one event, but now if you
> > ask for nevents > 1 we'll make the syscall too so we'll see the
> > WL_SOCKET_CLOSED.
>
> Isn't a certain postgres committer that cares a lot about unnecessary syscalls
> going to be upset about this one? Even with the nevents > 1 optimization? Yes,
> right now there's no other paths that do so, but I don't like the corner this
> paints us in.
Well, I was trying to avoid bikeshedding an API change just for a
hypothetical problem we could solve when the time comes (say, after
fixing the more egregious problems with Append's WES usage), but here
goes: we could do something like AddWaitEventToSet(FeBeWaitSet,
WL_LATCH_SET_LOPRIO, ...) that is translated to WL_LATCH_SET
internally but also sets a flag to enable this
no-really-please-poll-all-the-things-if-there-is-space behaviour.
> From a different angle: Why do we need to perform the client connection check
> if the latch is set?
Imagine a parallel message that arrives just as our connection check
CFI routine runs, and sets the latch. It'd be arbitrary and bizarre
if that caused us to skip the check. So we have to ignore it, and the
question is just how. I presented two different ways. A third way
would be to create an entirely new WES for this use case; nope, that's
either wasteful of an fd or wasteful of system calls for a temporary
WES and likely more PG_TRY() stuff to avoid leaking it. A fourth
could be to modify the WES like the simple code in v2/v3, but make the
WES code no-throw or pretend it's no-throw, which I didn't seriously
consider (I mean, if, say, WaitForMultipleObjects() returns
WAIT_FAILED and ERRORs then your session is pretty much hosed and your
WES is probably never going to work correctly again, but it still
seems to break basic rules of programming decency and exception safety
to leave the WES sans latch on non-local exit, which is why I added
the PG_FINALLY() that offended you to v3).