Re: pg_basebackup WAL streamer shutdown is bogus - leading to slow tests

Hi,

On 2022-01-17 14:50:27 +0100, Magnus Hagander wrote:
> On Mon, Jan 17, 2022 at 12:31 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > Hi,
> >
> > On 2022-01-16 15:28:00 -0800, Andres Freund wrote:
> > > I hacked that up last night. And a fix or two later, it seems to be
> > > working. What I'd missed at first is that the event needs to be reset in
> > > reached_end_position(), otherwise we'll busy loop.
>
> You can create the event with bManualReset set to False to avoid that,
> no? With this usecase, I don't really see a reason not to do that
> instead?

The problem I'm referring to is that some types of events are edge
triggered. Which we've been painfully reminded of recently:
https://www.postgresql.org/message-id/CA%2BhUKG%2BOeoETZQ%3DQw5Ub5h3tmwQhBmDA%3DnuNO3KG%3DzWfUypFAw%40mail.gmail.com

It appears there's no guarantee that you'll see e.g. FD_CLOSE if you use
short-lived events (the FD_CLOSE is recorded internally but not signalled
immediately if there's still readable data, and the internal record is reset
by WSAEventSelect()).

> > > I wonder if using a short-lived event handle would have dangers of missing
> > > FD_CLOSE here as well? It'd probably be worth avoiding the risk by creating
> > > the event just once.
> > >
> > > I just wasn't immediately sure where to stash it. Probably just by adding a
> > > field in StreamCtl, that ReceiveXlogStream() then sets? So far it's constant
> > > once passed to ReceiveXlogStream(), but I don't really see a reason why it'd
> > > need to stay that way?
> >
> > Oops, attached the patch this time.
>
> Do we really want to create a new event every time? Those are kernel
> objects, so they're not entirely free, but that part maybe doesn't
> matter. Wouldn't it be cleaner to do it like we do in
> pgwin32_waitforsinglesocket() which is create it once and store it in
> a static variable? Or is that what you're suggesting above in the "I
> wonder if" part?

Yes, that's what I was suggesting. I wasn't thinking of using a static var,
but putting it in StreamCtl. Note that what pgwin32_waitforsinglesocket()
is doing doesn't protect against the problem referenced above, because it
still is reset by WSAEventSelect.

Greetings,

Andres Freund