On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote:
> On Saturday, March 9, 2019 8:16 AM, Noah Misch <noah@leadboat.com> wrote:
> > I renamed IpcMemoryAnalyze() to PGSharedMemoryAttach() and deleted the old
> > function of that name. Now, this function never calls shmdt(); the caller is
> > responsible for that. I do like things better this way. What do you think?
>
> I think it makes for a good API for the caller to be responsible, but it does
> warrant a comment on the function to explicitly state that.
The name "PGSharedMemoryAttach" makes that fact sufficiently obvious, I think.
> A few other small comments:
>
> + state = PGSharedMemoryAttach((IpcMemoryId) id2, &memAddress);
> + if (memAddress)
> + shmdt(memAddress);
>
> This seems like a case where it would be useful to log a shmdt() error or do
> an Assert() around the success of the operation perhaps?
I'll add the same elog(LOG) we have at other shmdt() sites. I can't think of
a site where we Assert() about the results of a system call. While shmdt()
might be a justified exception, elog(LOG) seems reasonable.
> + * Loop till we find a free IPC key. Trust CreateDataDirLockFile() to
> + * ensure no more than one postmaster per data directory can enter this
> + * loop simultaneously. (CreateDataDirLockFile() does not ensure that,
> + * but prefer fixing it over coping here.)
>
> This comment make it seem like there is a fix to CreateLockFile() missing to
> his patch, is that correct? If so, do you have an idea for that patch?
That comment refers to
https://postgr.es/m/flat/20120803145635.GE9683%40tornado.leadboat.com
> Switching this to Ready for Committer since I can't see anything but tiny things.
Thanks.