remove reset_shared()
Hi hackers,
Is there any reason to keep reset_shared() around anymore? It is now just
a wrapper function for CreateSharedMemoryAndSemaphores(), and AFAICT the
information in the comments is already covered by comments in the shared
memory code. I think it's arguable that the name of the function makes it
clear that it might recreate the shared memory, but if that is a concern,
perhaps we could rename the function to something like
CreateOrRecreateSharedMemoryAndSemaphores().
I've attached a patch that simply removes this wrapper function. This is
admittedly just nitpicking, so I don't intend to carry this patch further
if anyone is opposed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Remove-reset_shared.patchtext/x-diff; charset=us-asciiDownload
From 295e1e93a2ff4e655e85040d931c0d332d14b5bd Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 29 Mar 2022 14:56:47 -0700
Subject: [PATCH v1 1/1] Remove reset_shared().
This is now just a wrapper for CreateSharedMemoryAndSemaphores(),
and the information in the comments is already covered by comments
in the shared memory code.
---
src/backend/postmaster/postmaster.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 80bb269599..f62d12e74a 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -399,7 +399,6 @@ static void getInstallationPaths(const char *argv0);
static void checkControlFile(void);
static Port *ConnCreate(int serverFd);
static void ConnFree(Port *port);
-static void reset_shared(void);
static void SIGHUP_handler(SIGNAL_ARGS);
static void pmdie(SIGNAL_ARGS);
static void reaper(SIGNAL_ARGS);
@@ -1072,7 +1071,7 @@ PostmasterMain(int argc, char *argv[])
/*
* Set up shared memory and semaphores.
*/
- reset_shared();
+ CreateSharedMemoryAndSemaphores();
/*
* Estimate number of openable files. This must happen after setting up
@@ -2736,23 +2735,6 @@ InitProcessGlobals(void)
}
-/*
- * reset_shared -- reset shared memory and semaphores
- */
-static void
-reset_shared(void)
-{
- /*
- * Create or re-create shared memory and semaphores.
- *
- * Note: in each "cycle of life" we will normally assign the same IPC keys
- * (if using SysV shmem and/or semas). This helps ensure that we will
- * clean up dead IPC objects if the postmaster crashes and is restarted.
- */
- CreateSharedMemoryAndSemaphores();
-}
-
-
/*
* SIGHUP -- reread config files, and tell children to do same
*/
@@ -4109,7 +4091,7 @@ PostmasterStateMachine(void)
/* re-read control file into local memory */
LocalProcessControlFile(true);
- reset_shared();
+ CreateSharedMemoryAndSemaphores();
StartupPID = StartupDataBase();
Assert(StartupPID != 0);
--
2.25.1
Hi,
On Tue, Mar 29, 2022 at 03:17:02PM -0700, Nathan Bossart wrote:
Hi hackers,
Is there any reason to keep reset_shared() around anymore? It is now just
a wrapper function for CreateSharedMemoryAndSemaphores(), and AFAICT the
information in the comments is already covered by comments in the shared
memory code. I think it's arguable that the name of the function makes it
clear that it might recreate the shared memory, but if that is a concern,
perhaps we could rename the function to something like
CreateOrRecreateSharedMemoryAndSemaphores().I've attached a patch that simply removes this wrapper function. This is
admittedly just nitpicking, so I don't intend to carry this patch further
if anyone is opposed.
I'm +0.5 for it, it doesn't bring much and makes things a bit harder to
understand, as you need to go through an extra function.
Hi!
In general I'm for this patch. Some time ago I was working on a patch
related to shared memory and noticed
no reason to have reset_shared() function.
--
Best regards,
Maxim Orlov.
On Fri, 15 Jul 2022 at 16:41, Maxim Orlov <orlovmg@gmail.com> wrote:
Hi!
In general I'm for this patch. Some time ago I was working on a patch
related to shared memory and noticed
no reason to have reset_shared() function.
Hi, hackers!
I see the proposed patch as uncontroversial and good enough to be
committed. It will make the code a little clearer. Personally, I don't like
leaving functions that are just wrappers for another and called only once.
But I think that if there's a question of code readability it's not bad to
restore the comments on the purpose of a call that were originally in the
code.
PFA v2 of a patch (only the comment removed in v1 is restored in v2).
Overall I'd like to move it to RfC if none have any objections.
Attachments:
v2-0001-Remove-a-wrapper-for-CreateSharedMemoryAndSemapho.patchapplication/octet-stream; name=v2-0001-Remove-a-wrapper-for-CreateSharedMemoryAndSemapho.patchDownload
From 7ca25429c9cbd073eaacd0e767ede4e7a513148a Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Fri, 15 Jul 2022 16:39:43 +0400
Subject: [PATCH v2] Remove a wrapper for CreateSharedMemoryAndSemaphores()
---
src/backend/postmaster/postmaster.c | 29 +++++++++--------------------
1 file changed, 9 insertions(+), 20 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index d7257e4056b..b409beb5250 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -391,7 +391,6 @@ static void getInstallationPaths(const char *argv0);
static void checkControlFile(void);
static Port *ConnCreate(int serverFd);
static void ConnFree(Port *port);
-static void reset_shared(void);
static void SIGHUP_handler(SIGNAL_ARGS);
static void pmdie(SIGNAL_ARGS);
static void reaper(SIGNAL_ARGS);
@@ -1082,7 +1081,7 @@ PostmasterMain(int argc, char *argv[])
/*
* Set up shared memory and semaphores.
*/
- reset_shared();
+ CreateSharedMemoryAndSemaphores();
/*
* Estimate number of openable files. This must happen after setting up
@@ -2723,23 +2722,6 @@ InitProcessGlobals(void)
}
-/*
- * reset_shared -- reset shared memory and semaphores
- */
-static void
-reset_shared(void)
-{
- /*
- * Create or re-create shared memory and semaphores.
- *
- * Note: in each "cycle of life" we will normally assign the same IPC keys
- * (if using SysV shmem and/or semas). This helps ensure that we will
- * clean up dead IPC objects if the postmaster crashes and is restarted.
- */
- CreateSharedMemoryAndSemaphores();
-}
-
-
/*
* SIGHUP -- reread config files, and tell children to do same
*/
@@ -4022,7 +4004,14 @@ PostmasterStateMachine(void)
/* re-read control file into local memory */
LocalProcessControlFile(true);
- reset_shared();
+ /*
+ * Create or re-create shared memory and semaphores.
+ *
+ * Note: in each "cycle of life" we will normally assign the same IPC keys
+ * (if using SysV shmem and/or semas). This helps ensure that we will
+ * clean up dead IPC objects if the postmaster crashes and is restarted.
+ */
+ CreateSharedMemoryAndSemaphores();
StartupPID = StartupDataBase();
Assert(StartupPID != 0);
--
2.24.3 (Apple Git-128)
Pavel Borisov <pashkin.elfe@gmail.com> writes:
I see the proposed patch as uncontroversial and good enough to be
committed. It will make the code a little clearer. Personally, I don't like
leaving functions that are just wrappers for another and called only once.
Yeah, I like this for a different reason: just a couple days ago I was
comparing the postmaster's startup sequence to that used in standalone
mode (in postgres.c) and was momentarily confused because one had
reset_shared() where the other had CreateSharedMemoryAndSemaphores().
Looking in our git history, it seems that reset_shared() used to embed
slightly more knowledge, but it sure looks pretty pointless now.
But I think that if there's a question of code readability it's not bad to
restore the comments on the purpose of a call that were originally in the
code.
Actually I think you chose the wrong place to move the comment to.
It applies to the initial postmaster start, because what it's pointing
out is that we'll probably choose the same IPC keys as any previous
run did. If we felt the need to enforce that during a crash restart,
we surely could do so directly.
Pushed after fiddling with the comments.
regards, tom lane
On Sat, Jul 16, 2022 at 12:34:11PM -0400, Tom Lane wrote:
Pushed after fiddling with the comments.
Thanks!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com