pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedremove reset_shared()
6+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2022-07-16 5e692dcaca Remove postmaster.c's reset_shared() wrapper function.
* remove reset_shared()
@ 2022-03-29 22:17 Nathan Bossart <nathandbossart@gmail.com>
2022-03-30 01:19 ` Re: remove reset_shared() Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Nathan Bossart @ 2022-03-29 22:17 UTC (permalink / raw)
To: pgsql-hackers
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.patch (text/x-diff, 2.1K) download
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
* Re: remove reset_shared()
2022-03-29 22:17 remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
@ 2022-03-30 01:19 ` Julien Rouhaud <rjuju123@gmail.com>
2022-07-15 12:40 ` Re: remove reset_shared() Maxim Orlov <orlovmg@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Julien Rouhaud @ 2022-03-30 01:19 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: pgsql-hackers
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: remove reset_shared()
2022-03-29 22:17 remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
2022-03-30 01:19 ` Re: remove reset_shared() Julien Rouhaud <rjuju123@gmail.com>
@ 2022-07-15 12:40 ` Maxim Orlov <orlovmg@gmail.com>
2022-07-15 12:48 ` Re: remove reset_shared() Pavel Borisov <pashkin.elfe@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Maxim Orlov @ 2022-07-15 12:40 UTC (permalink / raw)
To: Julien Rouhaud <rjuju123@gmail.com>; +Cc: Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: remove reset_shared()
2022-03-29 22:17 remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
2022-03-30 01:19 ` Re: remove reset_shared() Julien Rouhaud <rjuju123@gmail.com>
2022-07-15 12:40 ` Re: remove reset_shared() Maxim Orlov <orlovmg@gmail.com>
@ 2022-07-15 12:48 ` Pavel Borisov <pashkin.elfe@gmail.com>
2022-07-16 16:34 ` Re: remove reset_shared() Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Borisov @ 2022-07-15 12:48 UTC (permalink / raw)
To: Maxim Orlov <orlovmg@gmail.com>; +Cc: Julien Rouhaud <rjuju123@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
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.patch (application/octet-stream, 2.2K) download
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)
* Re: remove reset_shared()
2022-03-29 22:17 remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
2022-03-30 01:19 ` Re: remove reset_shared() Julien Rouhaud <rjuju123@gmail.com>
2022-07-15 12:40 ` Re: remove reset_shared() Maxim Orlov <orlovmg@gmail.com>
2022-07-15 12:48 ` Re: remove reset_shared() Pavel Borisov <pashkin.elfe@gmail.com>
@ 2022-07-16 16:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-07-16 20:30 ` Re: remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2022-07-16 16:34 UTC (permalink / raw)
To: Pavel Borisov <pashkin.elfe@gmail.com>; +Cc: Maxim Orlov <orlovmg@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; Nathan Bossart <nathandbossart@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: remove reset_shared()
2022-03-29 22:17 remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
2022-03-30 01:19 ` Re: remove reset_shared() Julien Rouhaud <rjuju123@gmail.com>
2022-07-15 12:40 ` Re: remove reset_shared() Maxim Orlov <orlovmg@gmail.com>
2022-07-15 12:48 ` Re: remove reset_shared() Pavel Borisov <pashkin.elfe@gmail.com>
2022-07-16 16:34 ` Re: remove reset_shared() Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-07-16 20:30 ` Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Bossart @ 2022-07-16 20:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pavel Borisov <pashkin.elfe@gmail.com>; Maxim Orlov <orlovmg@gmail.com>; Julien Rouhaud <rjuju123@gmail.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-07-16 20:30 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29 22:17 remove reset_shared() Nathan Bossart <nathandbossart@gmail.com>
2022-03-30 01:19 ` Julien Rouhaud <rjuju123@gmail.com>
2022-07-15 12:40 ` Maxim Orlov <orlovmg@gmail.com>
2022-07-15 12:48 ` Pavel Borisov <pashkin.elfe@gmail.com>
2022-07-16 16:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-07-16 20:30 ` Nathan Bossart <nathandbossart@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox