public inbox for [email protected]
help / color / mirror / Atom feedpostgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
7+ messages / 3 participants
[nested] [flat]
* postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-09-25 23:22 Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2022-09-25 23:22 UTC (permalink / raw)
To: pgsql-hackers; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>
Hi,
I amd working on adding an "installcheck" equivalent mode to the meson
build. One invocation of something "installcheck-world" like lead to things
getting stuck.
Lots of log messages like:
2022-09-25 16:16:30.999 PDT [2705454][client backend][28/1112:41269][pg_regress] LOG: still waiting for backend with PID 2705178 to accept ProcSignalBarrier
2022-09-25 16:16:30.999 PDT [2705454][client backend][28/1112:41269][pg_regress] STATEMENT: DROP DATABASE IF EXISTS "regression_test_parser_regress"
2022-09-25 16:16:31.006 PDT [2705472][client backend][22/3699:41294][pg_regress] LOG: still waiting for backend with PID 2705178 to accept ProcSignalBarrier
2022-09-25 16:16:31.006 PDT [2705472][client backend][22/3699:41294][pg_regress] STATEMENT: DROP DATABASE IF EXISTS "regression_test_predtest_regress"
a stacktrace of 2705178 shows:
(gdb) bt
#0 0x00007f67d26fe1b3 in __GI___poll (fds=fds@entry=0x7ffebe187c88, nfds=nfds@entry=1, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
#1 0x00007f67cfd03c1c in pqSocketPoll (sock=<optimized out>, forRead=forRead@entry=1, forWrite=forWrite@entry=0, end_time=end_time@entry=-1)
at ../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:1125
#2 0x00007f67cfd04310 in pqSocketCheck (conn=conn@entry=0x562f875a9b70, forRead=forRead@entry=1, forWrite=forWrite@entry=0, end_time=end_time@entry=-1)
at ../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:1066
#3 0x00007f67cfd043fd in pqWaitTimed (forRead=forRead@entry=1, forWrite=forWrite@entry=0, conn=conn@entry=0x562f875a9b70, finish_time=finish_time@entry=-1)
at ../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-misc.c:998
#4 0x00007f67cfcfc47b in connectDBComplete (conn=conn@entry=0x562f875a9b70) at ../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-connect.c:2166
#5 0x00007f67cfcfe248 in PQconnectdbParams (keywords=keywords@entry=0x562f87613d20, values=values@entry=0x562f87613d70, expand_dbname=expand_dbname@entry=0)
at ../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-connect.c:659
#6 0x00007f67cfd29536 in connect_pg_server (server=server@entry=0x562f876139b0, user=user@entry=0x562f87613980)
at ../../../../home/andres/src/postgresql/contrib/postgres_fdw/connection.c:474
#7 0x00007f67cfd29910 in make_new_connection (entry=entry@entry=0x562f8758b2c8, user=user@entry=0x562f87613980)
at ../../../../home/andres/src/postgresql/contrib/postgres_fdw/connection.c:344
#8 0x00007f67cfd29da0 in GetConnection (user=0x562f87613980, will_prep_stmt=will_prep_stmt@entry=false, state=state@entry=0x562f876136a0)
at ../../../../home/andres/src/postgresql/contrib/postgres_fdw/connection.c:204
#9 0x00007f67cfd35294 in postgresBeginForeignScan (node=0x562f87612e70, eflags=<optimized out>)
and it turns out that backend can't be graciously be terminated.
Maybe I am missing something, but I don't think it's OK for
connect_pg_server() to connect in a blocking manner, without accepting
interrupts?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-12-09 02:08 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2022-12-09 02:08 UTC (permalink / raw)
To: pgsql-hackers; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>
Hi,
I just re-discovered this issue, via
https://www.postgresql.org/message-id/20221209003607.bz2zdznvfnkq4zz3%40awork3.anarazel.de
On 2022-09-25 16:22:37 -0700, Andres Freund wrote:
> Maybe I am missing something, but I don't think it's OK for
> connect_pg_server() to connect in a blocking manner, without accepting
> interrupts?
It's definitely not. This basically means network issues or such can lead to
connections being unkillable...
We know how to do better, c.f. libpqrcv_connect(). I hacked that up for
postgres_fdw, and got through quite a few runs without related issues ([1]).
The same problem is present in two places in dblink.c. Obviously we can copy
and paste the code to dblink.c as well. But that means we have the same not
quite trivial code in three different c files. There's already a fair bit of
duplicated code around AcquireExternalFD().
It seems we should find a place to put backend specific libpq wrapper code
somewhere. Unless we want to relax the rule about not linking libpq into the
backend we can't just put it in the backend directly, though.
The only alternative way to provide a wrapper that I can think of are to
a) introduce a new static library that can be linked to by libpqwalreceiver,
postgres_fdw, dblink
b) add a header with static inline functions implementing interrupt-processing
connection establishment for libpq
Neither really has precedent.
The attached quick patch just adds and uses libpq_connect_interruptible() in
postgres_fdw. If we wanted to move this somewhere generic, at least part of
the external FD handling should also be moved into it.
dblink.c uses a lot of other blocking libpq functions, which obviously also
isn't ok.
Perhaps we could somehow make this easier from within libpq? My first thought
was a connection parameter that'd provide a different implementation of
pqSocketCheck() or such - but I don't think that'd work, because we might
throw errors when processing interrupts, and that'd not be ok from deep within
libpq.
Greetings,
Andres Freund
[1] I eventually encountered a deadlock in REINDEX, but it didn't involve
postgres_fw / ProcSignalBarrier
Attachments:
[text/x-diff] postgres_fdw_libpq_connect.diff (2.3K, ../../[email protected]/2-postgres_fdw_libpq_connect.diff)
download | inline diff:
diff --git i/contrib/postgres_fdw/connection.c w/contrib/postgres_fdw/connection.c
index f0c45b00db8..f9da564a539 100644
--- i/contrib/postgres_fdw/connection.c
+++ w/contrib/postgres_fdw/connection.c
@@ -347,6 +347,67 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
entry->conn, server->servername, user->umid, user->userid);
}
+/*
+ * PQconnectStartParams() wrapper that processes interrupts. Backend code
+ * should *never* enter blocking libpq code as that would prevent
+ * cancellations, global barriers etc from being processed.
+ */
+static PGconn *
+libpq_connect_interruptible(const char *const *keywords,
+ const char *const *values,
+ int expand_dbname,
+ uint32 wait_event_info)
+{
+ PGconn *conn;
+ PostgresPollingStatusType status;
+
+ conn = PQconnectStartParams(keywords, values, false);
+
+ if (!conn)
+ return NULL;
+
+ /*
+ * Poll connection until we have OK or FAILED status.
+ *
+ * Per spec for PQconnectPoll, first wait till socket is write-ready.
+ */
+ status = PGRES_POLLING_WRITING;
+ while (status != PGRES_POLLING_OK && status != PGRES_POLLING_FAILED)
+ {
+ int io_flag;
+ int rc;
+
+ if (status == PGRES_POLLING_READING)
+ io_flag = WL_SOCKET_READABLE;
+#ifdef WIN32
+ /* Windows needs a different test while waiting for connection-made */
+ else if (PQstatus(conn->streamConn) == CONNECTION_STARTED)
+ io_flag = WL_SOCKET_CONNECTED;
+#endif
+ else
+ io_flag = WL_SOCKET_WRITEABLE;
+
+ rc = WaitLatchOrSocket(MyLatch,
+ WL_EXIT_ON_PM_DEATH | WL_LATCH_SET | io_flag,
+ PQsocket(conn),
+ 0,
+ wait_event_info);
+
+ /* Interrupted? */
+ if (rc & WL_LATCH_SET)
+ {
+ ResetLatch(MyLatch);
+ CHECK_FOR_INTERRUPTS();
+ }
+
+ /* If socket is ready, advance the libpq state machine */
+ if (rc & io_flag)
+ status = PQconnectPoll(conn);
+ }
+
+ return conn;
+}
+
/*
* Connect to remote server using specified server and user mapping properties.
*/
@@ -471,7 +532,8 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
}
/* OK to make connection */
- conn = PQconnectdbParams(keywords, values, false);
+ conn = libpq_connect_interruptible(keywords, values,
+ false, PG_WAIT_EXTENSION);
if (!conn)
ReleaseExternalFD(); /* because the PG_CATCH block won't */
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-12-29 17:23 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2022-12-29 17:23 UTC (permalink / raw)
To: pgsql-hackers; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>
Hi,
On 2022-12-08 18:08:15 -0800, Andres Freund wrote:
> I just re-discovered this issue, via
> https://www.postgresql.org/message-id/20221209003607.bz2zdznvfnkq4zz3%40awork3.anarazel.de
>
>
> On 2022-09-25 16:22:37 -0700, Andres Freund wrote:
> > Maybe I am missing something, but I don't think it's OK for
> > connect_pg_server() to connect in a blocking manner, without accepting
> > interrupts?
>
> It's definitely not. This basically means network issues or such can lead to
> connections being unkillable...
>
> We know how to do better, c.f. libpqrcv_connect(). I hacked that up for
> postgres_fdw, and got through quite a few runs without related issues ([1]).
>
> The same problem is present in two places in dblink.c. Obviously we can copy
> and paste the code to dblink.c as well. But that means we have the same not
> quite trivial code in three different c files. There's already a fair bit of
> duplicated code around AcquireExternalFD().
>
> It seems we should find a place to put backend specific libpq wrapper code
> somewhere. Unless we want to relax the rule about not linking libpq into the
> backend we can't just put it in the backend directly, though.
>
> The only alternative way to provide a wrapper that I can think of are to
> a) introduce a new static library that can be linked to by libpqwalreceiver,
> postgres_fdw, dblink
> b) add a header with static inline functions implementing interrupt-processing
> connection establishment for libpq
>
> Neither really has precedent.
>
> The attached quick patch just adds and uses libpq_connect_interruptible() in
> postgres_fdw. If we wanted to move this somewhere generic, at least part of
> the external FD handling should also be moved into it.
>
>
> dblink.c uses a lot of other blocking libpq functions, which obviously also
> isn't ok.
>
>
> Perhaps we could somehow make this easier from within libpq? My first thought
> was a connection parameter that'd provide a different implementation of
> pqSocketCheck() or such - but I don't think that'd work, because we might
> throw errors when processing interrupts, and that'd not be ok from deep within
> libpq.
Any opinions? Due to the simplicity I'm currently leaning to a header-only
helper, but I don't feel confident about it.
The rate of cfbot failures is high enough that it'd be good to do something
about this.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-12-29 21:31 Thomas Munro <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 2 replies; 7+ messages in thread
From: Thomas Munro @ 2022-12-29 21:31 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>
On Fri, Dec 30, 2022 at 6:23 AM Andres Freund <[email protected]> wrote:
> On 2022-12-08 18:08:15 -0800, Andres Freund wrote:
> > On 2022-09-25 16:22:37 -0700, Andres Freund wrote:
> > The only alternative way to provide a wrapper that I can think of are to
> > a) introduce a new static library that can be linked to by libpqwalreceiver,
> > postgres_fdw, dblink
> > b) add a header with static inline functions implementing interrupt-processing
> > connection establishment for libpq
> >
> > Neither really has precedent.
> Any opinions? Due to the simplicity I'm currently leaning to a header-only
> helper, but I don't feel confident about it.
The header idea is a little bit sneaky (IIUC: a header that is part of
the core tree, but can't be used by core and possibly needs special
treatment in 'headercheck' to get the right include search path, can
only be used by libpqwalreceiver et al which are allowed to link to
libpq), but I think it is compatible with other goals we have
discussed in other threads. I think in the near future we'll probably
remove the concept of non-threaded server builds (as proposed before
in the post HP-UX 10 cleanup thread, with patches, but not quite over
the line yet). Then I think the server could be allowed to link libpq
directly? And at that point this code wouldn't be sneaky anymore and
could optionally move into a .c. Does that makes sense?
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-12-29 21:37 Tom Lane <[email protected]>
parent: Thomas Munro <[email protected]>
1 sibling, 0 replies; 7+ messages in thread
From: Tom Lane @ 2022-12-29 21:37 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>
Thomas Munro <[email protected]> writes:
> The header idea is a little bit sneaky (IIUC: a header that is part of
> the core tree, but can't be used by core and possibly needs special
> treatment in 'headercheck' to get the right include search path, can
> only be used by libpqwalreceiver et al which are allowed to link to
> libpq), but I think it is compatible with other goals we have
> discussed in other threads. I think in the near future we'll probably
> remove the concept of non-threaded server builds (as proposed before
> in the post HP-UX 10 cleanup thread, with patches, but not quite over
> the line yet). Then I think the server could be allowed to link libpq
> directly? And at that point this code wouldn't be sneaky anymore and
> could optionally move into a .c. Does that makes sense?
I don't like the idea of linking libpq directly into the backend.
It should remain a dynamically-loaded library to avoid problems
during software updates.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-12-29 21:54 Andres Freund <[email protected]>
parent: Thomas Munro <[email protected]>
1 sibling, 1 reply; 7+ messages in thread
From: Andres Freund @ 2022-12-29 21:54 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>
Hi,
On 2022-12-30 10:31:22 +1300, Thomas Munro wrote:
> On Fri, Dec 30, 2022 at 6:23 AM Andres Freund <[email protected]> wrote:
> > On 2022-12-08 18:08:15 -0800, Andres Freund wrote:
> > > On 2022-09-25 16:22:37 -0700, Andres Freund wrote:
> > > The only alternative way to provide a wrapper that I can think of are to
> > > a) introduce a new static library that can be linked to by libpqwalreceiver,
> > > postgres_fdw, dblink
> > > b) add a header with static inline functions implementing interrupt-processing
> > > connection establishment for libpq
> > >
> > > Neither really has precedent.
>
> > Any opinions? Due to the simplicity I'm currently leaning to a header-only
> > helper, but I don't feel confident about it.
>
> The header idea is a little bit sneaky (IIUC: a header that is part of
> the core tree, but can't be used by core and possibly needs special
> treatment in 'headercheck' to get the right include search path, can
> only be used by libpqwalreceiver et al which are allowed to link to
> libpq), but I think it is compatible with other goals we have
> discussed in other threads.
Hm, what special search path / headerscheck magic are you thinking of? I think
something like src/include/libpq/libpq-be-fe-helpers.h defining a bunch of
static inlines should "just" work?
We likely could guard against that header being included from code ending up
in the postgres binary directly by #error'ing if BUILDING_DLL is
defined. That's a very badly named define, but it IIRC has to be iff building
code ending up in postgres directly.
> I think in the near future we'll probably remove the concept of non-threaded
> server builds (as proposed before in the post HP-UX 10 cleanup thread, with
> patches, but not quite over the line yet). Then I think the server could be
> allowed to link libpq directly? And at that point this code wouldn't be
> sneaky anymore and could optionally move into a .c. Does that makes sense?
I was wondering about linking in libpq directly as well. But I am not sure
it's a good idea. I suspect we'd run into some issues around libraries
(including extensions) linking to different versions of libpq etc - if we
directly link to libpq that'd end up in tears.
It might be a different story if we had a version of libpq built with
different symbol names etc. But that's not exactly trivial either.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2022-12-30 01:14 Thomas Munro <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Thomas Munro @ 2022-12-30 01:14 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>
On Fri, Dec 30, 2022 at 10:54 AM Andres Freund <[email protected]> wrote:
> On 2022-12-30 10:31:22 +1300, Thomas Munro wrote:
> > On Fri, Dec 30, 2022 at 6:23 AM Andres Freund <[email protected]> wrote:
> > > On 2022-12-08 18:08:15 -0800, Andres Freund wrote:
> > > > On 2022-09-25 16:22:37 -0700, Andres Freund wrote:
> > > > The only alternative way to provide a wrapper that I can think of are to
> > > > a) introduce a new static library that can be linked to by libpqwalreceiver,
> > > > postgres_fdw, dblink
> > > > b) add a header with static inline functions implementing interrupt-processing
> > > > connection establishment for libpq
> > > >
> > > > Neither really has precedent.
> >
> > > Any opinions? Due to the simplicity I'm currently leaning to a header-only
> > > helper, but I don't feel confident about it.
> >
> > The header idea is a little bit sneaky (IIUC: a header that is part of
> > the core tree, but can't be used by core and possibly needs special
> > treatment in 'headercheck' to get the right include search path, can
> > only be used by libpqwalreceiver et al which are allowed to link to
> > libpq), but I think it is compatible with other goals we have
> > discussed in other threads.
>
> Hm, what special search path / headerscheck magic are you thinking of? I think
> something like src/include/libpq/libpq-be-fe-helpers.h defining a bunch of
> static inlines should "just" work?
Oh, I was imagining something slightly different. Not something under
src/include/libpq, but conceptually a separate header-only library
that is above both the backend and libpq. Maybe something like
src/include/febe_util/libpq_connect_interruptible.h. In other words,
I thought your idea b was a header-only version of your idea a. I
think that might be a bit nicer than putting it under libpq?
Superficial difference, perhaps...
And then I assumed that headerscheck would need to be told to add
libpq's header location in -I for that header, but on closer
inspection it already adds that unconditionally so I retract that
comment.
> > I think in the near future we'll probably remove the concept of non-threaded
> > server builds (as proposed before in the post HP-UX 10 cleanup thread, with
> > patches, but not quite over the line yet). Then I think the server could be
> > allowed to link libpq directly? And at that point this code wouldn't be
> > sneaky anymore and could optionally move into a .c. Does that makes sense?
>
> I was wondering about linking in libpq directly as well. But I am not sure
> it's a good idea. I suspect we'd run into some issues around libraries
> (including extensions) linking to different versions of libpq etc - if we
> directly link to libpq that'd end up in tears.
>
> It might be a different story if we had a version of libpq built with
> different symbol names etc. But that's not exactly trivial either.
Hmm, yeah. Some interesting things to think about. Whether it's a
feature or an accident that new backends can pick up new libpq minor
updates without restarting the postmaster, and how we'd manage a
future libpq major version/ABI break. Getting a bit off topic for
this thread I suppose.
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2022-12-30 01:14 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-09-25 23:22 postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2022-12-09 02:08 ` Andres Freund <[email protected]>
2022-12-29 17:23 ` Andres Freund <[email protected]>
2022-12-29 21:31 ` Thomas Munro <[email protected]>
2022-12-29 21:37 ` Tom Lane <[email protected]>
2022-12-29 21:54 ` Andres Freund <[email protected]>
2022-12-30 01:14 ` Thomas Munro <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox