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

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(at)gmail(dot)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.

Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)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