From: | "Tristan Partin" <tristan(at)neon(dot)tech> |
---|---|
To: | "Robert Haas" <robertmhaas(at)gmail(dot)com> |
Cc: | "Heikki Linnakangas" <hlinnaka(at)iki(dot)fi>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, <pgsql-hackers(at)postgresql(dot)org>, "Shlok Kyal" <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
Subject: | Re: psql not responding to SIGINT upon db reconnection |
Date: | 2024-01-08 06:03:45 |
Message-ID: | CY93J05RFBUI.3VGP5PLTG4EWI@neon.tech |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)neon(dot)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)
Attachment | Content-Type | Size |
---|---|---|
v7-0001-Allow-SIGINT-to-cancel-psql-database-reconnection.patch | text/x-patch | 6.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2024-01-08 06:09:39 | Re: Synchronizing slots from primary to standby |
Previous Message | jian he | 2024-01-08 05:59:54 | alter table add x wrong error position |