Horton

Re: [HACKERS] Weaker shmem interlock w/o postmaster.pid

Daniel Gustafsson <daniel@yesql.se>

View thread View on postgr.es
From: Daniel Gustafsson <daniel@yesql.se>
To: Noah Misch <noah@leadboat.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>, "9erthalion6@gmail.com" <9erthalion6@gmail.com>, "sfrost@snowman.net" <sfrost@snowman.net>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2019-03-29T09:53:51Z
Lists: pgsql-hackers 
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

Commits

  1. Probe only 127.0.0.1 when looking for ports on Unix.

  2. Don't write to stdin of a test process that could have already exited.

  3. Test both 0.0.0.0 and 127.0.0.x addresses to find a usable port.

  4. MSYS: Translate REGRESS_SHLIB to a Windows file name.

  5. MSYS: Skip src/test/recovery/t/017_shm.pl.

  6. When Perl "kill(9, ...)" fails, try "pg_ctl kill".

  7. Consistently test for in-use shared memory.

  8. Revert "Consistently test for in-use shared memory."

  9. Silence -Wimplicit-fallthrough in sysv_shmem.c.

  10. Make src/test/recovery/t/017_shm.pl safe for concurrent execution.

  11. Update HINT for pre-existing shared memory block.

  12. Add WL_EXIT_ON_PM_DEATH pseudo-event.

  13. The default values for shared_buffers and max_connections are now 1000

  14. XLOG (and related) changes:

  15. Significant cleanups in SysV IPC handling (shared mem and semaphores).