Return value of PathNameOpenFile()
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t46559psql -h localhost -U postgresBuilt from patchset v4 (message #4), September 20, 2026 at 11:16 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t46559_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t46559_4 && git checkout t46559_4Patchset v4 (message #4) is on t46559_4
I've noticed that some callers of PathNameOpenFile()
(e.g. bbsink_server_begin_archive()) consider the call failed even if the
function returned zero, while other ones do check whether the file descriptor
is strictly negative. Since the file descriptor is actually returned by the
open() system call, I assume that zero is a valid result, isn't it?
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
On 6 Sep 2022, at 09:26, Antonin Houska <ah@cybertec.at> wrote:
I've noticed that some callers of PathNameOpenFile()
(e.g. bbsink_server_begin_archive()) consider the call failed even if the
function returned zero, while other ones do check whether the file descriptor
is strictly negative. Since the file descriptor is actually returned by the
open() system call, I assume that zero is a valid result, isn't it?
Agreed, zero should be valid as it's a non-negative integer. However, callers
in fd.c are themselves checking for (fd <= 0) in some cases, and some have done
so since the very early days of the codebase, so I wonder if there historically
used to be a platform which considered 0 an invalid fd?
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
Agreed, zero should be valid as it's a non-negative integer. However, callers
in fd.c are themselves checking for (fd <= 0) in some cases, and some have done
so since the very early days of the codebase, so I wonder if there historically
used to be a platform which considered 0 an invalid fd?
I'm betting it's a thinko that never got caught because 0 would
always be taken up by stdin. Maybe you'd notice if you tried to
close-and-reopen stdin, but that's not something the server ever does.
regards, tom lane
On 6 Sep 2022, at 16:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Agreed, zero should be valid as it's a non-negative integer. However, callers
in fd.c are themselves checking for (fd <= 0) in some cases, and some have done
so since the very early days of the codebase, so I wonder if there historically
used to be a platform which considered 0 an invalid fd?I'm betting it's a thinko that never got caught because 0 would
always be taken up by stdin. Maybe you'd notice if you tried to
close-and-reopen stdin, but that's not something the server ever does.
Doh, of course. The attached is a quick (lightly make check tested) take on
allowing 0, but I'm not sure that's what we want?
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
Doh, of course. The attached is a quick (lightly make check tested) take on
allowing 0, but I'm not sure that's what we want?
Actually, wait a second. At least some of these are not dealing
with kernel FDs but with our "virtual FD" abstraction. For those,
zero is indeed an invalid value. There might well be some "<= 0"
versus "< 0" errors in this area, but it requires closer inspection.
regards, tom lane
On 6 Sep 2022, at 21:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Doh, of course. The attached is a quick (lightly make check tested) take on
allowing 0, but I'm not sure that's what we want?Actually, wait a second. At least some of these are not dealing
with kernel FDs but with our "virtual FD" abstraction. For those,
zero is indeed an invalid value.
Yes and no, I think; PathNameOpenFile kind of returns a Vfd on success but a
kernel fd on failure which makes this a bit confusing. Abbreviated for space,
the code looks like this:
file = AllocateVfd();
vfdP = &VfdCache[file];
...
vfdP->fd = BasicOpenFilePerm(fileName, fileFlags, fileMode);
if (vfdP->fd < 0)
{
...
return -1;
}
...
return file;
So if the underlying BasicOpenFilePerm fails then open(2) failed and we return
-1, which is what open(2) returned. If it succeeds, then we return the Vfd
returned by AllocateVfd which can never be zero, as thats the VFD_CLOSED
ringbuffer anchor. Since AllocateVfd doesn't return on error, it's easy to
confuse oneself on exactly which error is propagated.
Checking for (fd <= 0) on an fd returned from PathNameOpenFile is thus not
wrong, albeit wearing both belts and suspenders since it can never return 0.
Changing them seem like codechurn for no reason even if the check for 0 is
superfluous. The callsites that only check for (fd < 0) are equally correct,
and need not be changed.
Callers of BasicOpenFile need however allow for zero since they get the kernel
fd back, which AFAICT from scanning that they all do.
So in summary, I can't spot a callsite which isn't safe.
--
Daniel Gustafsson https://vmware.com/