> On Mon, Jul 06, 2026 at 03:07:35PM -0700, Zsolt Parragi wrote:
> + if (errno == EAGAIN)
> + ereport(FATAL,
> + (errcode(ERRCODE_LOCK_FILE_EXISTS),
>
> According to fcntl.2, this should handle both EACCESS and EAGAIN:
>
> ERRORS
> EACCES or EAGAIN
> Operation is prohibited by locks held by other processes.
Good point. From what I see after a cursory look at fcntl is that it
normally returns EAGAIN, but filesystems are allowed to implement a
custom lock operation, so it makes sense to be prepared.
> +static int
> +OFDLockFile(int fd, const char *filename)
> +...
> + else
> + return dup(fd);
>
> Isn't this missing an FD_CLOEXEC, so that launched processes doesn't
> inherit it and keep the lock open possibly longer than needed?
This is an interesting question. I haven't thought about this
originally, but now I think the current approach (no FD_CLOEXEC) is what
is actually needed. We want to keep the lock as long as any existing
process may access the data directory, thus the lock lifetime must be
equal to the lifetime of a longest living process. Currently the lock
file is created by the bootstrap process and the postmaster, which I
think fits the picture.
> Also, shouldn't the code verify the result of dup? (!= -1 / errno)
From the functionality perspective it's not needed, but yeah, it will
make reporting better.
>
> + flock_fd = OFDLockFile(fd, filename);
>
> Can't we leak flock_fd in the stale path?
How, do you see any particular scenario?
> + elog(WARNING, "Failed locking file \"%s\", %m", filename);
>
> This probably should be:
>
> ereport(WARNING, (errcode_for_file_access(), errmsg("could not lock
> file \"%s\": %m", filename)))
I would concider this one of those "shouldn't be possible" errors, and
as such elog is more appropriate here.
