Little cleanup of ShmemInit function names
It's bothered me for a long time that some of the shmem initialization
functions have non-standard names. Most of them are called
FoobarShmemSize() and FoobarShmemInit(), but there are a few exceptions:
InitBufferPool
InitLocks
InitPredicateLocks
CreateSharedProcArray
CreateSharedBackendStatus
CreateSharedInvalidationState
I always have trouble remembering what exactly these functions do and
when get called. But they are the same as all the FoobarShmemInit()
functions, just named differently.
The attached patches rename them to follow the usual naming convention.
The InitLocks() function is refactored a bit more, splitting the
per-backend initialization to a separate InitLockManagerAccess()
function. That's why it's in a separate commit.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
v1-0001-Refactor-lock-manager-initialization-to-make-it-a.patchtext/x-patch; charset=UTF-8; name=v1-0001-Refactor-lock-manager-initialization-to-make-it-a.patchDownload+25-23
v1-0002-Rename-some-shared-memory-initialization-routines.patchtext/x-patch; charset=UTF-8; name=v1-0002-Rename-some-shared-memory-initialization-routines.patchDownload+33-34
On 8/7/24 2:08 PM, Heikki Linnakangas wrote:
The attached patches rename them to follow the usual naming convention.
The InitLocks() function is refactored a bit more, splitting the
per-backend initialization to a separate InitLockManagerAccess()
function. That's why it's in a separate commit.
I like the idea behind the patches. And they still apply and build.
The first patch is clean and well commented. I just have two minor nitpicks.
Small typo with the extra "which" which makes the sentence not flow
correctly
"This is called from CreateSharedMemoryAndSemaphores(), which see for
more comments."
On the topic of minor language issues I think the comma below is redundant.
"In the normal postmaster case, the shared hash tables are created here."
The second patch is a simple renaming which reduces mental load by
making the naming more consistent so I like it. Also since these
functions are not really useful for any extension authors I do not see
any harm in renaming them.
After cleaning up the language of that comment I think these patches can
be committed.
Andreas
On 28/08/2024 18:26, Andreas Karlsson wrote:
On 8/7/24 2:08 PM, Heikki Linnakangas wrote:
The attached patches rename them to follow the usual naming convention.
The InitLocks() function is refactored a bit more, splitting the
per-backend initialization to a separate InitLockManagerAccess()
function. That's why it's in a separate commit.I like the idea behind the patches. And they still apply and build.
The first patch is clean and well commented. I just have two minor nitpicks.
Small typo with the extra "which" which makes the sentence not flow
correctly"This is called from CreateSharedMemoryAndSemaphores(), which see for
more comments."
Hmm, I don't see the issue. It's an uncommon sentence structure, but it
was there before this patch, and it's correct AFAICS. If you grep for
"which see ", you'll find some more examples of that.
On the topic of minor language issues I think the comma below is redundant.
"In the normal postmaster case, the shared hash tables are created here."
On second thoughts, I rearranged the sentences in the paragraph a
little, see attached.
The second patch is a simple renaming which reduces mental load by
making the naming more consistent so I like it. Also since these
functions are not really useful for any extension authors I do not see
any harm in renaming them.After cleaning up the language of that comment I think these patches can
be committed.
Thanks for the review!
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
v2-0001-Refactor-lock-manager-initialization-to-make-it-a.patchtext/x-patch; charset=UTF-8; name=v2-0001-Refactor-lock-manager-initialization-to-make-it-a.patchDownload+25-23
v2-0002-Rename-some-shared-memory-initialization-routines.patchtext/x-patch; charset=UTF-8; name=v2-0002-Rename-some-shared-memory-initialization-routines.patchDownload+33-34
On 8/28/24 7:26 PM, Heikki Linnakangas wrote:
Hmm, I don't see the issue. It's an uncommon sentence structure, but it
was there before this patch, and it's correct AFAICS. If you grep for
"which see ", you'll find some more examples of that.
Not sure if it is correct or not but from some googling it seems to be a
direct translation of "quod vide". I think "for which, see" would likely
be more proper English but it is not my native language and we use
"which see" elsewhere so we might as well be consistent and use "which see".
On the topic of minor language issues I think the comma below is
redundant."In the normal postmaster case, the shared hash tables are created here."
On second thoughts, I rearranged the sentences in the paragraph a
little, see attached.
Looks good!
Andreas
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 28/08/2024 18:26, Andreas Karlsson wrote:
Small typo with the extra "which" which makes the sentence not flow
correctly
"This is called from CreateSharedMemoryAndSemaphores(), which see for
more comments."
Hmm, I don't see the issue. It's an uncommon sentence structure, but it
was there before this patch, and it's correct AFAICS. If you grep for
"which see ", you'll find some more examples of that.
I didn't check the git history for this particular line, but at least
some of those examples are mine. I'm pretty certain it's good English.
regards, tom lane
On Wed, Aug 28, 2024 at 2:41 PM Andreas Karlsson <andreas@proxel.se> wrote:
Not sure if it is correct or not but from some googling it seems to be a
direct translation of "quod vide". I think "for which, see" would likely
be more proper English but it is not my native language and we use
"which see" elsewhere so we might as well be consistent and use "which see".
If somebody wrote "for which, see" in a patch I was reviewing, I would
definitely complain about it.
I wouldn't complain about "which see", but that's mostly because I
know Tom likes the expression. As a native English speaker, it sounds
basically grammatical to me, but it's an extremely uncommon usage. I
prefer to phrase things in ways that are closer to how people actually
talk, partly because I know that we do have many people working on the
project who are not native speakers of English, and are thus more
likely to be tripped up by obscure usages.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2024-Aug-28, Tom Lane wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
Hmm, I don't see the issue. It's an uncommon sentence structure, but it
was there before this patch, and it's correct AFAICS. If you grep for
"which see ", you'll find some more examples of that.I didn't check the git history for this particular line, but at least
some of those examples are mine. I'm pretty certain it's good English.
As a non-native speaker, I'm always grateful for examples of unusual
grammatical constructs. They have given me many more opportunities for
growth than if all comments were restricted to simple English. I have
had many a chance to visit english.stackexchange.net on account of
something I read in pgsql lists or code comments.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 29/08/2024 04:06, Alvaro Herrera wrote:
I have had many a chance to visit english.stackexchange.net on
account of something I read in pgsql lists or code comments.
I see what you did there :-).
Committed, with "which see". Thanks everyone!
--
Heikki Linnakangas
Neon (https://neon.tech)