Re: Safeguards against incorrect fd flags for fsync()

On Tue, Jun 10, 2025 at 12:26:48PM +0200, Michael Banck wrote:
> Well O_RDONLY might historically be 0 almost everywhere, but it is
> defined to 1 on the GNU system [1]:
>
> |#define O_RDONLY 0x0001 /* Open read-only. */
>
> So there, the comparison with 0 does not work and initdb (at least)
> fails on assert-enabled builds:
>
> |running bootstrap script ... TRAP: FailedAssertion("(desc_flags & (O_RDWR | O_WRONLY)) == 0", File: "fd.c", Line: 395, PID: 4560)
>
> TTBOMK, POSIX does not mandate that O_RDONLY be 0, so I think this check
> is overly zealous. The better way might be to mask the flags with
> O_ACCMODE and then just check what you want, like in the attached.

Okay, seems sensible.

- /*
- * O_RDONLY is historically 0, so just make sure that for directories
- * no write flags are used.
- */
+ desc_flags &= O_ACCMODE;
+
if (S_ISDIR(st.st_mode))
- Assert((desc_flags & (O_RDWR | O_WRONLY)) == 0);
+ Assert(desc_flags == O_RDONLY);
else
- Assert((desc_flags & (O_RDWR | O_WRONLY)) != 0);
+ Assert(desc_flags != O_RDONLY);
}
errno = 0;
#endif

We don't have a trace of O_ACCMODE in the tree, and POSIX defines it.
I'm wondering how the buildfarm would react on that, but perhaps
that's fine on !WIN32. It's hard to say with all the hosts there, at
least the CI is OK.

Another thing that may be worth considering is if we should remove
this sanity check. Still, that seems useful to catch things like [1],
even now, but that was mainly because I was too stupid with my flag
manipulations. The test coverage is much better than it was 6 years
ago, and we have the CI working as a first layer of checks so it seems
less useful now as long as fsync=on is forced in at least one host.
I'd rather keep it for the fsync=off cases, though, because that's
what the large majority of the regression test runs use.

Are you working on setting up a buildfarm member to support this
configuration? In the 6 years since 12198239c0a5 is in the tree,
we've been kind of OK with the current check.

[1]: https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org
--
Michael