On Sun, Jul 24, 2022 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Here are some more, a couple of which I posted before but I've now
> > gone a bit further with them in terms of removing configure checks
> > etc:
>
> After looking through these briefly, I'm pretty concerned about
> whether this won't break our Cygwin build in significant ways.
> For example, lorikeet reports "HAVE_SETSID 1", a condition that
> you want to replace with !WIN32. The question here is whether
> or not WIN32 is defined in a Cygwin build. ...
No, it should not be unless someone screws up and leaks <windows.h>
into a header when WIN32 isn't already defined. I've done some
analysis and testing of that, and proposed to nail it down a bit and
remove the confusion created by the inconsistent macro tests, over at
[1].
> More generally, I'm not exactly convinced that changes like
> this are a readability improvement:
>
> -#ifdef HAVE_SETSID
> +#ifndef WIN32
>
> I'd rather not have the code cluttered with a sea of
> indistinguishable "#ifndef WIN32" tests when some of them could be
> more specific and more mnemonic. So I think we'd be better off
> leaving that as-is. I don't mind nuking the configure-time test
> and hard-wiring "#define HAVE_SETSID 1" somewhere, but changing
> the code's #if tests doesn't seem to bring any advantage.
OK, in this version of the patch series I did this:
1. If it's something that only Unix has, and for Windows we do
nothing or skip a feature, then I've now hard-wired the macro as you
suggested. I put that in port.h. I agree that's a little easier to
grok than no-context !defined(WIN32). Examples: HAVE_SETSID,
HAVE_SHM_OPEN.
2. If it's something that Unix has and we supply a Windows
replacements, and we just can't cope without that function, then I
didn't bother with a vestigial macro. There generally weren't tests
for such things already (mostly stuff auto-generated by
AC_REPLACE_FUNCS). Example: HAVE_LINK.
3. In the special case of symlink() and readlink(), I defined the
macros on Unix even though we also have replacements on Windows.
(Previously we effectively did that for one but not the other...) My
idea here is that, wherever we are OK using our pseudo-symlinks made
from junction points (ie for tablespaces), then we should just go
ahead and use them without testing. But in just a couple of places
where fully compliant symlinks are clearly expected (ie non-directory
or relative path, eg tz file or executable symlinks), then the tests
can still be used. See also commit message. Does this make sense?
(I also propose to supply S_ISLNK and lstat() for Windows and make
usage of that stuff unconditional, but I put that in another
thread[2], as that's new code, and this thread is just about ripping
old dead stuff out.)
4. If it's something that already had very obvious Windows and Unix
code paths, then I didn't bother with a HAVE_XXX macro, because I
think it'd be more confusing than just #ifdef WIN32 ...windows stuff
... #else ...unix stuff... #endif. Example: HAVE_CLOCK_GETTIME.
> Specific to 0001, I don't especially like what you did to
> src/port/dlopen.c. The original intent (and reality until
> not so long ago) was that that would be a container for
> various dlopen replacements. Well, okay, maybe there will
> never be any more besides Windows, but I think then we should
> either rename the file to (say) win32dlopen.c or move it to
> src/backend/port/win32. Likewise for link.c in 0007 and
> pread.c et al in 0011. (But 0010 is fine, because the
> replacement code is already handled that way.)
Agreed on the file names win32dlopen.c, win32link.c, win32pread.c,
win32pwrite.c, and done.
Another characteristic of other Windows-only replacement code is that
it's called pgwin32_THING and then a macro replaces THING() with
pgwin32_THING(). I guess I should do that too, for consistency, and
move relevant declarations into win32_port.h? Done.
There are clearly many other candidates for X.c -> win32X.c renamings
by the same only-for-Windows argument, but I haven't touched those (at
least dirent.c, dirmod.c, gettimeofday.c, kill.c, open.c, system.c).
I'll also include the fdatasync configure change here (moved from
another thread). Now it also renames fdatasync.c -> win32datasync.c.
Erm, but I didn't add the pgwin32_ prefix to the function name,
because it shares a function declaration with macOS in c.h.
> OTOH, 0012 seems to immediately change pread.c et al back
> to NOT being Windows-only, though it's hard to tell for
> sure because the configure support seems all wrong.
> I'm quite confused by those two patches ... are they really
> correct?
The 0012 patch (now 0011 in v2) is about the variants with -v on the
end. The patches are as I intended. I've now put a longer
explanation into the commit message, but here's a short recap:
pread()/pwrite() replacements (without 'v' for vector) are now needed
only by Windows. HP-UX < 11 was the last Unix system I knew of
without these functions. That makes sense, as I think they were
related to the final POSIX threading push (multi-threaded programs
want to be able to skip file position side-effects), which HP-UX 10.x
predated slightly. Gaur's retirement unblocked this cleanup.
preadv()/pwritev() replacements (with 'v' for vector) are needed by
Solaris, macOS < 11 and Windows, and will likely be required for a
long time, because these functions still haven't been standardised.
My goal is to make our replacement code side-effect free, thread-safe,
in line with the de facto standard/convention seen on Linux, *BSD,
macOS, AIX, illumos.
Note that I have some better vector I/O code for Windows to propose a
bit later, so the real effect of this choice will be to drop true
vector I/O on Solaris, until such time as they get around to providing
the modern interface that almost every other Unix managed to agree on.
[1]
https://www.postgresql.org/message-id/flat/CA%2BhUKG%2Be13wK0PBX5Z63CCwWm7MfRQuwBRabM_3aKWSko2AUww%40mail.gmail.com
[2] https://www.postgresql.org/message-id/flat/CA+hUKGLfOOeyZpm5ByVcAt7x5Pn-=xGRNCvgiUPVVzjFLtnY0w@mail.gmail.com
