On Fri, Mar 29, 2019 at 09:53:51AM +0000, Daniel Gustafsson wrote: > On Saturday, March 9, 2019 8:16 AM, Noah Misch <n...@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.
