On Saturday, March 9, 2019 8:16 AM, Noah Misch <noah@leadboat.com> wrote:
This patch is not really in my wheelhouse, so I might very well be testing it
in the wrong way, but whats the fun in staying in shallow end. Below is my
attempt at reviewing this patchset.
Both patches applies with a little bit of fuzz, and passes check-world. No new
perlcritic warnings are introduced, but 016_shm.pl needs to be renamed to 017
since commit b0825d28ea83e44139bd319e6d1db2c499c claimed 016. They absolutely
seem to do what they on the tin, and to my understanding this seems like an
improvement we definitely want.
> 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.
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?
+ * 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?
> I tested on Red Hat and on Windows Server 2016; I won't be shocked
> if the test (not the code under test) breaks on other Windows configurations.
IIRC there are Windows versions where Win32::Process::KillProcess is required
for this, but thats admittedly somewhat dated knowledge. If the buildfarm goes
red on older Windows animals it might be something to look at perhaps.
Switching this to Ready for Committer since I can't see anything but tiny things.
cheers ./daniel
