The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, passed Spec compliant: not tested Documentation: not tested
Hi, I looked at v2, focusing on apply/build status and the PID namespace scenario described in the cover letter. I used the v2 patch from Dmitry's 2026-01-17 message, on origin/master at 4cb2a9863d89b320f37eb1bd76822f6f65e59311. The patch applies cleanly with git am, and git diff --check reports no issues. I built locally with: ./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu make -s -j8 make -s install The build completed successfully. Configure found F_OFD_SETLK on this macOS host too. I also built and tested on Linux with the same configure options: make -s -j3 make -s install There, configure also found F_OFD_SETLK, and: make -C src/test/regress check passed; all regression tests passed. For the namespace behavior, I used two PostgreSQL builds on the same Linux host: unpatched master and v2. The test starts the first postmaster in one pid/ipc/net namespace, then tries to start a second postmaster in another pid/ipc/net namespace on the same data directory. On unpatched master, both postmasters started. Both saw themselves as PID 2 in their own PID namespace, and the second postmaster rewrote postmaster.pid. After the second startup, the lock file contained the second socket directory: 2 .../pidns-base/data 1780661958 65437 .../pidns-base/sock2 With v2, the first postmaster started, and the second startup failed with: FATAL: cannot lock the lock file "postmaster.pid" HINT: Another server is starting. So the patch fixes the concrete Linux PID namespace failure mode I tested. One behavior I wanted to ask about: v2 also OFD-locks Unix socket lock files, not only the data directory lockfile. That follows from calling FlockDataDirLockFile() from the generic CreateLockFile() path, and I also saw it at runtime. With one v2 postmaster using a Unix socket directory, /proc/locks showed OFD write locks for both files: .../data/postmaster.pid .../sock/.s.PGSQL.65436.lock The corresponding postmaster fds were: /proc/835058/fd/5 -> .../data/postmaster.pid /proc/835058/fd/8 -> .../sock/.s.PGSQL.65436.lock I had expected the new lock to apply only to postmaster.pid, because the patch title, commit message, helper name, and comments all describe the data directory lockfile. The socket lockfile behavior therefore looked like either an intentional scope expansion that should be named as such, or an accidental side effect of using the generic CreateLockFile() path. Is locking the socket lock file intentional here? If so, maybe the helper name, comment, and fd tracking should reflect that broader scope. If not, perhaps the new lock should be applied only for the isDDLock case; otherwise v2 changes socket lockfile semantics too, not only postmaster.pid. While reading that code, I also noticed a small error-path issue: DataDirLockFD starts as 0, but if fcntl(F_OFD_SETLK) fails with a non-EAGAIN error the code only emits a warning and does not assign a duplicate fd. UnlinkLockFiles() then closes DataDirLockFD unconditionally. Initializing it to -1, closing it conditionally, and checking dup(fd) would make that path more explicit. Aside from those questions, this looks like a useful best-effort improvement to me, and the namespace failure mode appears to be addressed by the OFD lock in the Linux test above. I did not test NFS behavior, older stable branches, Windows behavior, or installcheck-world. Unprivileged namespace creation was not permitted on the Linux host I used, so the namespace repro was run with sudo. Regards, Ilmar Yunusov ## v3 follow-up draft Suggested CommitFest form: - In response to: Dmitry's 2026-06-23 v3 message - make installcheck-world: not tested - Implements feature: tested, passed - Spec compliant: not tested - Documentation: not tested - New status: Waiting on Author - Attachments: none Subject: Re: File locks for data directory lockfile in the context of Linux namespaces Body: Hi, I looked at v3, focusing on whether it addresses the two points from my previous review, the Linux runtime behavior, and the current CFBot failures. I used the v3 patch from Dmitry's 2026-06-23 message, on origin/master at 9d1188f29865e66c4196578501e74e8c815fba8d. The patch applies cleanly with git am, and git diff --check reports no issues. On Ubuntu 24.04.4, configure found F_OFD_SETLK, and this passed: ./configure --prefix=/home/master/pgcf-6335-v3-20260706/pg-install --without-readline --without-zlib --without-icu make -s -j3 make -s install make -C src/test/regress check All 245 regression tests passed. I re-ran the PID namespace reproducer from the earlier review against v3. The first postmaster started in one pid/ipc/net namespace on the test data directory. A second postmaster in another pid/ipc/net namespace on the same data directory failed with: FATAL: cannot lock the lock file "postmaster.pid" HINT: Another server is starting. So the concrete Linux PID namespace failure mode I tested still looks addressed in v3. I also checked the lockfile scope at runtime. With one v3 postmaster using a Unix socket directory, /proc/<pid>/fd showed fds for both files: .../data/postmaster.pid .../sock/.s.PGSQL.65442.lock /proc/locks also showed OFDLCK ADVISORY WRITE entries for both file inodes. That matches the v3 scope where socket lockfiles are intentionally locked too. The other question from my previous review also looks addressed: the single static DataDirLockFD issue is replaced by per-lockfile fd tracking in the lock_files list, with conditional close in UnlinkLockFiles(). There is still a build blocker, matching the current CFBot failures. v3 adds a typedef named LockFile in src/backend/utils/init/miscinit.c, but that name conflicts with the Windows LockFile() function. The CompilerWarnings log reports, for example: miscinit.c:79:3: error: 'LockFile' redeclared as different kind of symbol and the Windows logs show the same issue: miscinit.c(79): error C2365: 'LockFile': redefinition; previous definition was 'function' There is also a missing semicolon in the non-F_OFD_SETLK branch: miscinit.c:1169:18: error: expected ';' before '}' token As a quick local compile check, in the temporary worktree only, renaming the new typedef to LockFileData and adding the missing semicolon made the normal macOS build pass. With HAVE_DECL_F_OFD_SETLK temporarily forced to 0 in the generated pg_config.h, the affected miscinit.o target also compiled after those two mechanical changes. I did not run a local Windows build. I did not run installcheck-world, and did not test NFS behavior or stable-branch backpatch applicability. Regards, Ilmar Yunusov The new status of this patch is: Waiting on Author
