Thread: 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
Attachment
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 noticedno 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.
Attachment
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