Horton

Thread

Commits

  1. doc: Reorganize section for shared memory and LWLocks.

  1. reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-12T04:14:30Z

    I recently began trying to write documentation for the dynamic shared
memory registry feature [0], and I noticed that the "Shared Memory and
LWLocks" section of the documentation might need some improvement.  At
least, I felt that it would be hard to add any new content to this section
without making it very difficult to follow.

Concretely, I am proposing breaking it into two sections: one for shared
memory and one for LWLocks.  Furthermore, the LWLocks section will be split
into two: one for requesting locks at server startup and one for requesting
locks after server startup.  I intend to also split the shared memory
section into at-startup and after-startup sections if/when the dynamic
shared memory registry feature is committed.

Besides this restructuring, I felt that certain parts of this documentation
could benefit from rephrasing and/or additional detail.

Thoughts?

[0] https://postgr.es/m/20231205034647.GA2705267%40nathanxps13

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  2. Re: reorganize "Shared Memory and LWLocks" section of docs

    Aleksander Alekseev <aleksander@timescale.com> — 2024-01-12T14:12:28Z

    Hi,

> I recently began trying to write documentation for the dynamic shared
> memory registry feature [0], and I noticed that the "Shared Memory and
> LWLocks" section of the documentation might need some improvement.

I know that feeling.

> Thoughts?

"""
Any registered shmem_startup_hook will be executed shortly after each
backend attaches to shared memory.
"""

IMO the word "each" here can give the wrong impression as if there are
certain guarantees about synchronization between backends. Maybe we
should change this to simply "... will be executed shortly after
[the?] backend attaches..."

"""
should ensure that only one process allocates a new tranche_id
(LWLockNewTrancheId) and initializes each new LWLock
(LWLockInitialize).
"""

Personally I think that reminding the corresponding function name here
is redundant and complicates reading just a bit. But maybe it's just
me.

Except for these nitpicks the patch looks good.

-- 
Best regards,
Aleksander Alekseev

  3. Re: reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-12T15:46:50Z

    Thanks for reviewing.

On Fri, Jan 12, 2024 at 05:12:28PM +0300, Aleksander Alekseev wrote:
> """
> Any registered shmem_startup_hook will be executed shortly after each
> backend attaches to shared memory.
> """
> 
> IMO the word "each" here can give the wrong impression as if there are
> certain guarantees about synchronization between backends. Maybe we
> should change this to simply "... will be executed shortly after
> [the?] backend attaches..."

I see what you mean, but I don't think the problem is the word "each."  I
think the problem is the use of passive voice.  What do you think about
something like

	Each backend will execute the registered shmem_startup_hook shortly
	after it attaches to shared memory.

> """
> should ensure that only one process allocates a new tranche_id
> (LWLockNewTrancheId) and initializes each new LWLock
> (LWLockInitialize).
> """
> 
> Personally I think that reminding the corresponding function name here
> is redundant and complicates reading just a bit. But maybe it's just
> me.

Yeah, I waffled on this one.  I don't mind removing it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  4. Re: reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-12T17:23:50Z

    On Fri, Jan 12, 2024 at 09:46:50AM -0600, Nathan Bossart wrote:
> On Fri, Jan 12, 2024 at 05:12:28PM +0300, Aleksander Alekseev wrote:
>> """
>> Any registered shmem_startup_hook will be executed shortly after each
>> backend attaches to shared memory.
>> """
>> 
>> IMO the word "each" here can give the wrong impression as if there are
>> certain guarantees about synchronization between backends. Maybe we
>> should change this to simply "... will be executed shortly after
>> [the?] backend attaches..."
> 
> I see what you mean, but I don't think the problem is the word "each."  I
> think the problem is the use of passive voice.  What do you think about
> something like
> 
> 	Each backend will execute the registered shmem_startup_hook shortly
> 	after it attaches to shared memory.
> 
>> """
>> should ensure that only one process allocates a new tranche_id
>> (LWLockNewTrancheId) and initializes each new LWLock
>> (LWLockInitialize).
>> """
>> 
>> Personally I think that reminding the corresponding function name here
>> is redundant and complicates reading just a bit. But maybe it's just
>> me.
> 
> Yeah, I waffled on this one.  I don't mind removing it.

