Re: File descriptors in exec'd subprocesses - Mailing list pgsql-hackers

From Thomas Munro
Subject Re: File descriptors in exec'd subprocesses
Date
Msg-id CA+hUKGLuC9aZMMqD8PkuYm3fNoqCRqb90Az7Qn56iAtoTZTyDQ@mail.gmail.com
Whole thread Raw
In response to Re: File descriptors in exec'd subprocesses  (Thomas Munro <thomas.munro@gmail.com>)
Responses Re: File descriptors in exec'd subprocesses
List pgsql-hackers
Something bothered me about the previous versions:  Which layer should
add O_CLOEXEC, given that we have md.c -> PathNameOpenXXX() ->
BasicOpenFile() -> open()?  Previously I had md.c adding it, but on
reflection, it makes no sense to open a "File" (virtual file
descriptor) that is *not* O_CLOEXEC.  The client doesn't really know
when the raw descriptor is currently open and shouldn't really access
it; it would be strange to want it to survive a call to exec*().  I
think the 'highest' level API that we could consider requiring
O_CLOEXEC to be passed in explicitly, or not, is BasicOpenFile().
Done like that in this version.  This is the version I'm thinking of
committing, unless someone wants to argue for another level.

Another good choice would be to do it inside BasicOpenFile(), and then
the patch would be smaller again (xlog.c wouldn't need to mention it,
and there would perhaps be less risk that some long-lived descriptor
somewhere else has failed to request it), but perhaps that would be
presumptuous.  That function returns raw descriptors, and the caller,
perhaps an extension, might legitimately want to make an inheritable
descriptor for some reason, I guess?  Does anyone think I should move
it in there instead?

I realised that if we're going to use accept4() to cut down on
syscalls, we could also do the same for the postmaster pipe with
pipe2().

Here also is a tiny archeological cleanup to avoid creating
contradictory claims about whether all computers have O_CLOEXEC.

I toyed with the idea of a tiny Linux-only regression test using "COPY
fds FROM PROGRAM 'ls /proc/self/fd'" expecting 0, 1, 2, 3 (3 being
ls's opendir()), but that's probably a little too cute; and also
showed me that pg_regress.c leaks its log file, the fix for which is
to add "e" to its fdopen(), but that's another POSIX-next feature[1]
that seems a little harder to detect, and I gave up on that.

[1] https://wiki.freebsd.org/AtomicCloseOnExec

Attachment

pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Allow ordered partition scans in more cases
Next
From: Nathan Bossart
Date:
Subject: Re: Weird failure with latches in curculio on v15