From: Tristan Partin <tristan@neon.tech>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Cc: Shlok Kyal <shlok.kyal.oss@gmail.com>
Subject: Re: psql not responding to SIGINT upon db reconnection
Date: Tue, 30 Jan 2024 16:19:54 -0600
Message-ID: <CYSE2DQI20CH.19LUJSCFH0748@neon.tech> (raw)
In-Reply-To: <CYCWA43RK8AH.11BZB0P45NOD5@neon.tech>
References: <CUAJJDVD8L61.2AH6VEEDMTXX3@gonk>
<CUAKHCX5472I.SJP0AWQRYVFA@gonk>
<CUAKIOVISECO.2S7MSQFOP1304@gonk>
<1588058.1690220636@sss.pgh.pa.us>
<CUAO24Z1Y72E.2069AD01EB4LO@gonk>
<CANhcyEVBQWoPJK+C_h1CKyN3Wr2VKHuPx=bSB4PH2LuGdut4qg@mail.gmail.com>
<CWRWDTTPP0WZ.2W75ZAVL5ND1M@neon.tech>
<5e28b89f-0033-4e42-8184-b71b731c1aa2@iki.fi>
<CX5IOLTR8MAD.1UUDEG0X673Z4@neon.tech>
<9b3693b7-0bc3-430a-b43f-eae44e8a39b6@iki.fi>
<CX5NSE8R07D8.9D8M8LBKP119@neon.tech>
<032868e0-a530-4fc7-972c-af6f08950b7e@iki.fi>
<CXBHH2O58HVA.JUP07AES8JVK@neon.tech>
<CXGM82ZEGMR2.8EX2V7UBFIK3@neon.tech>
<CA+TgmoYK1JCJUyrK==TBj7St1hhke5AOwXyrmDAs-pA_mmAwVw@mail.gmail.com>
<CY93J05RFBUI.3VGP5PLTG4EWI@neon.tech>
<CA+TgmoZb1Ojhw9numxWp8nCioh_wULxOcYCQ6Jwku4t_e43PkA@mail.gmail.com>
<CYCWA43RK8AH.11BZB0P45NOD5@neon.tech>
On Fri Jan 12, 2024 at 11:13 AM CST, Tristan Partin wrote:
> On Fri Jan 12, 2024 at 10:45 AM CST, Robert Haas wrote:
> > On Mon, Jan 8, 2024 at 1:03 AM Tristan Partin <tristan@neon.tech> wrote:
> > > 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.
> >
> > I encourage further exploration of this line of attack. I fear that if
> > I were to commit something like what you've posted up until now,
> > people would complain that that code was too ugly to live, and I'd
> > have a hard time telling them that they're wrong.
>
> Completely agree. Let me look into this. Perhaps I can get something up
> next week or the week after.
Not next week, but here is a respin. I've exposed pqSocketPoll as
PQsocketPoll and am just using that. You can see the diff is so much
smaller, which is great!
In order to fight the race condition, I am just using a 1 second timeout
instead of trying to integrate pselect or ppoll. We could add
a PQsocketPPoll() to support those use cases, but I am not sure how
available pselect and ppoll are. I guess on Windows we don't have
pselect. I don't think using the pipe trick that Heikki mentioned
earlier is suitable to expose via an API in libpq, but someone else
might have a different opinion.
Maybe this is good enough until someone complains? Most people would
probably just chalk any latency between keypress and cancellation as
network latency and not a hardcoded 1 second.
Thanks for your feedback Robert!
--
Tristan Partin
Neon (https://neon.tech)
Attachments:download all (2 files, .tar.gz)v8-0001-Expose-PQsocketPoll-for-frontends.patch (text/x-patch, 3.6K) download
From 4fa6db1900a355a171d3e16019d02f3a415764d0 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Tue, 30 Jan 2024 15:40:57 -0600
Subject: [PATCH v8 1/2] Expose PQsocketPoll for frontends
PQsocketPoll should help with state machine processing when connecting
to a database asynchronously via PQconnectStart().
---
doc/src/sgml/libpq.sgml | 5 ++++-
src/interfaces/libpq/fe-misc.c | 7 +++----
src/interfaces/libpq/libpq-fe.h | 4 ++++
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgmlindex d0d5aefadc..aa26c2cc8d 100644--- a/doc/src/sgml/libpq.sgml+++ b/doc/src/sgml/libpq.sgml@@ -358,7 +358,10 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn);
Loop thus: If <function>PQconnectPoll(conn)</function> last returned
<symbol>PGRES_POLLING_READING</symbol>, wait until the socket is ready to
read (as indicated by <function>select()</function>, <function>poll()</function>, or
- similar system function).+ similar system function). Note that <function>PQsocketPoll</function>+ can help reduce boilerplate by abstracting the setup of+ <function>select()</function>, or <function>poll()</function> if it is+ available on your system.
Then call <function>PQconnectPoll(conn)</function> again.
Conversely, if <function>PQconnectPoll(conn)</function> last returned
<symbol>PGRES_POLLING_WRITING</symbol>, wait until the socket is ready
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.cindex 47a28b0a3a..ee917d375d 100644--- a/src/interfaces/libpq/fe-misc.c+++ b/src/interfaces/libpq/fe-misc.c@@ -55,7 +55,6 @@ static int pqPutMsgBytes(const void *buf, size_t len, PGconn *conn);
static int pqSendSome(PGconn *conn, int len);
static int pqSocketCheck(PGconn *conn, int forRead, int forWrite,
time_t end_time);
-static int pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time);
/*
* PQlibVersion: return the libpq version number
@@ -1059,7 +1058,7 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time)
/* We will retry as long as we get EINTR */
do
- result = pqSocketPoll(conn->sock, forRead, forWrite, end_time);+ result = PQsocketPoll(conn->sock, forRead, forWrite, end_time);
while (result < 0 && SOCK_ERRNO == EINTR);
if (result < 0)
@@ -1083,8 +1082,8 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time)
* Timeout is infinite if end_time is -1. Timeout is immediate (no blocking)
* if end_time is 0 (or indeed, any time before now).
*/
-static int-pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time)+int+PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time)
{
/* We use poll(2) if available, otherwise select(2) */
#ifdef HAVE_POLL
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.hindex defc415fa3..11a7fd32b6 100644--- a/src/interfaces/libpq/libpq-fe.h+++ b/src/interfaces/libpq/libpq-fe.h@@ -21,6 +21,7 @@ extern "C"
#endif
#include <stdio.h>
+#include <time.h>
/*
* postgres_ext.h defines the backend's externally visible types,
@@ -644,6 +645,9 @@ extern int lo_export(PGconn *conn, Oid lobjId, const char *filename);
/* Get the version of the libpq library in use */
extern int PQlibVersion(void);
+/* Poll a file descriptor for reading and/or writing with a timeout */+extern int PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time);+
/* Determine length of multibyte encoded char at *s */
extern int PQmblen(const char *s, int encoding);
--
Tristan Partin
Neon (https://neon.tech)
v8-0002-Allow-SIGINT-to-cancel-psql-database-reconnection.patch (text/x-patch, 2.8K) download
From e4d61216b822852940631ada65b903b65780fb71 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Tue, 30 Jan 2024 15:53:10 -0600
Subject: [PATCH v8 2/2] Allow SIGINT to cancel psql database reconnections
After installing the SIGINT handler in psql, SIGINT can no longer cancel
database reconnections. For instance, if the user starts a reconnection
and then needs to do some form of interaction (ie psql is polling),
there is no way to cancel the reconnection process currently.
Use PQconnectStartParams() in order to insert a cancel_pressed check
into the polling loop.
---
leak.supp | 2 ++
src/bin/psql/command.c | 43 +++++++++++++++++++++++++++++++-
src/interfaces/libpq/exports.txt | 1 +
3 files changed, 45 insertions(+), 1 deletion(-)
create mode 100644 leak.supp
diff --git a/leak.supp b/leak.suppnew file mode 100644index 0000000000..f3fc27a1d8--- /dev/null+++ b/leak.supp@@ -0,0 +1,2 @@+leak:save_ps_display_args+leak:parse_psql_optionsdiff --git a/src/bin/psql/command.c b/src/bin/psql/command.cindex 5c906e4806..1900657324 100644--- a/src/bin/psql/command.c+++ b/src/bin/psql/command.c@@ -3279,6 +3279,46 @@ param_is_newly_set(const char *old_val, const char *new_val)
return false;
}
+static void+process_connection_state_machine(PGconn *conn)+{+ bool for_read = false;++ while (true)+ {+ int rc;+ int sock;++ if (cancel_pressed)+ break;++ sock = PQsocket(conn);+ if (sock == -1)+ break;++ rc = PQsocketPoll(sock, for_read, !for_read, 1);+ if (rc == -1)+ return;++ switch (PQconnectPoll(conn))+ {+ case PGRES_POLLING_OK:+ case PGRES_POLLING_FAILED:+ return;+ case PGRES_POLLING_READING:+ for_read = true;+ continue;+ case PGRES_POLLING_WRITING:+ for_read = false;+ continue;+ case PGRES_POLLING_ACTIVE:+ pg_unreachable();+ }+ }++ pg_unreachable();+}+
/*
* do_connect -- handler for \connect
*
@@ -3595,11 +3635,12 @@ do_connect(enum trivalue reuse_previous_specification,
values[paramnum] = NULL;
/* Note we do not want libpq to re-expand the dbname parameter */
- n_conn = PQconnectdbParams(keywords, values, false);+ n_conn = PQconnectStartParams(keywords, values, false);
pg_free(keywords);
pg_free(values);
+ process_connection_state_machine(n_conn);
if (PQstatus(n_conn) == CONNECTION_OK)
break;
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txtindex 088592deb1..7a7a6ddbab 100644--- a/src/interfaces/libpq/exports.txt+++ b/src/interfaces/libpq/exports.txt@@ -193,3 +193,4 @@ PQsendClosePrepared 190
PQsendClosePortal 191
PQchangePassword 192
PQsendPipelineSync 193
+PQsocketPoll 194--
Tristan Partin
Neon (https://neon.tech)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tristan@neon.tech, robertmhaas@gmail.com, hlinnaka@iki.fi, tgl@sss.pgh.pa.us, shlok.kyal.oss@gmail.com
Subject: Re: psql not responding to SIGINT upon db reconnection
In-Reply-To: <CYSE2DQI20CH.19LUJSCFH0748@neon.tech>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox