Horton

Re: Cleaning up historical portability baggage

Andres Freund <andres@anarazel.de>

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

Can we get a few more of these committed soon? It's all tests that I need to
sync with the meson stuff, and I'd rather get it over with :). And it reduces
the set of tests that need to be compared...  Or is there a blocker (leaving
the prairedog one aside)?


On 2022-08-03 14:25:01 +1200, Thomas Munro wrote:
> Subject: [PATCH v3 01/13] Remove configure probe for dlopen.
> Subject: [PATCH v3 02/13] Remove configure probe and extra tests for
>  getrlimit.

LGTM.


> From 96a4935ff9480c2786634e9892b1f44782b403fb Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas.munro@gmail.com>
> Date: Sat, 23 Jul 2022 23:49:27 +1200
> Subject: [PATCH v3 03/13] Remove configure probe for shm_open.
>
> shm_open() is in SUSv2 (realtime) and all targeted Unix systems have it.
>
> We retain a HAVE_SHM_OPEN macro, because it's clearer to readers than
> something like !defined(WIN32).

I don't like these. I don't find them clearer - if we really just assume this
to be the case on windows, it's easier to understand the checks if they talk
about windows rather than having to know whether this specific check just
applies to windows or potentially an unspecified separate set of systems.

But I guess I should complain upthread...


> Subject: [PATCH v3 04/13] Remove configure probe for setsid.

LGTM.



> Subject: [PATCH v3 05/13] Remove configure probes for symlink/readlink, and
>  dead code.

Nice win.

>

> From 143f6917bbc7d8f457d52d02a5fbc79d849744e1 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas.munro@gmail.com>
> Date: Sun, 24 Jul 2022 01:19:05 +1200

> Subject: [PATCH v3 06/13] Remove configure probe for link.
> --- a/src/include/port.h
> +++ b/src/include/port.h
> @@ -402,7 +402,8 @@ extern float pg_strtof(const char *nptr, char **endptr);
>  #define strtof(a,b) (pg_strtof((a),(b)))
>  #endif
>
> -#ifndef HAVE_LINK
> +#ifdef WIN32
> +/* src/port/win32link.c */
>  extern int	link(const char *src, const char *dst);
>  #endif

It bothers me that we have all this windows crap in port.h instead of
win32_port.h. But that's not this patch's fault.


> Subject: [PATCH v3 07/13] Remove dead replacement code for clock_gettime().

Nice.


> XXX This can only be committed once prairedog is decommissioned, because
> macOS 10.4 didn't have clock_gettime().

Maybe put it later in the queue?


> Subject: [PATCH v3 08/13] Remove configure probes for poll and poll.h.
>
> poll() and <poll.h> are in SUSv2 and all targeted Unix systems have
> them.
>
> Retain HAVE_POLL and HAVE_POLL_H macros for readability.  There's an
> error in latch.c that is now unreachable (since logically always have
> one of WIN32 or HAVE_POLL defined), but that falls out of a decision to
> keep using defined(HAVE_POLL) instead of !defined(WIN32) to guard the
> poll() code.

Wonder if we instead should add an empty poll.h to src/include/port/win32?


> Subject: [PATCH v3 09/13] Remove dead setenv, unsetenv replacement code.
> Subject: [PATCH v3 10/13] Remove dead pread and pwrite replacement code.

LGTM.


> Subject: [PATCH v3 11/13] Simplify replacement code for preadv and pwritev.
>
> preadv() and pwritev() are not standardized by POSIX.  Most targeted
> Unix systems have had them for more than a decade, since they are
> obvious combinations of standard p- and -v functions.
>
> In 15, we had two replacement implementations: one based on lseek() + -v
> function if available, and the other based on a loop over p- function.
> They aren't used for much yet, but are heavily used in a current
> proposal.
>
> Supporting two ways of falling back, at the cost of having a
> pg_preadv/pg_pwritev that could never be used in a multi-threaded
> program accessing the same file descriptor from two threads without
> unpleasant locking does not sound like a good trade.
>
> Therefore, drop the lseek()-based variant, and also the pg_ prefix, now
> that the file position portability hazard is gone.  Previously, both
> fallbacks had the file position portability hazard, because our
> pread()/pwrite() replacement had the same hazard, but that problem has
> been fixed for pread()/pwrite() by an earlier commit.  Now the way is
> clear to expunge the file position portability hazard of the
> lseek()-based variants too.
>
> At the time of writing, the following systems in our build farm lack
> native preadv/pwritev and thus use fallback code:
>
>  * Solaris (but not illumos)
>  * macOS before release 11.0
>  * Windows with Cygwin
>  * Windows native
>
> With this commit, all of the above systems will now use the *same*
> fallback code, the one that loops over pread()/pwrite() (which is
> translated to equivalent calls in Windows).  Previously, all but Windows
> native would use the readv()/writev()-based fallback that this commit
> removes.

Given that it's just solaris and old macOS that "benefited" from writev, just
using the "full" fallback there makes sense.


> Subject: [PATCH v3 12/13] Remove fdatasync configure probe.

> @@ -1928,7 +1925,6 @@ if test "$PORTNAME" = "win32"; then
>    AC_CHECK_FUNCS(_configthreadlocale)
>    AC_REPLACE_FUNCS(gettimeofday)
>    AC_LIBOBJ(dirmod)
> -  AC_LIBOBJ(fdatasync)
>    AC_LIBOBJ(getrusage)
>    AC_LIBOBJ(kill)
>    AC_LIBOBJ(open)
> @@ -1936,6 +1932,7 @@ if test "$PORTNAME" = "win32"; then
>    AC_LIBOBJ(win32dlopen)
>    AC_LIBOBJ(win32env)
>    AC_LIBOBJ(win32error)
> +  AC_LIBOBJ(win32fdatasync)
>    AC_LIBOBJ(win32link)
>    AC_LIBOBJ(win32ntdll)
>    AC_LIBOBJ(win32pread)

I like that we might get away from all those "generically" named libobjs that
are hardcoded to be used only on windows...



> From 5bda430998d502a7f8a6fe662a56b63ac374c925 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas.munro@gmail.com>
> Date: Fri, 26 Mar 2021 22:58:06 +1300
> Subject: [PATCH v3 13/13] Remove --disable-thread-safety.
>
> Threads are in SUSv2 and all targeted Unix systems have the option.
> There are no known Unix systems that don't choose to implement the
> threading option, and we're no longer testing such builds.
>
> Future work to improve our use of threads will be simplified by not
> having to cope with a no-threads build option.

Yeha.


>  AC_CHECK_HEADER(pthread.h, [], [AC_MSG_ERROR([
> -pthread.h not found;  use --disable-thread-safety to disable thread safety])])
> +pthread.h not found])])

Is this really needed after these changes?  We probably can't get away from
AX_PTHREAD just yet, but we should be able to rely on pthread.h?


> diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
> index 2a0d08d10f..a4ef016c06 100644
> --- a/src/include/pg_config.h.in
> +++ b/src/include/pg_config.h.in
> @@ -51,10 +51,6 @@
>  /* Define to 1 if you want National Language Support. (--enable-nls) */
>  #undef ENABLE_NLS
>
> -/* Define to 1 to build client libraries as thread-safe code.
> -   (--enable-thread-safety) */
> -#undef ENABLE_THREAD_SAFETY
> -

Might be worth grepping around whether there are extensions that reference
ENABLE_THREAD_SAFETY (e.g. to then error out if not available). If common
enough we could decide to keep it, given that it's pretty reasonable for code
to want to know that across versions?


>  # Add libraries that libpq depends (or might depend) on into the
> diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
> index 49a1c626f6..3504ab2c34 100644
> --- a/src/interfaces/libpq/fe-auth.c
> +++ b/src/interfaces/libpq/fe-auth.c
> @@ -1116,11 +1116,10 @@ pg_fe_getusername(uid_t user_id, PQExpBuffer errorMessage)
>  #endif
>
>  	/*
> -	 * Some users are using configure --enable-thread-safety-force, so we
> -	 * might as well do the locking within our library to protect getpwuid().
> -	 * In fact, application developers can use getpwuid() in their application
> -	 * if they use the locking call we provide, or install their own locking
> -	 * function using PQregisterThreadLock().
> +	 * We do the locking within our library to protect getpwuid().  Application
> +	 * developers can use getpwuid() in their application if they use the
> +	 * locking call we provide, or install their own locking function using
> +	 * PQregisterThreadLock().
>  	 */
>  	pglock_thread();

Probably worth using getpwuid_r where available - might even be everywhere
(except windows of course, but GetUserName() is threadsafe).

> --- a/src/port/getaddrinfo.c
> +++ b/src/port/getaddrinfo.c
> @@ -414,7 +414,7 @@ pqGethostbyname(const char *name,
>  				struct hostent **result,
>  				int *herrno)
>  {
> -#if defined(ENABLE_THREAD_SAFETY) && defined(HAVE_GETHOSTBYNAME_R)
> +#if defined(HAVE_GETHOSTBYNAME_R)
>  	/*
>  	 * broken (well early POSIX draft) gethostbyname_r() which returns 'struct

Depressingly there's still plenty systems without gethostbyname_r() :(

curculio, gombessa, longfin, lorikeet, morepork, pollock, sifaka, wrasse ...



Greetings,

Andres Freund

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)