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

On Sat, Jan 29, 2022 at 10:47 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2022-01-29 12:44:22 -0800, Andres Freund wrote:
> > On 2022-01-17 10:06:56 -0800, Andres Freund wrote:
> > > 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.
> >
> > Do we are about breaking StreamCtl ABI? I don't think so?
>
> Here's a version of the patch only creating the event once. Needs a small bit
> of comment polishing, but otherwise I think it's sane?

LGTM in general, yes.

I'm wondering about the part that does:
+ events[0] = stream->net_event;
+ nevents++;
+
+ if (stream->stop_event != NULL)
+ {
+ events[1] = stream->stop_event;
+ nevents++;
+ }
+

Using a combination of nevents but hardcoded indexes does work -- but
only as long as there is only one optional entry. Should they perhaps
be written
+ events[nevents++] = stream->net_event;

instead, for future proofing? But then you'd also have to change the
if() statement on the return side I guess.

Can of course also be changed at such a point where a third event
might be added. Not important, but it poked me in the eye when I was
reading it.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/