Focusing on this one patch in this series:
On 11/07/2023 01:50, Andres Freund wrote:
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 12 Jun 2023 16:33:20 +0300
Subject: [PATCH 4/9] Use FD_CLOEXEC on ListenSockets
We went through some effort to close them in the child process. Better to
not hand them down to the child process in the first place.
I think Thomas has a larger version of this patch:
https://postgr.es/m/CA%2BhUKGKPNFcfBQduqof4-7C%3DavjcSfdkKBGvQoRuAvfocnvY0A%40mail.gmail.com
Hmm, no, that's a little different. Thomas added the FD_CLOEXEC option
to the *accepted* socket in commit 1da569ca1f. That was part of that
thread. This patch adds the option to the *listen* sockets. That was not
discussed in that thread, but it's certainly in the same vein.
Thomas: What do you think of the attached?
On 11/07/2023 00:07, Tristan Partin wrote:
@@ -831,7 +834,8 @@ StreamConnection(pgsocket server_fd, Port *port)
void
StreamClose(pgsocket sock)
{
- closesocket(sock);
+ if (closesocket(sock) != 0)
+ elog(LOG, "closesocket failed: %m");
}
/*
Do you think WARNING would be a more appropriate log level?
No, WARNING is for messages that you expect the client to receive. This
failure is unexpected at the system level, the message is for the
administrator. The distinction isn't always very clear, but LOG seems
more appropriate in this case.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 95bd11720c8e88a1f65b9d600f50d71662241ba9 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 24 Aug 2023 13:53:33 +0300
Subject: [PATCH v2 1/1] Use FD_CLOEXEC on ListenSockets
It's good hygiene if e.g. an extension launches a subprogram when
being loaded. And we went through some effort to close them in the
child process in EXEC_BACKEND mode; better to not hand them down to
the child process in the first place. We still need to close them
after fork when !EXEC_BACKEND, but it's a little simpler.
In the passing, LOG a message if closing the client connection or
listen socket fails. Shouldn't happen, but if it does, would be nice
to know.
Reviewed-by: Tristan Partin, Andres Freund
Discussion: https://www.postgresql.org/message-id/7a59b073-5b5b-151e-7ed3-8b01ff7ce...@iki.fi
---
src/backend/libpq/pqcomm.c | 6 +++++-
src/backend/postmaster/postmaster.c | 14 ++++++--------
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 1bdcbe4f636..8d217b06455 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -458,6 +458,9 @@ StreamServerPort(int family, const char *hostName, unsigned short portNumber,
}
#ifndef WIN32
+ /* Don't give the listen socket to any subprograms we execute. */
+ if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)
+ elog(FATAL, "fcntl(F_SETFD) failed on socket: %m");
/*
* Without the SO_REUSEADDR flag, a new postmaster can't be started
@@ -831,7 +834,8 @@ StreamConnection(pgsocket server_fd, Port *port)
void
StreamClose(pgsocket sock)
{
- closesocket(sock);
+ if (closesocket(sock) != 0)
+ elog(LOG, "could not close client or listen socket: %m");
}
/*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 07d376d77ec..41bccb46a87 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -496,7 +496,6 @@ typedef struct
Port port;
InheritableSocket portsocket;
char DataDir[MAXPGPATH];
- pgsocket ListenSocket[MAXLISTEN];
int32 MyCancelKey;
int MyPMChildSlot;
#ifndef WIN32
@@ -2545,8 +2544,6 @@ ConnFree(Port *port)
void
ClosePostmasterPorts(bool am_syslogger)
{
- int i;
-
/* Release resources held by the postmaster's WaitEventSet. */
if (pm_wait_set)
{
@@ -2573,8 +2570,12 @@ ClosePostmasterPorts(bool am_syslogger)
/*
* Close the postmaster's listen sockets. These aren't tracked by fd.c,
* so we don't call ReleaseExternalFD() here.
+ *
+ * The listen sockets are marked as FD_CLOEXEC, so this isn't needed in
+ * EXEC_BACKEND mode.
*/
- for (i = 0; i < MAXLISTEN; i++)
+#ifndef EXEC_BACKEND
+ for (int i = 0; i < MAXLISTEN; i++)
{
if (ListenSocket[i] != PGINVALID_SOCKET)
{
@@ -2582,6 +2583,7 @@ ClosePostmasterPorts(bool am_syslogger)
ListenSocket[i] = PGINVALID_SOCKET;
}
}
+#endif
/*
* If using syslogger, close the read side of the pipe. We don't bother
@@ -6038,8 +6040,6 @@ save_backend_variables(BackendParameters *param, Port *port,
strlcpy(param->DataDir, DataDir, MAXPGPATH);
- memcpy(¶m->ListenSocket, &ListenSocket, sizeof(ListenSocket));
-
param->MyCancelKey = MyCancelKey;
param->MyPMChildSlot = MyPMChildSlot;
@@ -6271,8 +6271,6 @@ restore_backend_variables(BackendParameters *param, Port *port)
SetDataDir(param->DataDir);
- memcpy(&ListenSocket, ¶m->ListenSocket, sizeof(ListenSocket));
-
MyCancelKey = param->MyCancelKey;
MyPMChildSlot = param->MyPMChildSlot;
--
2.39.2
