Horton

Thread

Commits

  1. Rename some shared memory initialization routines

  2. Refactor lock manager initialization to make it a bit less special

  1. Little cleanup of ShmemInit function names

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-07T12:08:42Z

    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)

  2. Re: Little cleanup of ShmemInit function names

    Andreas Karlsson <andreas@proxel.se> — 2024-08-28T15:26:38Z

    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

  3. Re: Little cleanup of ShmemInit function names

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-28T17:26:06Z

    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)

  4. Re: Little cleanup of ShmemInit function names

    Andreas Karlsson <andreas@proxel.se> — 2024-08-28T18:41:22Z

    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

  5. Re: Little cleanup of ShmemInit function names

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-08-28T18:44:36Z

    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

  6. Re: Little cleanup of ShmemInit function names

    Robert Haas <robertmhaas@gmail.com> — 2024-08-28T19:15:02Z

    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

  7. Re: Little cleanup of ShmemInit function names

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-08-29T01:06:45Z

    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/

  8. Re: Little cleanup of ShmemInit function names

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-08-29T07:01:34Z

    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)