Thread: 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)
Attachment
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 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)