public inbox for [email protected]
help / color / mirror / Atom feedRe: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
6+ messages / 3 participants
[nested] [flat]
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
@ 2023-01-03 20:05 Andres Freund <[email protected]>
2023-01-10 02:05 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2023-01-03 20:05 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Bharath Rupireddy <[email protected]>
Hi,
On 2022-12-30 14:14:55 +1300, Thomas Munro wrote:
> 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...
It doesn't seem entirely right to introduce a top-level "module" for
libpq-in-extensions to me - we don't do that for other APIs used for
extensions. But a header only library also doesn't quite seem right. So ...
Looking at turning my patch upthread into something slightly less prototype-y,
I noticed that libpqwalreceiver doesn't do AcquireExternalFD(), added to other
backend uses of libpq in 3d475515a15. It's unlikely to matter for
walreceiver.c itelf, but it seems problematic for the logical replication
cases?
It's annoying to introduce PG_TRY/CATCH blocks, just to deal with the
potential for errors inside WaitLatchOrSocket(), which should never happen. I
wonder if we should consider making latch.c error out fatally, instead of
elog(ERROR). If latches are broken, things are bad.
The PG_CATCH() logic in postgres_fdw's GetConnection() looks quite suspicious
to me. It looks like 32a9c0bdf493 took entirely the wrong path. Instead of
optionally not throwing or directly re-establishing connections in
begin_remote_xact(), the PG_CATCH() hammer was applied.
The attached patch adds libpq-be-fe-helpers.h and uses it in libpqwalreceiver,
dblink, postgres_fdw.
As I made libpq-be-fe-helpers.h handle reserving external fds,
libpqwalreceiver now does so. I briefly looked through its users without
seeing cases of leaking in case of errors - which would already have been bad,
since we'd already have leaked a libpq connection/socket.
Given the lack of field complaints and the size of the required changes, I
don't think we should backpatch this, even though it's pretty clearly buggy
as-is.
Some time back Thomas had a patch to introduce a wrapper around
libpq-in-extensions that fixed issues cause by some events being
edge-triggered on windows. It's possible that combining these two efforts
would yield something better. I resisted the urge to create a wrapper around
each connection in this patch, as it'd have ended up being a whole lot more
invasive. But... Thomas, do you have a reference to that code handy?
Greetings,
Andres Freund
Attachments:
[text/x-diff] v2-0001-wip-don-t-block-inside-PQconnectdb-et-al.patch (16.8K, ../../[email protected]/2-v2-0001-wip-don-t-block-inside-PQconnectdb-et-al.patch)
download | inline diff:
From 34e5ea311322b3bc3bb0f8c925f9ade1a59c6f09 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Tue, 3 Jan 2023 11:31:04 -0800
Subject: [PATCH v2] wip: don't block inside PQconnectdb et al
---
src/include/libpq/libpq-be-fe-helpers.h | 233 ++++++++++++++++++
.../libpqwalreceiver/libpqwalreceiver.c | 53 +---
contrib/dblink/dblink.c | 80 +-----
contrib/postgres_fdw/connection.c | 42 +---
4 files changed, 256 insertions(+), 152 deletions(-)
create mode 100644 src/include/libpq/libpq-be-fe-helpers.h
diff --git a/src/include/libpq/libpq-be-fe-helpers.h b/src/include/libpq/libpq-be-fe-helpers.h
new file mode 100644
index 00000000000..4f3d3b821f0
--- /dev/null
+++ b/src/include/libpq/libpq-be-fe-helpers.h
@@ -0,0 +1,233 @@
+/*-------------------------------------------------------------------------
+ *
+ * libpq-be-fe-helpers.h
+ * Helper functions for using libpq in extensions
+ *
+ * Code built directly into the backend is not allowed to link to libpq
+ * directly. Extension code is allowed to use libpq however. However, libpq
+ * used in extensions has to be careful to block inside libpq, otherwise
+ * interrupts will not be processed, leading to issues like unresolvable
+ * deadlocks. Backend code also needs to take care to acquire/release an
+ * external fd for the connection, otherwise fd.c's accounting of fd's is
+ * broken.
+ *
+ * This file provides helper functions to make it easier to comply with these
+ * rules. It is a header only library as it needs to be linked into each
+ * extension using libpq, and it seems too small to be worth adding a
+ * dedicated static library for.
+ *
+ * TODO: For historical reasons the connections established here are not put
+ * into non-blocking mode. That can lead to blocking even when only the async
+ * libpq functions are used. This should be fixed.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/libpq/libpq-be-fe-helpers.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef LIBPQ_BE_FE_HELPERS_H
+#define LIBPQ_BE_FE_HELPERS_H
+
+/*
+ * Despite the name, BUILDING_DLL is set only when building code directly part
+ * of the backend. Which also is where libpq isn't allowed to be
+ * used. Obviously this doesn't protect against libpq-fe.h getting included
+ * otherwise, but perhaps still protects against a few mistakes...
+ */
+#ifdef BUILDING_DLL
+#error "libpq may not be used code directly built into the backend"
+#endif
+
+#include "libpq-fe.h"
+#include "miscadmin.h"
+#include "storage/fd.h"
+#include "storage/latch.h"
+
+
+static inline void libpqsrv_connect_prepare(void);
+static inline void libpqsrv_connect_internal(PGconn *conn, uint32 wait_event_info);
+
+/*
+ * PQfinish() wrapper that additionally releases the reserved file descriptor.
+ *
+ * It is allowed to call this with a NULL pgconn iff returned by
+ * libpqsrv_connect*.
+ */
+static inline void
+libpqsrv_disconnect(PGconn *conn)
+{
+ /*
+ * If no connection was established, we haven't reserved an FD for it (or
+ * already released it). This rule makes it easier to write PG_CATCH()
+ * handlers for this facility's users.
+ *
+ * See also libpqsrv_connect_internal().
+ */
+ if (conn == NULL)
+ return;
+
+ ReleaseExternalFD();
+ PQfinish(conn);
+}
+
+/*
+ * PQconnectdb() wrapper that reserves a file descriptor and processes
+ * interrupts.
+ */
+static inline PGconn *
+libpqsrv_connect(const char *conninfo, uint32 wait_event_info)
+{
+ PGconn *conn = NULL;
+
+ libpqsrv_connect_prepare();
+
+ conn = PQconnectStart(conninfo);
+
+ libpqsrv_connect_internal(conn, wait_event_info);
+
+ return conn;
+}
+
+/*
+ * PQconnectdbParams() wrapper that reserves a file descriptor and processes
+ * interrupts.
+ */
+static inline PGconn *
+libpqsrv_connect_params(const char *const *keywords,
+ const char *const *values,
+ int expand_dbname,
+ uint32 wait_event_info)
+{
+ PGconn *conn = NULL;
+
+ libpqsrv_connect_prepare();
+
+ conn = PQconnectStartParams(keywords, values, expand_dbname);
+
+ libpqsrv_connect_internal(conn, wait_event_info);
+
+ return conn;
+}
+
+/*
+ * Helper function for all connection establishment functions.
+ */
+static inline void
+libpqsrv_connect_prepare(void)
+{
+ /*
+ * We must obey fd.c's limit on non-virtual file descriptors. Assume that
+ * a PGconn represents one long-lived FD. (Doing this here also ensures
+ * that VFDs are closed if needed to make room.)
+ */
+ if (!AcquireExternalFD())
+ {
+#ifndef WIN32 /* can't write #if within ereport() macro */
+ ereport(ERROR,
+ (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not establish connection"),
+ errdetail("There are too many open files on the local server."),
+ errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
+#else
+ ereport(ERROR,
+ (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not establish connection"),
+ errdetail("There are too many open files on the local server."),
+ errhint("Raise the server's max_files_per_process setting.")));
+#endif
+ }
+
+}
+
+/*
+ * Helper function for all connection establishment functions.
+ */
+static inline void
+libpqsrv_connect_internal(PGconn *conn, uint32 wait_event_info)
+{
+ /*
+ * With conn == NULL libpqsrv_disconnect() wouldn't release the FD. So do
+ * that here.
+ */
+ if (conn == NULL)
+ {
+ ReleaseExternalFD();
+ return;
+ }
+
+ /*
+ * Can't wait without a socket. Note that we don't want to close the libpq
+ * connection yet, so callers can emit a useful error.
+ */
+ if (PQstatus(conn) == CONNECTION_BAD)
+ return;
+
+ /*
+ * WaitLatchOrSocket() can conceivably fail, handle this case here instead
+ * of requiring all callers to do so.
+ */
+ PG_TRY();
+ {
+ PostgresPollingStatusType status;
+
+ /*
+ * 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) == 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);
+ }
+ }
+ PG_CATCH();
+ {
+ /*
+ * If an error is thrown here, the callers won't call
+ * libpqsrv_disconnect() with a conn, so release resources
+ * immediately.
+ */
+ ReleaseExternalFD();
+ PQfinish(conn);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+}
+
+#endif /* LIBPQ_BE_FE_HELPERS_H */
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 219cd73b7fc..bab2fe29562 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -24,6 +24,7 @@
#include "common/connect.h"
#include "funcapi.h"
#include "libpq-fe.h"
+#include "libpq/libpq-be-fe-helpers.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -125,7 +126,6 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
char **err)
{
WalReceiverConn *conn;
- PostgresPollingStatusType status;
const char *keys[6];
const char *vals[6];
int i = 0;
@@ -172,52 +172,9 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
Assert(i < sizeof(keys));
conn = palloc0(sizeof(WalReceiverConn));
- conn->streamConn = PQconnectStartParams(keys, vals,
- /* expand_dbname = */ true);
- if (PQstatus(conn->streamConn) == CONNECTION_BAD)
- {
- *err = pchomp(PQerrorMessage(conn->streamConn));
- 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;
- do
- {
- 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->streamConn),
- 0,
- WAIT_EVENT_LIBPQWALRECEIVER_CONNECT);
-
- /* Interrupted? */
- if (rc & WL_LATCH_SET)
- {
- ResetLatch(MyLatch);
- ProcessWalRcvInterrupts();
- }
-
- /* If socket is ready, advance the libpq state machine */
- if (rc & io_flag)
- status = PQconnectPoll(conn->streamConn);
- } while (status != PGRES_POLLING_OK && status != PGRES_POLLING_FAILED);
+ conn->streamConn = libpqsrv_connect_params(keys, vals,
+ /* expand_dbname = */ true,
+ WAIT_EVENT_LIBPQWALRECEIVER_CONNECT);
if (PQstatus(conn->streamConn) != CONNECTION_OK)
{
@@ -740,7 +697,7 @@ libpqrcv_PQgetResult(PGconn *streamConn)
static void
libpqrcv_disconnect(WalReceiverConn *conn)
{
- PQfinish(conn->streamConn);
+ libpqsrv_disconnect(conn->streamConn);
PQfreemem(conn->recvBuf);
pfree(conn);
}
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 8dd122042b4..130320db571 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -48,6 +48,7 @@
#include "funcapi.h"
#include "lib/stringinfo.h"
#include "libpq-fe.h"
+#include "libpq/libpq-be-fe-helpers.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "parser/scansup.h"
@@ -59,6 +60,7 @@
#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/varlena.h"
+#include "utils/wait_event.h"
PG_MODULE_MAGIC;
@@ -199,37 +201,14 @@ dblink_get_conn(char *conname_or_str,
connstr = conname_or_str;
dblink_connstr_check(connstr);
- /*
- * We must obey fd.c's limit on non-virtual file descriptors. Assume
- * that a PGconn represents one long-lived FD. (Doing this here also
- * ensures that VFDs are closed if needed to make room.)
- */
- if (!AcquireExternalFD())
- {
-#ifndef WIN32 /* can't write #if within ereport() macro */
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
-#else
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process setting.")));
-#endif
- }
-
/* OK to make connection */
- conn = PQconnectdb(connstr);
+ conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
char *msg = pchomp(PQerrorMessage(conn));
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
ereport(ERROR,
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
errmsg("could not establish connection"),
@@ -312,36 +291,13 @@ dblink_connect(PG_FUNCTION_ARGS)
/* check password in connection string if not superuser */
dblink_connstr_check(connstr);
- /*
- * We must obey fd.c's limit on non-virtual file descriptors. Assume that
- * a PGconn represents one long-lived FD. (Doing this here also ensures
- * that VFDs are closed if needed to make room.)
- */
- if (!AcquireExternalFD())
- {
-#ifndef WIN32 /* can't write #if within ereport() macro */
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
-#else
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process setting.")));
-#endif
- }
-
/* OK to make connection */
- conn = PQconnectdb(connstr);
+ conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
msg = pchomp(PQerrorMessage(conn));
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
if (rconn)
pfree(rconn);
@@ -366,10 +322,7 @@ dblink_connect(PG_FUNCTION_ARGS)
else
{
if (pconn->conn)
- {
- PQfinish(pconn->conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
pconn->conn = conn;
}
@@ -402,8 +355,7 @@ dblink_disconnect(PG_FUNCTION_ARGS)
if (!conn)
dblink_conn_not_avail(conname);
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
if (rconn)
{
deleteConnection(conname);
@@ -838,10 +790,7 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
{
/* if needed, close the connection to the database */
if (freeconn)
- {
- PQfinish(conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
}
PG_END_TRY();
@@ -1516,10 +1465,7 @@ dblink_exec(PG_FUNCTION_ARGS)
{
/* if needed, close the connection to the database */
if (freeconn)
- {
- PQfinish(conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
}
PG_END_TRY();
@@ -2606,8 +2552,7 @@ createNewConnection(const char *name, remoteConn *rconn)
if (found)
{
- PQfinish(rconn->conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(rconn->conn);
pfree(rconn);
ereport(ERROR,
@@ -2647,8 +2592,7 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
{
if (!PQconnectionUsedPassword(conn))
{
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
if (rconn)
pfree(rconn);
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index ed75ce3f79c..7760380f00d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -17,6 +17,7 @@
#include "catalog/pg_user_mapping.h"
#include "commands/defrem.h"
#include "funcapi.h"
+#include "libpq/libpq-be-fe-helpers.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -446,35 +447,10 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* verify the set of connection parameters */
check_conn_params(keywords, values, user);
- /*
- * We must obey fd.c's limit on non-virtual file descriptors. Assume
- * that a PGconn represents one long-lived FD. (Doing this here also
- * ensures that VFDs are closed if needed to make room.)
- */
- if (!AcquireExternalFD())
- {
-#ifndef WIN32 /* can't write #if within ereport() macro */
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not connect to server \"%s\"",
- server->servername),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
-#else
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not connect to server \"%s\"",
- server->servername),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process setting.")));
-#endif
- }
-
/* OK to make connection */
- conn = PQconnectdbParams(keywords, values, false);
-
- if (!conn)
- ReleaseExternalFD(); /* because the PG_CATCH block won't */
+ conn = libpqsrv_connect_params(keywords, values,
+ /* expand_dbname = */ false,
+ PG_WAIT_EXTENSION);
if (!conn || PQstatus(conn) != CONNECTION_OK)
ereport(ERROR,
@@ -507,12 +483,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
}
PG_CATCH();
{
- /* Release PGconn data structure if we managed to create one */
- if (conn)
- {
- PQfinish(conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
PG_RE_THROW();
}
PG_END_TRY();
@@ -528,9 +499,8 @@ disconnect_pg_server(ConnCacheEntry *entry)
{
if (entry->conn != NULL)
{
- PQfinish(entry->conn);
+ libpqsrv_disconnect(entry->conn);
entry->conn = NULL;
- ReleaseExternalFD();
}
}
--
2.38.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
2023-01-03 20:05 Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
@ 2023-01-10 02:05 ` Andres Freund <[email protected]>
2023-01-14 01:45 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Thomas Munro <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2023-01-10 02:05 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Bharath Rupireddy <[email protected]>
Hi,
On 2023-01-03 12:05:20 -0800, Andres Freund wrote:
> The attached patch adds libpq-be-fe-helpers.h and uses it in libpqwalreceiver,
> dblink, postgres_fdw.
> As I made libpq-be-fe-helpers.h handle reserving external fds,
> libpqwalreceiver now does so. I briefly looked through its users without
> seeing cases of leaking in case of errors - which would already have been bad,
> since we'd already have leaked a libpq connection/socket.
>
>
> Given the lack of field complaints and the size of the required changes, I
> don't think we should backpatch this, even though it's pretty clearly buggy
> as-is.
Any comments on this? Otherwise I think I'll go with this approach.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
2023-01-03 20:05 Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2023-01-10 02:05 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
@ 2023-01-14 01:45 ` Thomas Munro <[email protected]>
2023-01-21 03:00 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Munro @ 2023-01-14 01:45 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Bharath Rupireddy <[email protected]>
On Tue, Jan 10, 2023 at 3:05 PM Andres Freund <[email protected]> wrote:
> On 2023-01-03 12:05:20 -0800, Andres Freund wrote:
> > The attached patch adds libpq-be-fe-helpers.h and uses it in libpqwalreceiver,
> > dblink, postgres_fdw.
>
> > As I made libpq-be-fe-helpers.h handle reserving external fds,
> > libpqwalreceiver now does so. I briefly looked through its users without
> > seeing cases of leaking in case of errors - which would already have been bad,
> > since we'd already have leaked a libpq connection/socket.
> >
> >
> > Given the lack of field complaints and the size of the required changes, I
> > don't think we should backpatch this, even though it's pretty clearly buggy
> > as-is.
>
> Any comments on this? Otherwise I think I'll go with this approach.
+1. Not totally convinced about the location but we are free to
re-organise it any time, and the random CI failures are bad.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
2023-01-03 20:05 Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2023-01-10 02:05 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2023-01-14 01:45 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Thomas Munro <[email protected]>
@ 2023-01-21 03:00 ` Andres Freund <[email protected]>
2023-01-24 03:28 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Andres Freund @ 2023-01-21 03:00 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Bharath Rupireddy <[email protected]>
Hi,
On 2023-01-14 14:45:06 +1300, Thomas Munro wrote:
> On Tue, Jan 10, 2023 at 3:05 PM Andres Freund <[email protected]> wrote:
> > On 2023-01-03 12:05:20 -0800, Andres Freund wrote:
> > > The attached patch adds libpq-be-fe-helpers.h and uses it in libpqwalreceiver,
> > > dblink, postgres_fdw.
> >
> > > As I made libpq-be-fe-helpers.h handle reserving external fds,
> > > libpqwalreceiver now does so. I briefly looked through its users without
> > > seeing cases of leaking in case of errors - which would already have been bad,
> > > since we'd already have leaked a libpq connection/socket.
> > >
> > >
> > > Given the lack of field complaints and the size of the required changes, I
> > > don't think we should backpatch this, even though it's pretty clearly buggy
> > > as-is.
> >
> > Any comments on this? Otherwise I think I'll go with this approach.
>
> +1. Not totally convinced about the location but we are free to
> re-organise it any time, and the random CI failures are bad.
Cool.
Updated patch attached. I split it into multiple pieces.
1) A fix for [1], included here because I encountered it while testing
2) Introduction of libpq-be-fe-helpers.h
3) Convert dblink and postgres_fdw to the helper
4) Convert libpqwalreceiver.c to the helper
Even if we eventually decide to backpatch 3), we'd likely not backpatch 4), as
there's no bug (although perhaps the lack of FD handling could be called a
bug?).
There's also some light polishing, improving commit message, comments and
moving some internal helper functions to later in the file.
Greetings,
Andres Freund
[1] https://postgr.es/m/20230121011237.q52apbvlarfv6jm6%40awork3.anarazel.de
Attachments:
[text/x-diff] v3-0001-Fix-error-handling-in-libpqrcv_connect.patch (2.3K, ../../[email protected]/2-v3-0001-Fix-error-handling-in-libpqrcv_connect.patch)
download | inline diff:
From da9345151423180732d2928fe7fe6ff0b6a11d4d Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 20 Jan 2023 18:27:05 -0800
Subject: [PATCH v3 1/4] Fix error handling in libpqrcv_connect()
Author:
Reviewed-by:
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
.../libpqwalreceiver/libpqwalreceiver.c | 26 +++++++++++--------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index c40c6220db8..fefc8660259 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -175,10 +175,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
conn->streamConn = PQconnectStartParams(keys, vals,
/* expand_dbname = */ true);
if (PQstatus(conn->streamConn) == CONNECTION_BAD)
- {
- *err = pchomp(PQerrorMessage(conn->streamConn));
- return NULL;
- }
+ goto bad_connection_errmsg;
/*
* Poll connection until we have OK or FAILED status.
@@ -220,10 +217,7 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
} while (status != PGRES_POLLING_OK && status != PGRES_POLLING_FAILED);
if (PQstatus(conn->streamConn) != CONNECTION_OK)
- {
- *err = pchomp(PQerrorMessage(conn->streamConn));
- return NULL;
- }
+ goto bad_connection_errmsg;
if (logical)
{
@@ -234,9 +228,9 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
if (PQresultStatus(res) != PGRES_TUPLES_OK)
{
PQclear(res);
- ereport(ERROR,
- (errmsg("could not clear search path: %s",
- pchomp(PQerrorMessage(conn->streamConn)))));
+ *err = psprintf(_("could not clear search path: %s"),
+ pchomp(PQerrorMessage(conn->streamConn)));
+ goto bad_connection;
}
PQclear(res);
}
@@ -244,6 +238,16 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
conn->logical = logical;
return conn;
+
+ /* error path, using libpq's error message */
+bad_connection_errmsg:
+ *err = pchomp(PQerrorMessage(conn->streamConn));
+
+ /* error path, error already set */
+bad_connection:
+ PQfinish(conn->streamConn);
+ pfree(conn);
+ return NULL;
}
/*
--
2.38.0
[text/x-diff] v3-0002-Add-helper-library-for-use-of-libpq-inside-the-se.patch (8.3K, ../../[email protected]/3-v3-0002-Add-helper-library-for-use-of-libpq-inside-the-se.patch)
download | inline diff:
From 61531d075fc385bd22f7ec4cdb38e87da8aaec6d Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 20 Jan 2023 17:15:49 -0800
Subject: [PATCH v3 2/4] Add helper library for use of libpq inside the server
environment
Currently dblink and postgres_fdw don't process interrupts during connection
establishment. Besides preventing query cancellations etc, this can lead to
undetected deadlocks, as global barriers are not processed.
Libpqwalreceiver in contrast, processes interrupts during connection
establishment. The required code is not trivial, so duplicating it into
additional places does not seem like a good option.
These aforementioned undetected deadlocks are the reason for the spate of CI
test failures in the FreeBSD 'test_running' step.
For now the helper library is just a header, as it needs to be linked into
each extension using libpq, and it seems too small to be worth adding a
dedicated static library for.
The conversion to the helper are done in subsequent commits.
Reviewed-by: Thomas Munro <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/include/libpq/libpq-be-fe-helpers.h | 242 ++++++++++++++++++++++++
1 file changed, 242 insertions(+)
create mode 100644 src/include/libpq/libpq-be-fe-helpers.h
diff --git a/src/include/libpq/libpq-be-fe-helpers.h b/src/include/libpq/libpq-be-fe-helpers.h
new file mode 100644
index 00000000000..41e3bb4376a
--- /dev/null
+++ b/src/include/libpq/libpq-be-fe-helpers.h
@@ -0,0 +1,242 @@
+/*-------------------------------------------------------------------------
+ *
+ * libpq-be-fe-helpers.h
+ * Helper functions for using libpq in extensions
+ *
+ * Code built directly into the backend is not allowed to link to libpq
+ * directly. Extension code is allowed to use libpq however. However, libpq
+ * used in extensions has to be careful to block inside libpq, otherwise
+ * interrupts will not be processed, leading to issues like unresolvable
+ * deadlocks. Backend code also needs to take care to acquire/release an
+ * external fd for the connection, otherwise fd.c's accounting of fd's is
+ * broken.
+ *
+ * This file provides helper functions to make it easier to comply with these
+ * rules. It is a header only library as it needs to be linked into each
+ * extension using libpq, and it seems too small to be worth adding a
+ * dedicated static library for.
+ *
+ * TODO: For historical reasons the connections established here are not put
+ * into non-blocking mode. That can lead to blocking even when only the async
+ * libpq functions are used. This should be fixed.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/libpq/libpq-be-fe-helpers.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef LIBPQ_BE_FE_HELPERS_H
+#define LIBPQ_BE_FE_HELPERS_H
+
+/*
+ * Despite the name, BUILDING_DLL is set only when building code directly part
+ * of the backend. Which also is where libpq isn't allowed to be
+ * used. Obviously this doesn't protect against libpq-fe.h getting included
+ * otherwise, but perhaps still protects against a few mistakes...
+ */
+#ifdef BUILDING_DLL
+#error "libpq may not be used code directly built into the backend"
+#endif
+
+#include "libpq-fe.h"
+#include "miscadmin.h"
+#include "storage/fd.h"
+#include "storage/latch.h"
+#include "utils/wait_event.h"
+
+
+static inline void libpqsrv_connect_prepare(void);
+static inline void libpqsrv_connect_internal(PGconn *conn, uint32 wait_event_info);
+
+
+/*
+ * PQconnectdb() wrapper that reserves a file descriptor and processes
+ * interrupts during connection establishment.
+ *
+ * Throws an error if AcquireExternalFD() fails, but does not throw if
+ * connection establishment itself fails. Callers need to use PQstatus() to
+ * check if connection establishment succeeded.
+ */
+static inline PGconn *
+libpqsrv_connect(const char *conninfo, uint32 wait_event_info)
+{
+ PGconn *conn = NULL;
+
+ libpqsrv_connect_prepare();
+
+ conn = PQconnectStart(conninfo);
+
+ libpqsrv_connect_internal(conn, wait_event_info);
+
+ return conn;
+}
+
+/*
+ * Like libpqsrv_connect(), except that this is a wrapper for
+ * PQconnectdbParams().
+ */
+static inline PGconn *
+libpqsrv_connect_params(const char *const *keywords,
+ const char *const *values,
+ int expand_dbname,
+ uint32 wait_event_info)
+{
+ PGconn *conn = NULL;
+
+ libpqsrv_connect_prepare();
+
+ conn = PQconnectStartParams(keywords, values, expand_dbname);
+
+ libpqsrv_connect_internal(conn, wait_event_info);
+
+ return conn;
+}
+
+/*
+ * PQfinish() wrapper that additionally releases the reserved file descriptor.
+ *
+ * It is allowed to call this with a NULL pgconn iff NULL was returned by
+ * libpqsrv_connect*.
+ */
+static inline void
+libpqsrv_disconnect(PGconn *conn)
+{
+ /*
+ * If no connection was established, we haven't reserved an FD for it (or
+ * already released it). This rule makes it easier to write PG_CATCH()
+ * handlers for this facility's users.
+ *
+ * See also libpqsrv_connect_internal().
+ */
+ if (conn == NULL)
+ return;
+
+ ReleaseExternalFD();
+ PQfinish(conn);
+}
+
+
+/* internal helper functions follow */
+
+
+/*
+ * Helper function for all connection establishment functions.
+ */
+static inline void
+libpqsrv_connect_prepare(void)
+{
+ /*
+ * We must obey fd.c's limit on non-virtual file descriptors. Assume that
+ * a PGconn represents one long-lived FD. (Doing this here also ensures
+ * that VFDs are closed if needed to make room.)
+ */
+ if (!AcquireExternalFD())
+ {
+#ifndef WIN32 /* can't write #if within ereport() macro */
+ ereport(ERROR,
+ (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not establish connection"),
+ errdetail("There are too many open files on the local server."),
+ errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
+#else
+ ereport(ERROR,
+ (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
+ errmsg("could not establish connection"),
+ errdetail("There are too many open files on the local server."),
+ errhint("Raise the server's max_files_per_process setting.")));
+#endif
+ }
+}
+
+/*
+ * Helper function for all connection establishment functions.
+ */
+static inline void
+libpqsrv_connect_internal(PGconn *conn, uint32 wait_event_info)
+{
+ /*
+ * With conn == NULL libpqsrv_disconnect() wouldn't release the FD. So do
+ * that here.
+ */
+ if (conn == NULL)
+ {
+ ReleaseExternalFD();
+ return;
+ }
+
+ /*
+ * Can't wait without a socket. Note that we don't want to close the libpq
+ * connection yet, so callers can emit a useful error.
+ */
+ if (PQstatus(conn) == CONNECTION_BAD)
+ return;
+
+ /*
+ * WaitLatchOrSocket() can conceivably fail, handle that case here instead
+ * of requiring all callers to do so.
+ */
+ PG_TRY();
+ {
+ PostgresPollingStatusType status;
+
+ /*
+ * 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) == 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);
+ }
+ }
+ PG_CATCH();
+ {
+ /*
+ * If an error is thrown here, the callers won't call
+ * libpqsrv_disconnect() with a conn, so release resources
+ * immediately.
+ */
+ ReleaseExternalFD();
+ PQfinish(conn);
+
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
+}
+
+#endif /* LIBPQ_BE_FE_HELPERS_H */
--
2.38.0
[text/x-diff] v3-0003-dblink-postgres_fdw-Handle-interrupts-during-conn.patch (8.1K, ../../[email protected]/4-v3-0003-dblink-postgres_fdw-Handle-interrupts-during-conn.patch)
download | inline diff:
From 149399a0de2f388451cfeea22d37842a1f7494ec Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 20 Jan 2023 17:33:21 -0800
Subject: [PATCH v3 3/4] dblink, postgres_fdw: Handle interrupts during
connection establishment
Until now dblink and postgres_fdw did not process interrupts during connection
establishment. Besides preventing query cancellations etc, this can lead to
undetected deadlocks, as global barriers are not processed.
These aforementioned undetected deadlocks are the reason for the spate of CI
test failures in the FreeBSD 'test_running' step.
Fix the bug by using the helper from libpq-be-fe-helpers.h, introduced in a
prior commit. Besides fixing the bug, this also removes duplicated code
around reserving file descriptors.
As the change is relatively large and there are no field reports of the
problem, don't backpatch for now.
Reviewed-by: Thomas Munro <[email protected]>
Discussion: https://postgr.es/m/[email protected]
Backpatch:
---
contrib/dblink/dblink.c | 79 +++++--------------------------
contrib/postgres_fdw/connection.c | 42 +++-------------
2 files changed, 17 insertions(+), 104 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 8dd122042b4..8982d623d3b 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -48,6 +48,7 @@
#include "funcapi.h"
#include "lib/stringinfo.h"
#include "libpq-fe.h"
+#include "libpq/libpq-be-fe-helpers.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "parser/scansup.h"
@@ -199,37 +200,14 @@ dblink_get_conn(char *conname_or_str,
connstr = conname_or_str;
dblink_connstr_check(connstr);
- /*
- * We must obey fd.c's limit on non-virtual file descriptors. Assume
- * that a PGconn represents one long-lived FD. (Doing this here also
- * ensures that VFDs are closed if needed to make room.)
- */
- if (!AcquireExternalFD())
- {
-#ifndef WIN32 /* can't write #if within ereport() macro */
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
-#else
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process setting.")));
-#endif
- }
-
/* OK to make connection */
- conn = PQconnectdb(connstr);
+ conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
char *msg = pchomp(PQerrorMessage(conn));
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
ereport(ERROR,
(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
errmsg("could not establish connection"),
@@ -312,36 +290,13 @@ dblink_connect(PG_FUNCTION_ARGS)
/* check password in connection string if not superuser */
dblink_connstr_check(connstr);
- /*
- * We must obey fd.c's limit on non-virtual file descriptors. Assume that
- * a PGconn represents one long-lived FD. (Doing this here also ensures
- * that VFDs are closed if needed to make room.)
- */
- if (!AcquireExternalFD())
- {
-#ifndef WIN32 /* can't write #if within ereport() macro */
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
-#else
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not establish connection"),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process setting.")));
-#endif
- }
-
/* OK to make connection */
- conn = PQconnectdb(connstr);
+ conn = libpqsrv_connect(connstr, PG_WAIT_EXTENSION);
if (PQstatus(conn) == CONNECTION_BAD)
{
msg = pchomp(PQerrorMessage(conn));
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
if (rconn)
pfree(rconn);
@@ -366,10 +321,7 @@ dblink_connect(PG_FUNCTION_ARGS)
else
{
if (pconn->conn)
- {
- PQfinish(pconn->conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
pconn->conn = conn;
}
@@ -402,8 +354,7 @@ dblink_disconnect(PG_FUNCTION_ARGS)
if (!conn)
dblink_conn_not_avail(conname);
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
if (rconn)
{
deleteConnection(conname);
@@ -838,10 +789,7 @@ dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
{
/* if needed, close the connection to the database */
if (freeconn)
- {
- PQfinish(conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
}
PG_END_TRY();
@@ -1516,10 +1464,7 @@ dblink_exec(PG_FUNCTION_ARGS)
{
/* if needed, close the connection to the database */
if (freeconn)
- {
- PQfinish(conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
}
PG_END_TRY();
@@ -2606,8 +2551,7 @@ createNewConnection(const char *name, remoteConn *rconn)
if (found)
{
- PQfinish(rconn->conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(rconn->conn);
pfree(rconn);
ereport(ERROR,
@@ -2647,8 +2591,7 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
{
if (!PQconnectionUsedPassword(conn))
{
- PQfinish(conn);
- ReleaseExternalFD();
+ libpqsrv_disconnect(conn);
if (rconn)
pfree(rconn);
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index ed75ce3f79c..7760380f00d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -17,6 +17,7 @@
#include "catalog/pg_user_mapping.h"
#include "commands/defrem.h"
#include "funcapi.h"
+#include "libpq/libpq-be-fe-helpers.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -446,35 +447,10 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
/* verify the set of connection parameters */
check_conn_params(keywords, values, user);
- /*
- * We must obey fd.c's limit on non-virtual file descriptors. Assume
- * that a PGconn represents one long-lived FD. (Doing this here also
- * ensures that VFDs are closed if needed to make room.)
- */
- if (!AcquireExternalFD())
- {
-#ifndef WIN32 /* can't write #if within ereport() macro */
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not connect to server \"%s\"",
- server->servername),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process and/or \"ulimit -n\" limits.")));
-#else
- ereport(ERROR,
- (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION),
- errmsg("could not connect to server \"%s\"",
- server->servername),
- errdetail("There are too many open files on the local server."),
- errhint("Raise the server's max_files_per_process setting.")));
-#endif
- }
-
/* OK to make connection */
- conn = PQconnectdbParams(keywords, values, false);
-
- if (!conn)
- ReleaseExternalFD(); /* because the PG_CATCH block won't */
+ conn = libpqsrv_connect_params(keywords, values,
+ /* expand_dbname = */ false,
+ PG_WAIT_EXTENSION);
if (!conn || PQstatus(conn) != CONNECTION_OK)
ereport(ERROR,
@@ -507,12 +483,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
}
PG_CATCH();
{
- /* Release PGconn data structure if we managed to create one */
- if (conn)
- {
- PQfinish(conn);
- ReleaseExternalFD();
- }
+ libpqsrv_disconnect(conn);
PG_RE_THROW();
}
PG_END_TRY();
@@ -528,9 +499,8 @@ disconnect_pg_server(ConnCacheEntry *entry)
{
if (entry->conn != NULL)
{
- PQfinish(entry->conn);
+ libpqsrv_disconnect(entry->conn);
entry->conn = NULL;
- ReleaseExternalFD();
}
}
--
2.38.0
[text/x-diff] v3-0004-libpqwalreceiver-Convert-to-libpq-be-fe-helpers.h.patch (3.8K, ../../[email protected]/5-v3-0004-libpqwalreceiver-Convert-to-libpq-be-fe-helpers.h.patch)
download | inline diff:
From 70db02b9f7525f290f583351b240ef9c1bf6dd5d Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 20 Jan 2023 18:37:18 -0800
Subject: [PATCH v3 4/4] libpqwalreceiver: Convert to libpq-be-fe-helpers.h
In contrast to the changes to dblink and postgres_fdw, this does not fix a
bug, as libpqwalreceiver did already process interrupts.
Besides reducing code duplication, the conversion leads to libpqwalreceiver
now using reserving file descriptors for libpq connections. While not strictly
required for the use in walreceiver, we are also using libpqwalreceiver for
logical replication, where it does seem more important.
Even if we eventually decide to backpatch the prior commits, there'd be no
need to backpatch this commit, due to not fixing an active bug.
Reviewed-by: Thomas Munro <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
.../libpqwalreceiver/libpqwalreceiver.c | 53 +++----------------
1 file changed, 7 insertions(+), 46 deletions(-)
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index fefc8660259..560ec974fa7 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -24,6 +24,7 @@
#include "common/connect.h"
#include "funcapi.h"
#include "libpq-fe.h"
+#include "libpq/libpq-be-fe-helpers.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "pgstat.h"
@@ -125,7 +126,6 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
char **err)
{
WalReceiverConn *conn;
- PostgresPollingStatusType status;
const char *keys[6];
const char *vals[6];
int i = 0;
@@ -172,49 +172,10 @@ libpqrcv_connect(const char *conninfo, bool logical, const char *appname,
Assert(i < sizeof(keys));
conn = palloc0(sizeof(WalReceiverConn));
- conn->streamConn = PQconnectStartParams(keys, vals,
- /* expand_dbname = */ true);
- if (PQstatus(conn->streamConn) == CONNECTION_BAD)
- goto bad_connection_errmsg;
-
- /*
- * Poll connection until we have OK or FAILED status.
- *
- * Per spec for PQconnectPoll, first wait till socket is write-ready.
- */
- status = PGRES_POLLING_WRITING;
- do
- {
- 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->streamConn),
- 0,
- WAIT_EVENT_LIBPQWALRECEIVER_CONNECT);
-
- /* Interrupted? */
- if (rc & WL_LATCH_SET)
- {
- ResetLatch(MyLatch);
- ProcessWalRcvInterrupts();
- }
-
- /* If socket is ready, advance the libpq state machine */
- if (rc & io_flag)
- status = PQconnectPoll(conn->streamConn);
- } while (status != PGRES_POLLING_OK && status != PGRES_POLLING_FAILED);
+ conn->streamConn =
+ libpqsrv_connect_params(keys, vals,
+ /* expand_dbname = */ true,
+ WAIT_EVENT_LIBPQWALRECEIVER_CONNECT);
if (PQstatus(conn->streamConn) != CONNECTION_OK)
goto bad_connection_errmsg;
@@ -245,7 +206,7 @@ bad_connection_errmsg:
/* error path, error already set */
bad_connection:
- PQfinish(conn->streamConn);
+ libpqsrv_disconnect(conn->streamConn);
pfree(conn);
return NULL;
}
@@ -744,7 +705,7 @@ libpqrcv_PQgetResult(PGconn *streamConn)
static void
libpqrcv_disconnect(WalReceiverConn *conn)
{
- PQfinish(conn->streamConn);
+ libpqsrv_disconnect(conn->streamConn);
PQfreemem(conn->recvBuf);
pfree(conn);
}
--
2.38.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier
2023-01-03 20:05 Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2023-01-10 02:05 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2023-01-14 01:45 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Thomas Munro <[email protected]>
2023-01-21 03:00 ` Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
@ 2023-01-24 03:28 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Andres Freund @ 2023-01-24 03:28 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Tom Lane <[email protected]>; Fujii Masao <[email protected]>; Bharath Rupireddy <[email protected]>
Hi,
On 2023-01-20 19:00:08 -0800, Andres Freund wrote:
> Updated patch attached. I split it into multiple pieces.
> 1) A fix for [1], included here because I encountered it while testing
> 2) Introduction of libpq-be-fe-helpers.h
> 3) Convert dblink and postgres_fdw to the helper
> 4) Convert libpqwalreceiver.c to the helper
>
> Even if we eventually decide to backpatch 3), we'd likely not backpatch 4), as
> there's no bug (although perhaps the lack of FD handling could be called a
> bug?).
>
> There's also some light polishing, improving commit message, comments and
> moving some internal helper functions to later in the file.
After a tiny bit further polishing, and after separately pushing a resource
leak fix for walrcv_connect(), I pushed this.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH v15 6/8] Row pattern recognition patch (docs).
@ 2024-03-28 10:30 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Tatsuo Ishii @ 2024-03-28 10:30 UTC (permalink / raw)
---
doc/src/sgml/advanced.sgml | 80 ++++++++++++++++++++++++++++++++++++
doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++
doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++-
3 files changed, 170 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 755c9f1485..cf18dd887e 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -537,6 +537,86 @@ WHERE pos < 3;
<literal>rank</literal> less than 3.
</para>
+ <para>
+ Row pattern common syntax can be used to perform row pattern recognition
+ in a query. Row pattern common syntax includes two sub
+ clauses: <literal>DEFINE</literal>
+ and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines
+ definition variables along with an expression. The expression must be a
+ logical expression, which means it must
+ return <literal>TRUE</literal>, <literal>FALSE</literal>
+ or <literal>NULL</literal>. The expression may comprise column references
+ and functions. Window functions, aggregate functions and subqueries are
+ not allowed. An example of <literal>DEFINE</literal> is as follows.
+
+<programlisting>
+DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+</programlisting>
+
+ Note that <function>PREV</function> returns the price column in the
+ previous row if it's called in a context of row pattern recognition. So in
+ the second line the definition variable "UP" is <literal>TRUE</literal>
+ when the price column in the current row is greater than the price column
+ in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when
+ the price column in the current row is lower than the price column in the
+ previous row.
+ </para>
+ <para>
+ Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be
+ used. <literal>PATTERN</literal> defines a sequence of rows that satisfies
+ certain conditions. For example following <literal>PATTERN</literal>
+ defines that a row starts with the condition "LOWPRICE", then one or more
+ rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that
+ "+" means one or more matches. Also you can use "*", which means zero or
+ more matches. If a sequence of rows which satisfies the PATTERN is found,
+ in the starting row of the sequence of rows all window functions and
+ aggregates are shown in the target list. Note that aggregations only look
+ into the matched rows, rather than whole frame. In the second or
+ subsequent rows all window functions and aggregates are NULL. For rows
+ that do not match the PATTERN, all window functions and aggregates are
+ shown AS NULL too, except count which shows 0. This is because the
+ unmatched rows are in an empty frame. Example of
+ a <literal>SELECT</literal> using the <literal>DEFINE</literal>
+ and <literal>PATTERN</literal> clause is as follows.
+
+<programlisting>
+SELECT company, tdate, price,
+ first_value(price) OVER w,
+ max(price) OVER w,
+ count(price) OVER w
+FROM stock,
+ WINDOW w AS (
+ PARTITION BY company
+ ORDER BY tdate
+ ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
+ AFTER MATCH SKIP PAST LAST ROW
+ INITIAL
+ PATTERN (LOWPRICE UP+ DOWN+)
+ DEFINE
+ LOWPRICE AS price <= 100,
+ UP AS price > PREV(price),
+ DOWN AS price < PREV(price)
+);
+</programlisting>
+<screen>
+ company | tdate | price | first_value | max | count
+----------+------------+-------+-------------+-----+-------
+ company1 | 2023-07-01 | 100 | 100 | 200 | 4
+ company1 | 2023-07-02 | 200 | | |
+ company1 | 2023-07-03 | 150 | | |
+ company1 | 2023-07-04 | 140 | | |
+ company1 | 2023-07-05 | 150 | | | 0
+ company1 | 2023-07-06 | 90 | 90 | 130 | 4
+ company1 | 2023-07-07 | 110 | | |
+ company1 | 2023-07-08 | 130 | | |
+ company1 | 2023-07-09 | 120 | | |
+ company1 | 2023-07-10 | 130 | | | 0
+</screen>
+ </para>
+
<para>
When a query involves multiple window functions, it is possible to write
out each one with a separate <literal>OVER</literal> clause, but this is
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 93b0bc2bc6..d25eeb3327 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -22637,6 +22637,7 @@ SELECT count(*) FROM sometable;
returns <literal>NULL</literal> if there is no such row.
</para></entry>
</row>
+
</tbody>
</tgroup>
</table>
@@ -22676,6 +22677,59 @@ SELECT count(*) FROM sometable;
Other frame specifications can be used to obtain other effects.
</para>
+ <para>
+ Row pattern recognition navigation functions are listed in
+ <xref linkend="functions-rpr-navigation-table"/>. These functions
+ can be used to describe DEFINE clause of Row pattern recognition.
+ </para>
+
+ <table id="functions-rpr-navigation-table">
+ <title>Row Pattern Navigation Functions</title>
+ <tgroup cols="1">
+ <thead>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ Function
+ </para>
+ <para>
+ Description
+ </para></entry>
+ </row>
+ </thead>
+
+ <tbody>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>prev</primary>
+ </indexterm>
+ <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the previous row;
+ returns NULL if there is no previous row in the window frame.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>next</primary>
+ </indexterm>
+ <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> )
+ <returnvalue>anyelement</returnvalue>
+ </para>
+ <para>
+ Returns the column value at the next row;
+ returns NULL if there is no next row in the window frame.
+ </para></entry>
+ </row>
+
+ </tbody>
+ </tgroup>
+ </table>
+
<note>
<para>
The SQL standard defines a <literal>RESPECT NULLS</literal> or
diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index 066aed44e6..8f18718d58 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl
The <replaceable class="parameter">frame_clause</replaceable> can be one of
<synopsis>
-{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
-{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ]
+{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
+{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax]
</synopsis>
where <replaceable>frame_start</replaceable>
@@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS
a given peer group will be in the frame or excluded from it.
</para>
+ <para>
+ The
+ optional <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ defines the <firstterm>row pattern recognition condition</firstterm> for
+ this
+ window. <replaceable class="parameter">row_pattern_common_syntax</replaceable>
+ includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST
+ ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls
+ how to proceed to next row position after a match
+ found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the
+ default) next row position is next to the last row of previous match. On
+ the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next
+ row position is always next to the last row of previous
+ match. <literal>DEFINE</literal> defines definition variables along with a
+ boolean expression. <literal>PATTERN</literal> defines a sequence of rows
+ that satisfies certain conditions using variables defined
+ in <literal>DEFINE</literal> clause. If the variable is not defined in
+ the <literal>DEFINE</literal> clause, it is implicitly assumed
+ following is defined in the <literal>DEFINE</literal> clause.
+
+<synopsis>
+<literal>variable_name</literal> AS TRUE
+</synopsis>
+
+ Note that the maximu number of variables defined
+ in <literal>DEFINE</literal> clause is 26.
+
+<synopsis>
+[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ]
+PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...]
+DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...]
+</synopsis>
+ </para>
+
<para>
The purpose of a <literal>WINDOW</literal> clause is to specify the
behavior of <firstterm>window functions</firstterm> appearing in the query's
--
2.25.1
----Next_Part(Thu_Mar_28_19_59_25_2024_076)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v15-0007-Row-pattern-recognition-patch-tests.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-03-28 10:30 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-01-03 20:05 Re: postgres_fdw uninterruptible during connection establishment / ProcSignalBarrier Andres Freund <[email protected]>
2023-01-10 02:05 ` Andres Freund <[email protected]>
2023-01-14 01:45 ` Thomas Munro <[email protected]>
2023-01-21 03:00 ` Andres Freund <[email protected]>
2023-01-24 03:28 ` Andres Freund <[email protected]>
2024-03-28 10:30 [PATCH v15 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[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