Here is a new version of the patch with these changes.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  5. Re: reorganize "Shared Memory and LWLocks" section of docs

    Aleksander Alekseev <aleksander@timescale.com> — 2024-01-13T10:49:08Z

    Hi,

Thanks for the updated patch.

> > I see what you mean, but I don't think the problem is the word "each."  I
> > think the problem is the use of passive voice.  What do you think about
> > something like
> >
> >       Each backend will execute the registered shmem_startup_hook shortly
> >       after it attaches to shared memory.

That's much better, thanks.

I think the patch could use another pair of eyes, ideally from a
native English speaker. But if no one will express any objections for
a while I suggest merging it.

-- 
Best regards,
Aleksander Alekseev

  6. Re: reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-13T21:28:15Z

    On Sat, Jan 13, 2024 at 01:49:08PM +0300, Aleksander Alekseev wrote:
> That's much better, thanks.
> 
> I think the patch could use another pair of eyes, ideally from a
> native English speaker. But if no one will express any objections for
> a while I suggest merging it.

Great.  I've attached a v3 with a couple of fixes suggested in the other
thread [0].  I'll wait a little while longer in case anyone else wants to
take a look.

[0] https://postgr.es/m/ZaF6UpYImGqVIhVp%40toroid.org

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  7. Re: reorganize "Shared Memory and LWLocks" section of docs

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-16T04:32:15Z

    On Sun, Jan 14, 2024 at 2:58 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> Great.  I've attached a v3 with a couple of fixes suggested in the other
> thread [0].  I'll wait a little while longer in case anyone else wants to
> take a look.

The v3 patch looks good to me except for a nitpick: the input
parameter for RequestAddinShmemSpace is 'Size' not 'int'

 <programlisting>
 void RequestAddinShmemSpace(int size)
 </programlisting>

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  8. Re: reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-16T14:20:19Z

    On Tue, Jan 16, 2024 at 10:02:15AM +0530, Bharath Rupireddy wrote:
> The v3 patch looks good to me except for a nitpick: the input
> parameter for RequestAddinShmemSpace is 'Size' not 'int'
> 
>  <programlisting>
>  void RequestAddinShmemSpace(int size)
>  </programlisting>

Hah, I think this mistake is nearly old enough to vote (e0dece1, 5f78aa5).
Good catch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  9. Re: reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-16T15:52:52Z

    On Tue, Jan 16, 2024 at 08:20:19AM -0600, Nathan Bossart wrote:
> On Tue, Jan 16, 2024 at 10:02:15AM +0530, Bharath Rupireddy wrote:
>> The v3 patch looks good to me except for a nitpick: the input
>> parameter for RequestAddinShmemSpace is 'Size' not 'int'
>> 
>>  <programlisting>
>>  void RequestAddinShmemSpace(int size)
>>  </programlisting>
> 
> Hah, I think this mistake is nearly old enough to vote (e0dece1, 5f78aa5).
> Good catch.

I fixed this in v4.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  10. Re: reorganize "Shared Memory and LWLocks" section of docs

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-17T01:18:37Z

    On Tue, Jan 16, 2024 at 9:22 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> I fixed this in v4.

LGTM.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  11. Re: reorganize "Shared Memory and LWLocks" section of docs

    Nathan Bossart <nathandbossart@gmail.com> — 2024-01-19T17:22:06Z

    On Wed, Jan 17, 2024 at 06:48:37AM +0530, Bharath Rupireddy wrote:
> LGTM.

Committed.  Thanks for reviewing!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com