Horton

Re: Cleaning up historical portability baggage

Thomas Munro <thomas.munro@gmail.com>

View thread View on postgr.es
From: Thomas Munro <thomas.munro@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrew Dunstan <andrew@dunslane.net>, Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-08-02T00:18:07Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Make sure IOV_MAX is defined.

  2. Doc: Abstract AF_UNIX sockets don't work on Windows.

  3. Clean up inconsistent use of fflush().

  4. Remove configure probe for sockaddr_in6 and require AF_INET6.

  5. Remove configure probes for sockaddr_storage members.

  6. Remove configure probe for netinet/tcp.h.

  7. Fix macro problem with gai_strerror on Windows.

  8. Remove configure probe for net/if.h.

  9. mstcpip.h is not missing on MinGW.

  10. Remove dead ifaddr.c fallback code.

  11. Remove configure probe for gethostbyname_r.

  12. Remove replacement code for getaddrinfo.

  13. Remove configure probe for struct sockaddr_storage.

  14. Remove HAVE_UNIX_SOCKETS.

  15. Remove configurability of PPC spinlock assembly code.

  16. Remove configure probe for shl_load library.

  17. Remove configure probe for sys/select.h.

  18. Remove configure probes for sys/un.h and struct sockaddr_un.

  19. Remove configure probes for sys/ipc.h, sys/sem.h, sys/shm.h.

  20. Remove configure probe for sys/resource.h and refactor.

  21. Remove configure probe for sys/uio.h.

  22. Fix obsolete comments in instr_time.h.

  23. aix: fix misreading of condition in 8f12a4e7add

  24. aix: Remove checks for very old OS versions

  25. windows: Remove HAVE_MINIDUMP_TYPE test

  26. solaris: Remove unnecessary gcc / gnu ld vs sun studio differences

  27. Trust a few system headers to stand on their own

  28. Rely on __func__ being supported

  29. Simplify replacement code for strtof.

  30. Simplify gettimeofday for Windows.

  31. Simplify gettimeofday() fallback logic.

  32. Remove fallbacks for strtoll, strtoull.

  33. Remove configure probe for fdatasync.

  34. Remove configure probe for clock_gettime.

  35. Simplify replacement code for preadv and pwritev.

  36. Remove dead pread and pwrite replacement code.

  37. Remove dead setenv, unsetenv replacement code.

  38. Remove configure probes for poll and poll.h.

  39. Remove configure probe for link.

  40. Remove configure probes for symlink/readlink, and dead code.

  41. Remove configure probe for setsid.

  42. Remove configure probe for shm_open.

  43. Remove configure probe and related tests for getrlimit.

  44. Remove configure probe for dlopen, and refactor.

  45. Remove dead getpwuid_r replacement code.

  46. Remove dead handling for pre-POSIX sigwait().

  47. Remove dead getrusage replacement code.

  48. Cygwin and Mingw floating-point fixes.

  49. Support platforms where strtoll/strtoull are spelled __strtoll/__strtoull.

  50. Arrange to supply declarations for strtoll/strtoull if needed.

  51. Lock down regression testing temporary clusters on Windows.

  52. Secure Unix-domain sockets of "make check" temporary clusters.

  53. Add configure checks for strtoll, strtoull (or strto[u]q). Disable

  54. Portability fix for old SunOS releases: fflush(NULL)