Subject: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier

From: Andres Freund <[email-masked]>
Date: Sun, 25 Sep 2022 16:22:37 -0700

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

---

From: Andres Freund <[email-masked]>
Date: Thu, 8 Dec 2022 18:08:15 -0800

Hi,

I just re-discovered this issue, via
https://www.postgresql.org/message-id/20221209003607.bz2zdznvfnkq4zz3%40awork3.anarazel.de

> connect_pg_server() to connect in a blocking manner, without accepting

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
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 */

---

From: Andres Freund <[email-masked]>
Date: Thu, 29 Dec 2022 09:23:30 -0800

Hi,

> throw errors when processing interrupts, and that'd not be ok from deep within

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

---

From: Thomas Munro <[email-masked]>
Date: Fri, 30 Dec 2022 10:31:22 +1300

> 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?

---

From: Tom Lane <[email-masked]>
Date: Thu, 29 Dec 2022 16:37:45 -0500

> 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

---

From: Andres Freund <[email-masked]>
Date: Thu, 29 Dec 2022 13:54:37 -0800

Hi,

On 2022-12-30 10:31:22 +1300, Thomas Munro wrote:
> 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

---

From: Thomas Munro <[email-masked]>
Date: Fri, 30 Dec 2022 14:14:55 +1300

> 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.

> 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.