Thread: reorganize "Shared Memory and LWLocks" section of docs
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
Attachment
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
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
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
Attachment
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
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
Attachment
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
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
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
Attachment
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
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