On Fri Jan 5, 2024 at 12:24 PM CST, Robert Haas wrote:
> On Tue, Dec 5, 2023 at 1:35 PM Tristan Partin <tristan@neon.tech> wrote:
> > On Wed Nov 29, 2023 at 11:48 AM CST, Tristan Partin wrote:
> > > I am not completely in love with the code I have written. Lots of
> > > conditional compilation which makes it hard to read. Looking forward to
> > > another round of review to see what y'all think.
> >
> > Ok. Here is a patch which just uses select(2) with a timeout of 1s or
> > pselect(2) if it is available. I also moved the state machine processing
> > into its own function.
>
> Hmm, this adds a function called pqSocketPoll to psql/command.c. But
> there already is such a function in libpq/fe-misc.c. It's not quite
> the same, though. Having the same function in two different modules
> with subtly different definitions seems like it's probably not the
> right idea.
Yep, not tied to the function name. Happy to rename as anyone suggests.
> Also, this seems awfully complicated for something that's supposed to
> (checks notes) wait for a file descriptor to become ready for I/O for
> up to 1 second. It's 160 lines of code in pqSocketPoll and another 50
> in the caller. If this is really the simplest way to do this, we
> really need to rethink our libpq APIs or, uh, something. I wonder if
> we could make this simpler by, say:
>
> - always use select
> - always use a 1-second timeout
> - if it returns faster because the race condition doesn't happen, cool
> - if it doesn't, well then you get to wait for a second, oh well
>
> I don't feel strongly that that's the right way to go and if Heikki or
> some other committer wants to go with this more complex conditional
> approach, that's fine. But to me it seems like a lot.
I think the way to go is to expose some variation of libpq's
pqSocketPoll(), which I would be happy to put together a patch for.
Making frontends, psql in this case, have to reimplement the polling
logic doesn't strike me as fruitful, which is essentially what I have
done.
Thanks for your input!
But also wait a second. In my last email, I said:
> Ok. Here is a patch which just uses select(2) with a timeout of 1s or
> pselect(2) if it is available. I also moved the state machine processing
> into its own function.
This is definitely not the patch I meant to send. What the? Here is what
I meant to send, but I stand by my comment that we should just expose
a variation of pqSocketPoll().
--
Tristan Partin
Neon (https://neon.tech)