Re: Cleaning up historical portability baggage - Mailing list 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