Thread: [PATCH v3 1/1] Fix detection of preadv/pwritev support for OSX.

Fixes:
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -O2
-I../../../../src/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   -c -o fd.o fd.c
 
fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
                part = pg_pwritev(fd, iov, iovcnt, offset);
                       ^~~~~~~~~~
../../../../src/include/port/pg_iovec.h:49:20: note: expanded from macro 'pg_pwritev'
                   ^~~~~~~

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk/usr/include/sys/uio.h:104:9:
note:'pwritev' has been marked as being introduced in macOS 11.0
 
      here, but the deployment target is macOS 10.15.0
ssize_t pwritev(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0),
ios(14.0),watchos(7.0), tvos(14.0));
 
        ^
fd.c:3661:10: note: enclose 'pwritev' in a __builtin_available check to silence this warning
                part = pg_pwritev(fd, iov, iovcnt, offset);
                       ^~~~~~~~~~
../../../../src/include/port/pg_iovec.h:49:20: note: expanded from macro 'pg_pwritev'
                   ^~~~~~~
1 warning generated.

This results in a runtime error:
running bootstrap script ... dyld: lazy symbol binding failed: Symbol not found: _pwritev
  Referenced from: /usr/local/pgsql/bin/postgres
  Expected in: /usr/lib/libSystem.B.dylib

dyld: Symbol not found: _pwritev
  Referenced from: /usr/local/pgsql/bin/postgres
  Expected in: /usr/lib/libSystem.B.dylib

child process was terminated by signal 6: Abort trap: 6

To fix this we set -Werror=unguarded-availability-new so that a declaration
check for preadv/pwritev will fail if the symbol is unavailable on the requested
SDK version.
---
Changes v2 -> v3:
  - Replace compile check with AC_CHECK_DECLS
  - Fix preadv detection as well
Changes v1 -> v2:
  - Add AC_LIBOBJ(pwritev) when pwritev not available
  - set -Werror=unguarded-availability-new for CXX flags as well
---
 configure                   | 164 ++++++++++++++++++++++++++++++------
 configure.ac                |   9 +-
 src/include/pg_config.h.in  |  14 +--
 src/include/port/pg_iovec.h |   4 +-
 src/tools/msvc/Solution.pm  |   4 +-
 5 files changed, 157 insertions(+), 38 deletions(-)

diff --git a/configure b/configure
index 8af4b99021..07a9b08d80 100755
--- a/configure
+++ b/configure
@@ -5373,6 +5373,98 @@ if test x"$pgac_cv_prog_CC_cflags__Werror_vla" = x"yes"; then
 fi
 
 
+  # Prevent usage of symbols marked as newer than our target.
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Werror=unguarded-availability-new, for
CFLAGS">&5
 
+$as_echo_n "checking whether ${CC} supports -Werror=unguarded-availability-new, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Werror=unguarded-availability-new"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new=yes
+else
+  pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" = x"yes"; then
+  CFLAGS="${CFLAGS} -Werror=unguarded-availability-new"
+fi
+
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -Werror=unguarded-availability-new, for
CXXFLAGS">&5
 
+$as_echo_n "checking whether ${CXX} supports -Werror=unguarded-availability-new, for CXXFLAGS... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS} -Werror=unguarded-availability-new"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new=yes
+else
+  pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" = x"yes"; then
+  CXXFLAGS="${CXXFLAGS} -Werror=unguarded-availability-new"
+fi
+
+
   # -Wvla is not applicable for C++
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wendif-labels, for CFLAGS" >&5
@@ -15646,6 +15738,52 @@ cat >>confdefs.h <<_ACEOF
 _ACEOF
 
 
+# AC_REPLACE_FUNCS does not respect the deployment target on macOS
+ac_fn_c_check_decl "$LINENO" "preadv" "ac_cv_have_decl_preadv" "#include <sys/uio.h>
+"
+if test "x$ac_cv_have_decl_preadv" = xyes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_PREADV $ac_have_decl
+_ACEOF
+if test $ac_have_decl = 1; then :
+
+else
+  case " $LIBOBJS " in
+  *" preadv.$ac_objext "* ) ;;
+  *) LIBOBJS="$LIBOBJS preadv.$ac_objext"
+ ;;
+esac
+
+fi
+
+ac_fn_c_check_decl "$LINENO" "pwritev" "ac_cv_have_decl_pwritev" "#include <sys/uio.h>
+"
+if test "x$ac_cv_have_decl_pwritev" = xyes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_PWRITEV $ac_have_decl
+_ACEOF
+if test $ac_have_decl = 1; then :
+
+else
+  case " $LIBOBJS " in
+  *" pwritev.$ac_objext "* ) ;;
+  *) LIBOBJS="$LIBOBJS pwritev.$ac_objext"
+ ;;
+esac
+
+fi
+
+
 ac_fn_c_check_decl "$LINENO" "RTLD_GLOBAL" "ac_cv_have_decl_RTLD_GLOBAL" "#include <dlfcn.h>
 "
 if test "x$ac_cv_have_decl_RTLD_GLOBAL" = xyes; then :
@@ -15845,19 +15983,6 @@ esac
 
 fi
 
-ac_fn_c_check_func "$LINENO" "preadv" "ac_cv_func_preadv"
-if test "x$ac_cv_func_preadv" = xyes; then :
-  $as_echo "#define HAVE_PREADV 1" >>confdefs.h
-
-else
-  case " $LIBOBJS " in
-  *" preadv.$ac_objext "* ) ;;
-  *) LIBOBJS="$LIBOBJS preadv.$ac_objext"
- ;;
-esac
-
-fi
-
 ac_fn_c_check_func "$LINENO" "pwrite" "ac_cv_func_pwrite"
 if test "x$ac_cv_func_pwrite" = xyes; then :
   $as_echo "#define HAVE_PWRITE 1" >>confdefs.h
@@ -15871,19 +15996,6 @@ esac
 
 fi
 
-ac_fn_c_check_func "$LINENO" "pwritev" "ac_cv_func_pwritev"
-if test "x$ac_cv_func_pwritev" = xyes; then :
-  $as_echo "#define HAVE_PWRITEV 1" >>confdefs.h
-
-else
-  case " $LIBOBJS " in
-  *" pwritev.$ac_objext "* ) ;;
-  *) LIBOBJS="$LIBOBJS pwritev.$ac_objext"
- ;;
-esac
-
-fi
-
 ac_fn_c_check_func "$LINENO" "random" "ac_cv_func_random"
 if test "x$ac_cv_func_random" = xyes; then :
   $as_echo "#define HAVE_RANDOM 1" >>confdefs.h
diff --git a/configure.ac b/configure.ac
index 868a94c9ba..0cd1ee8909 100644
--- a/configure.ac
+++ b/configure.ac
@@ -494,6 +494,9 @@ if test "$GCC" = yes -a "$ICC" = no; then
   AC_SUBST(PERMIT_DECLARATION_AFTER_STATEMENT)
   # Really don't want VLAs to be used in our dialect of C
   PGAC_PROG_CC_CFLAGS_OPT([-Werror=vla])
+  # Prevent usage of symbols marked as newer than our target.
+  PGAC_PROG_CC_CFLAGS_OPT([-Werror=unguarded-availability-new])
+  PGAC_PROG_CXX_CFLAGS_OPT([-Werror=unguarded-availability-new])
   # -Wvla is not applicable for C++
   PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
   PGAC_PROG_CXX_CFLAGS_OPT([-Wendif-labels])
@@ -1705,6 +1708,10 @@ AC_CHECK_DECLS([strlcat, strlcpy, strnlen])
 # This is probably only present on macOS, but may as well check always
 AC_CHECK_DECLS(F_FULLFSYNC, [], [], [#include <fcntl.h>])
 
+# AC_REPLACE_FUNCS does not respect the deployment target on macOS
+AC_CHECK_DECLS([preadv], [], [AC_LIBOBJ(preadv)], [#include <sys/uio.h>])
+AC_CHECK_DECLS([pwritev], [], [AC_LIBOBJ(pwritev)], [#include <sys/uio.h>])
+
 AC_CHECK_DECLS([RTLD_GLOBAL, RTLD_NOW], [], [], [#include <dlfcn.h>])
 
 AC_CHECK_TYPE([struct sockaddr_in6],
@@ -1737,9 +1744,7 @@ AC_REPLACE_FUNCS(m4_normalize([
     link
     mkdtemp
     pread
-    preadv
     pwrite
-    pwritev
     random
     srandom
     strlcat
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index f4d9f3b408..9c30d008c9 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -142,6 +142,14 @@
    don't. */
 #undef HAVE_DECL_POSIX_FADVISE
 
+/* Define to 1 if you have the declaration of `preadv', and to 0 if you don't.
+   */
+#undef HAVE_DECL_PREADV
+
+/* Define to 1 if you have the declaration of `pwritev', and to 0 if you
+   don't. */
+#undef HAVE_DECL_PWRITEV
+
 /* Define to 1 if you have the declaration of `RTLD_GLOBAL', and to 0 if you
    don't. */
 #undef HAVE_DECL_RTLD_GLOBAL
@@ -412,9 +420,6 @@
 /* Define to 1 if you have the `pread' function. */
 #undef HAVE_PREAD
 
-/* Define to 1 if you have the `preadv' function. */
-#undef HAVE_PREADV
-
 /* Define to 1 if you have the `pstat' function. */
 #undef HAVE_PSTAT
 
@@ -433,9 +438,6 @@
 /* Define to 1 if you have the `pwrite' function. */
 #undef HAVE_PWRITE
 
-/* Define to 1 if you have the `pwritev' function. */
-#undef HAVE_PWRITEV
-
 /* Define to 1 if you have the `random' function. */
 #undef HAVE_RANDOM
 
diff --git a/src/include/port/pg_iovec.h b/src/include/port/pg_iovec.h
index 365d605a9b..760ec4980c 100644
--- a/src/include/port/pg_iovec.h
+++ b/src/include/port/pg_iovec.h
@@ -39,13 +39,13 @@ struct iovec
 /* Define a reasonable maximum that is safe to use on the stack. */
 #define PG_IOV_MAX Min(IOV_MAX, 32)
 
-#ifdef HAVE_PREADV
+#if HAVE_DECL_PREADV
 #define pg_preadv preadv
 #else
 extern ssize_t pg_preadv(int fd, const struct iovec *iov, int iovcnt, off_t offset);
 #endif
 
-#ifdef HAVE_PWRITEV
+#if HAVE_DECL_PWRITEV
 #define pg_pwritev pwritev
 #else
 extern ssize_t pg_pwritev(int fd, const struct iovec *iov, int iovcnt, off_t offset);
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 2f28de0355..7c235f227d 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -329,14 +329,14 @@ sub GenerateFiles
         HAVE_PPC_LWARX_MUTEX_HINT   => undef,
         HAVE_PPOLL                  => undef,
         HAVE_PREAD                  => undef,
-        HAVE_PREADV                 => undef,
+        HAVE_DECL_PREADV            => 0,
         HAVE_PSTAT                  => undef,
         HAVE_PS_STRINGS             => undef,
         HAVE_PTHREAD                => undef,
         HAVE_PTHREAD_IS_THREADED_NP => undef,
         HAVE_PTHREAD_PRIO_INHERIT   => undef,
         HAVE_PWRITE                 => undef,
-        HAVE_PWRITEV                => undef,
+        HAVE_DECL_PWRITEV           => 0,
         HAVE_RANDOM                 => undef,
         HAVE_READLINE_H             => undef,
         HAVE_READLINE_HISTORY_H     => undef,
-- 
2.30.0

On Fri, Jan 22, 2021 at 12:32 PM James Hilliard
<james.hilliard1@gmail.com> wrote:
>
> Fixes:
> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -O2
-I../../../../src/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   -c -o fd.o fd.c 
> fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
>                 part = pg_pwritev(fd, iov, iovcnt, offset);
>                        ^~~~~~~~~~
> ../../../../src/include/port/pg_iovec.h:49:20: note: expanded from macro 'pg_pwritev'
>                    ^~~~~~~
>
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk/usr/include/sys/uio.h:104:9:
note:'pwritev' has been marked as being introduced in macOS 11.0 
>       here, but the deployment target is macOS 10.15.0
> ssize_t pwritev(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0),
ios(14.0),watchos(7.0), tvos(14.0)); 
>         ^
> fd.c:3661:10: note: enclose 'pwritev' in a __builtin_available check to silence this warning
>                 part = pg_pwritev(fd, iov, iovcnt, offset);
>                        ^~~~~~~~~~
> ../../../../src/include/port/pg_iovec.h:49:20: note: expanded from macro 'pg_pwritev'
>                    ^~~~~~~
> 1 warning generated.
>
> This results in a runtime error:
> running bootstrap script ... dyld: lazy symbol binding failed: Symbol not found: _pwritev
>   Referenced from: /usr/local/pgsql/bin/postgres
>   Expected in: /usr/lib/libSystem.B.dylib
>
> dyld: Symbol not found: _pwritev
>   Referenced from: /usr/local/pgsql/bin/postgres
>   Expected in: /usr/lib/libSystem.B.dylib
>
> child process was terminated by signal 6: Abort trap: 6
>
> To fix this we set -Werror=unguarded-availability-new so that a declaration
> check for preadv/pwritev will fail if the symbol is unavailable on the requested
> SDK version.
> ---
> Changes v2 -> v3:
>   - Replace compile check with AC_CHECK_DECLS
>   - Fix preadv detection as well
> Changes v1 -> v2:
>   - Add AC_LIBOBJ(pwritev) when pwritev not available
>   - set -Werror=unguarded-availability-new for CXX flags as well
> ---
>  configure                   | 164 ++++++++++++++++++++++++++++++------
>  configure.ac                |   9 +-
>  src/include/pg_config.h.in  |  14 +--
>  src/include/port/pg_iovec.h |   4 +-
>  src/tools/msvc/Solution.pm  |   4 +-
>  5 files changed, 157 insertions(+), 38 deletions(-)
>
> diff --git a/configure b/configure
> index 8af4b99021..07a9b08d80 100755
> --- a/configure
> +++ b/configure
> @@ -5373,6 +5373,98 @@ if test x"$pgac_cv_prog_CC_cflags__Werror_vla" = x"yes"; then
>  fi
>
>
> +  # Prevent usage of symbols marked as newer than our target.
> +
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Werror=unguarded-availability-new, for
CFLAGS">&5 
> +$as_echo_n "checking whether ${CC} supports -Werror=unguarded-availability-new, for CFLAGS... " >&6; }
> +if ${pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  pgac_save_CFLAGS=$CFLAGS
> +pgac_save_CC=$CC
> +CC=${CC}
> +CFLAGS="${CFLAGS} -Werror=unguarded-availability-new"
> +ac_save_c_werror_flag=$ac_c_werror_flag
> +ac_c_werror_flag=yes
> +cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +
> +int
> +main ()
> +{
> +
> +  ;
> +  return 0;
> +}
> +_ACEOF
> +if ac_fn_c_try_compile "$LINENO"; then :
> +  pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new=yes
> +else
> +  pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new=no
> +fi
> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
> +ac_c_werror_flag=$ac_save_c_werror_flag
> +CFLAGS="$pgac_save_CFLAGS"
> +CC="$pgac_save_CC"
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" >&5
> +$as_echo "$pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" >&6; }
> +if test x"$pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" = x"yes"; then
> +  CFLAGS="${CFLAGS} -Werror=unguarded-availability-new"
> +fi
> +
> +
> +  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -Werror=unguarded-availability-new, for
CXXFLAGS">&5 
> +$as_echo_n "checking whether ${CXX} supports -Werror=unguarded-availability-new, for CXXFLAGS... " >&6; }
> +if ${pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new+:} false; then :
> +  $as_echo_n "(cached) " >&6
> +else
> +  pgac_save_CXXFLAGS=$CXXFLAGS
> +pgac_save_CXX=$CXX
> +CXX=${CXX}
> +CXXFLAGS="${CXXFLAGS} -Werror=unguarded-availability-new"
> +ac_save_cxx_werror_flag=$ac_cxx_werror_flag
> +ac_cxx_werror_flag=yes
> +ac_ext=cpp
> +ac_cpp='$CXXCPP $CPPFLAGS'
> +ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
> +ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
> +ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
> +
> +cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h.  */
> +
> +int
> +main ()
> +{
> +
> +  ;
> +  return 0;
> +}
> +_ACEOF
> +if ac_fn_cxx_try_compile "$LINENO"; then :
> +  pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new=yes
> +else
> +  pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new=no
> +fi
> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
> +ac_ext=c
> +ac_cpp='$CPP $CPPFLAGS'
> +ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
> +ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
> +ac_compiler_gnu=$ac_cv_c_compiler_gnu
> +
> +ac_cxx_werror_flag=$ac_save_cxx_werror_flag
> +CXXFLAGS="$pgac_save_CXXFLAGS"
> +CXX="$pgac_save_CXX"
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" >&5
> +$as_echo "$pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" >&6; }
> +if test x"$pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" = x"yes"; then
> +  CXXFLAGS="${CXXFLAGS} -Werror=unguarded-availability-new"
> +fi
> +
> +
>    # -Wvla is not applicable for C++
>
>  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wendif-labels, for CFLAGS" >&5
> @@ -15646,6 +15738,52 @@ cat >>confdefs.h <<_ACEOF
>  _ACEOF
>
>
> +# AC_REPLACE_FUNCS does not respect the deployment target on macOS
> +ac_fn_c_check_decl "$LINENO" "preadv" "ac_cv_have_decl_preadv" "#include <sys/uio.h>
> +"
> +if test "x$ac_cv_have_decl_preadv" = xyes; then :
> +  ac_have_decl=1
> +else
> +  ac_have_decl=0
> +fi
> +
> +cat >>confdefs.h <<_ACEOF
> +#define HAVE_DECL_PREADV $ac_have_decl
> +_ACEOF
> +if test $ac_have_decl = 1; then :
> +
> +else
> +  case " $LIBOBJS " in
> +  *" preadv.$ac_objext "* ) ;;
> +  *) LIBOBJS="$LIBOBJS preadv.$ac_objext"
> + ;;
> +esac
> +
> +fi
> +
> +ac_fn_c_check_decl "$LINENO" "pwritev" "ac_cv_have_decl_pwritev" "#include <sys/uio.h>
> +"
> +if test "x$ac_cv_have_decl_pwritev" = xyes; then :
> +  ac_have_decl=1
> +else
> +  ac_have_decl=0
> +fi
> +
> +cat >>confdefs.h <<_ACEOF
> +#define HAVE_DECL_PWRITEV $ac_have_decl
> +_ACEOF
> +if test $ac_have_decl = 1; then :
> +
> +else
> +  case " $LIBOBJS " in
> +  *" pwritev.$ac_objext "* ) ;;
> +  *) LIBOBJS="$LIBOBJS pwritev.$ac_objext"
> + ;;
> +esac
> +
> +fi
> +
> +
>  ac_fn_c_check_decl "$LINENO" "RTLD_GLOBAL" "ac_cv_have_decl_RTLD_GLOBAL" "#include <dlfcn.h>
>  "
>  if test "x$ac_cv_have_decl_RTLD_GLOBAL" = xyes; then :
> @@ -15845,19 +15983,6 @@ esac
>
>  fi
>
> -ac_fn_c_check_func "$LINENO" "preadv" "ac_cv_func_preadv"
> -if test "x$ac_cv_func_preadv" = xyes; then :
> -  $as_echo "#define HAVE_PREADV 1" >>confdefs.h
> -
> -else
> -  case " $LIBOBJS " in
> -  *" preadv.$ac_objext "* ) ;;
> -  *) LIBOBJS="$LIBOBJS preadv.$ac_objext"
> - ;;
> -esac
> -
> -fi
> -
>  ac_fn_c_check_func "$LINENO" "pwrite" "ac_cv_func_pwrite"
>  if test "x$ac_cv_func_pwrite" = xyes; then :
>    $as_echo "#define HAVE_PWRITE 1" >>confdefs.h
> @@ -15871,19 +15996,6 @@ esac
>
>  fi
>
> -ac_fn_c_check_func "$LINENO" "pwritev" "ac_cv_func_pwritev"
> -if test "x$ac_cv_func_pwritev" = xyes; then :
> -  $as_echo "#define HAVE_PWRITEV 1" >>confdefs.h
> -
> -else
> -  case " $LIBOBJS " in
> -  *" pwritev.$ac_objext "* ) ;;
> -  *) LIBOBJS="$LIBOBJS pwritev.$ac_objext"
> - ;;
> -esac
> -
> -fi
> -
>  ac_fn_c_check_func "$LINENO" "random" "ac_cv_func_random"
>  if test "x$ac_cv_func_random" = xyes; then :
>    $as_echo "#define HAVE_RANDOM 1" >>confdefs.h
> diff --git a/configure.ac b/configure.ac
> index 868a94c9ba..0cd1ee8909 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -494,6 +494,9 @@ if test "$GCC" = yes -a "$ICC" = no; then
>    AC_SUBST(PERMIT_DECLARATION_AFTER_STATEMENT)
>    # Really don't want VLAs to be used in our dialect of C
>    PGAC_PROG_CC_CFLAGS_OPT([-Werror=vla])
> +  # Prevent usage of symbols marked as newer than our target.
> +  PGAC_PROG_CC_CFLAGS_OPT([-Werror=unguarded-availability-new])
> +  PGAC_PROG_CXX_CFLAGS_OPT([-Werror=unguarded-availability-new])
>    # -Wvla is not applicable for C++
>    PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
>    PGAC_PROG_CXX_CFLAGS_OPT([-Wendif-labels])
> @@ -1705,6 +1708,10 @@ AC_CHECK_DECLS([strlcat, strlcpy, strnlen])
>  # This is probably only present on macOS, but may as well check always
>  AC_CHECK_DECLS(F_FULLFSYNC, [], [], [#include <fcntl.h>])
>
> +# AC_REPLACE_FUNCS does not respect the deployment target on macOS
> +AC_CHECK_DECLS([preadv], [], [AC_LIBOBJ(preadv)], [#include <sys/uio.h>])
> +AC_CHECK_DECLS([pwritev], [], [AC_LIBOBJ(pwritev)], [#include <sys/uio.h>])
Does this approach using a different standard autoconf probe adequately address
the maintainability issue regarding the AC_LANG_PROGRAM autoconf probes in my
previous patch brought up in https://postgr.es/m/915981.1611254324@sss.pgh.pa.us
or should I look for another alternative?
> +
>  AC_CHECK_DECLS([RTLD_GLOBAL, RTLD_NOW], [], [], [#include <dlfcn.h>])
>
>  AC_CHECK_TYPE([struct sockaddr_in6],
> @@ -1737,9 +1744,7 @@ AC_REPLACE_FUNCS(m4_normalize([
>         link
>         mkdtemp
>         pread
> -       preadv
>         pwrite
> -       pwritev
>         random
>         srandom
>         strlcat
> diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
> index f4d9f3b408..9c30d008c9 100644
> --- a/src/include/pg_config.h.in
> +++ b/src/include/pg_config.h.in
> @@ -142,6 +142,14 @@
>     don't. */
>  #undef HAVE_DECL_POSIX_FADVISE
>
> +/* Define to 1 if you have the declaration of `preadv', and to 0 if you don't.
> +   */
> +#undef HAVE_DECL_PREADV
> +
> +/* Define to 1 if you have the declaration of `pwritev', and to 0 if you
> +   don't. */
> +#undef HAVE_DECL_PWRITEV
> +
>  /* Define to 1 if you have the declaration of `RTLD_GLOBAL', and to 0 if you
>     don't. */
>  #undef HAVE_DECL_RTLD_GLOBAL
> @@ -412,9 +420,6 @@
>  /* Define to 1 if you have the `pread' function. */
>  #undef HAVE_PREAD
>
> -/* Define to 1 if you have the `preadv' function. */
> -#undef HAVE_PREADV
> -
>  /* Define to 1 if you have the `pstat' function. */
>  #undef HAVE_PSTAT
>
> @@ -433,9 +438,6 @@
>  /* Define to 1 if you have the `pwrite' function. */
>  #undef HAVE_PWRITE
>
> -/* Define to 1 if you have the `pwritev' function. */
> -#undef HAVE_PWRITEV
> -
>  /* Define to 1 if you have the `random' function. */
>  #undef HAVE_RANDOM
>
> diff --git a/src/include/port/pg_iovec.h b/src/include/port/pg_iovec.h
> index 365d605a9b..760ec4980c 100644
> --- a/src/include/port/pg_iovec.h
> +++ b/src/include/port/pg_iovec.h
> @@ -39,13 +39,13 @@ struct iovec
>  /* Define a reasonable maximum that is safe to use on the stack. */
>  #define PG_IOV_MAX Min(IOV_MAX, 32)
>
> -#ifdef HAVE_PREADV
> +#if HAVE_DECL_PREADV
I could rework this to use HAVE_PWRITEV like before if that is
preferable, I just
went with this since it is the default for AC_CHECK_DECLS.
>  #define pg_preadv preadv
>  #else
>  extern ssize_t pg_preadv(int fd, const struct iovec *iov, int iovcnt, off_t offset);
>  #endif
>
> -#ifdef HAVE_PWRITEV
> +#if HAVE_DECL_PWRITEV
>  #define pg_pwritev pwritev
>  #else
>  extern ssize_t pg_pwritev(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
> index 2f28de0355..7c235f227d 100644
> --- a/src/tools/msvc/Solution.pm
> +++ b/src/tools/msvc/Solution.pm
> @@ -329,14 +329,14 @@ sub GenerateFiles
>                 HAVE_PPC_LWARX_MUTEX_HINT   => undef,
>                 HAVE_PPOLL                  => undef,
>                 HAVE_PREAD                  => undef,
> -               HAVE_PREADV                 => undef,
> +               HAVE_DECL_PREADV            => 0,
>                 HAVE_PSTAT                  => undef,
>                 HAVE_PS_STRINGS             => undef,
>                 HAVE_PTHREAD                => undef,
>                 HAVE_PTHREAD_IS_THREADED_NP => undef,
>                 HAVE_PTHREAD_PRIO_INHERIT   => undef,
>                 HAVE_PWRITE                 => undef,
> -               HAVE_PWRITEV                => undef,
> +               HAVE_DECL_PWRITEV           => 0,
>                 HAVE_RANDOM                 => undef,
>                 HAVE_READLINE_H             => undef,
>                 HAVE_READLINE_HISTORY_H     => undef,
> --
> 2.30.0
>

Hi James,

On 1/31/21 1:59 AM, James Hilliard wrote:
> On Fri, Jan 22, 2021 at 12:32 PM James Hilliard
> <james.hilliard1@gmail.com> wrote:
>>
>> Fixes:
>> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -O2
-I../../../../src/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   -c -o fd.o fd.c
 
>> fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
>>                  part = pg_pwritev(fd, iov, iovcnt, offset);
>>                         ^~~~~~~~~~

It would be better to provide this patch as an attachment so the cfbot 
(http://commitfest.cputube.org/) can test it.

Regards,
-- 
-David
david@pgmasters.net

Should it work if I just attach it to the thread like this?

On Mon, Mar 29, 2021 at 7:52 AM David Steele <david@pgmasters.net> wrote:
>
> Hi James,
>
> On 1/31/21 1:59 AM, James Hilliard wrote:
> > On Fri, Jan 22, 2021 at 12:32 PM James Hilliard
> > <james.hilliard1@gmail.com> wrote:
> >>
> >> Fixes:
> >> gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -O2
-I../../../../src/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   -c -o fd.o fd.c 
> >> fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
> >>                  part = pg_pwritev(fd, iov, iovcnt, offset);
> >>                         ^~~~~~~~~~
>
> It would be better to provide this patch as an attachment so the cfbot
> (http://commitfest.cputube.org/) can test it.
>
> Regards,
> --
> -David
> david@pgmasters.net

On Tue, Mar 30, 2021 at 6:37 AM James Hilliard
<james.hilliard1@gmail.com> wrote:
> Should it work if I just attach it to the thread like this?

Yes.  It automatically tries patches that are attached to threads that
are registered on commitfest.postgresql.org on 4 OSes, and we can see
that it succeeded, and we can inspect the configure output and see
that only the two clang-based systems detected and used the new
unguarded-availability-new flags and used them.

This should be alphabetised better:

         HAVE_PREAD                  => undef,
-        HAVE_PREADV                 => undef,
+        HAVE_DECL_PREADV            => 0,
         HAVE_PSTAT                  => undef,

So the question here is really:  do we want to support Apple cross-SDK
builds, in our configure scripts?  It costs very little to switch from
traditional "does-this-symbol-exist?" tests to testing declarations,
so no objections here.

I doubt people will remember to do this for other new syscall probes
though, so it might be a matter of discussing it case-by-case when a
problem shows up.  For example, I recently added another new test,
specifically targeting macOS: pthread_barrier_wait.  One day they
might add it to libSystem and we might need to tweak that one
similarly.

On Mon, Mar 29, 2021 at 4:10 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 6:37 AM James Hilliard
> <james.hilliard1@gmail.com> wrote:
> > Should it work if I just attach it to the thread like this?
>
> Yes.  It automatically tries patches that are attached to threads that
> are registered on commitfest.postgresql.org on 4 OSes, and we can see
> that it succeeded, and we can inspect the configure output and see
> that only the two clang-based systems detected and used the new
> unguarded-availability-new flags and used them.
That sounds right, the flag is clang specific from my understanding.
>
> This should be alphabetised better:
>
>          HAVE_PREAD                  => undef,
> -        HAVE_PREADV                 => undef,
> +        HAVE_DECL_PREADV            => 0,
>          HAVE_PSTAT                  => undef,
Should I resend with that changed or can it just be fixed when applied?
>
> So the question here is really:  do we want to support Apple cross-SDK
> builds, in our configure scripts?  It costs very little to switch from
> traditional "does-this-symbol-exist?" tests to testing declarations,
> so no objections here.
Well this adds support for the target availability setting, which applies
to effectively all Apple SDK's, so it's really more of a cross target issue
rather than a cross SDK issue than anything from what I can tell.

Effectively without this change setting MACOSX_DEPLOYMENT_TARGET
would not work properly on any Apple SDK's. Currently postgres essentially
is relying on the command line tools not supporting newer targets than
the host system, which is not something that appears to be guaranteed
at all by Apple from my understanding. This is because the current
detection technique is unable to detect if a symbol is restricted by
MACOSX_DEPLOYMENT_TARGET, so it will essentially always
use the newest SDK symbols even if they are only available to a
MACOSX_DEPLOYMENT_TARGET newer than the configured
deployment target.

Say for example if I want to build for a 10.14 target from a 10.15 host
with the standard 10.15 command line tools, with this change that is
possible simply by setting MACOSX_DEPLOYMENT_TARGET,
otherwise it will only build for a 10.14 target from a SDK that does not
have 10.15 only symbols present at all, even if that SDK has full
support for 10.14 targets.
>
> I doubt people will remember to do this for other new syscall probes
> though, so it might be a matter of discussing it case-by-case when a
> problem shows up.  For example, I recently added another new test,
> specifically targeting macOS: pthread_barrier_wait.  One day they
> might add it to libSystem and we might need to tweak that one
> similarly.
Yeah, this technique should allow for trivially supporting new symbol
target availability in the OSX SDK fairly easily, as long as you have
the unguarded-availability-new flag set the compile tests respect the
compilers target availability settings if the appropriate header is included.

On Tue, Mar 30, 2021 at 12:32 PM James Hilliard
<james.hilliard1@gmail.com> wrote:
> Should I resend with that changed or can it just be fixed when applied?

I'll move it when committing.  I'll let this patch sit for another day
to see if any other objections show up.

Thomas Munro <thomas.munro@gmail.com> writes:
> I'll move it when committing.  I'll let this patch sit for another day
> to see if any other objections show up.

FWIW, I remain fairly strongly against this, precisely because of the
point that it requires us to start using a randomly different
feature-probing technology anytime Apple decides that they're going to
implement some standard API that they didn't before.  Even if it works
everywhere for preadv/pwritev (which we won't know in advance of
buildfarm testing, and maybe not then, since detection failures will
probably be silent), it seems likely that we'll hit some case in the
future where this interacts badly with some other platform's weirdness.
We haven't claimed in the past to support MACOSX_DEPLOYMENT_TARGET,
and I'm not sure we should start now.  How many people actually care
about that?

            regards, tom lane

On Mon, Mar 29, 2021 at 11:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Thomas Munro <thomas.munro@gmail.com> writes:
> > I'll move it when committing.  I'll let this patch sit for another day
> > to see if any other objections show up.
>
> FWIW, I remain fairly strongly against this, precisely because of the
> point that it requires us to start using a randomly different
> feature-probing technology anytime Apple decides that they're going to
> implement some standard API that they didn't before.

This effectively works automatically as long as the feature check includes
the appropriate header. This works with AC_CHECK_DECLS or any
other autoconf probes that include the appropriate headers.

> Even if it works
> everywhere for preadv/pwritev (which we won't know in advance of
> buildfarm testing, and maybe not then, since detection failures will
> probably be silent), it seems likely that we'll hit some case in the
> future where this interacts badly with some other platform's weirdness.

Well part of the motivation for setting unguarded-availability-new is so
that we get a hard error instead of just a deployment target version
incompatibility warning, the current situation does actually result in
weird runtime errors, this change makes those much more obvious
build errors.

To ensure these errors surface quickly one could have some of the
buildfarm tests set different MACOSX_DEPLOYMENT_TARGET's
which should then hard error during the build if incompatible symbols are
accidentally included.

I don't think this is likely to cause issues for other platforms unless they use
the availability attribute incorrectly see: https://reviews.llvm.org/D34264

> We haven't claimed in the past to support MACOSX_DEPLOYMENT_TARGET,
> and I'm not sure we should start now.  How many people actually care
> about that?

Seems kinda important for anyone who wants to build postgres
compatible with targets older than the host system.

On Tue, Mar 30, 2021 at 7:39 PM James Hilliard
<james.hilliard1@gmail.com> wrote:
> On Mon, Mar 29, 2021 at 11:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > We haven't claimed in the past to support MACOSX_DEPLOYMENT_TARGET,
> > and I'm not sure we should start now.  How many people actually care
> > about that?
>
> Seems kinda important for anyone who wants to build postgres
> compatible with targets older than the host system.

Personally I'm mostly concerned about making it easy for new
contributors to get a working dev system going on a super common
platform without dealing with hard-to-diagnose errors, than people who
actually want a different target as a deliberate choice.  Do I
understand correctly that there a period of time each year when major
upgrades come out of sync and lots of people finish up running a
toolchain and OS with this problem for a while due to the default
target not matching?   If so I wonder if other projects are running
into this with AC_REPLACE_FUNCS and what they're doing.

I suppose an alternative strategy would be to try to detect the
mismatch and spit out a friendlier warning, if we decide we're not
going to support such builds.

On Tue, Mar 30, 2021 at 6:43 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 7:39 PM James Hilliard
> <james.hilliard1@gmail.com> wrote:
> > On Mon, Mar 29, 2021 at 11:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > We haven't claimed in the past to support MACOSX_DEPLOYMENT_TARGET,
> > > and I'm not sure we should start now.  How many people actually care
> > > about that?
> >
> > Seems kinda important for anyone who wants to build postgres
> > compatible with targets older than the host system.
>
> Personally I'm mostly concerned about making it easy for new
> contributors to get a working dev system going on a super common
> platform without dealing with hard-to-diagnose errors, than people who
> actually want a different target as a deliberate choice.

Yeah, that's where I was running into this. Currently we build for the max
deployment target available in the SDK, regardless of if that is the deployment
target actually set, the compiler by default automatically sets the deployment
target to the build host but if the SDK supports newer deployment targets
that's where things break down.

> Do I
> understand correctly that there a period of time each year when major
> upgrades come out of sync and lots of people finish up running a
> toolchain and OS with this problem for a while due to the default
> target not matching?

Well you can hit this if you try and build against a toolchain that supports
targets newer than the host pretty easily, although I think postgres
tries to use the cli tools SDK by default which appears somewhat less
prone to this issue(although I don't think this behavior is guaranteed).

> If so I wonder if other projects are running
> into this with AC_REPLACE_FUNCS and what they're doing.

Well I did come up with another approach, which uses AC_LANG_PROGRAM
instead of AC_CHECK_DECLS that might be better
https://lists.gnu.org/archive/html/autoconf-patches/2021-02/msg00007.html

However I didn't submit that version here since it uses a custom probe via
AC_LANG_PROGRAM instead of a standard probe like AC_CHECK_DECLS
which Tom Lane said would be a maintenance issue, at least with this
AC_CHECK_DECLS method we can avoid using any non-standard probes:
https://postgr.es/m/915981.1611254324%40sss.pgh.pa.us


>
> I suppose an alternative strategy would be to try to detect the
> mismatch and spit out a friendlier warning, if we decide we're not
> going to support such builds.

Thomas Munro <thomas.munro@gmail.com> writes:
> Personally I'm mostly concerned about making it easy for new
> contributors to get a working dev system going on a super common
> platform without dealing with hard-to-diagnose errors, than people who
> actually want a different target as a deliberate choice.  Do I
> understand correctly that there a period of time each year when major
> upgrades come out of sync and lots of people finish up running a
> toolchain and OS with this problem for a while due to the default
> target not matching?   If so I wonder if other projects are running
> into this with AC_REPLACE_FUNCS and what they're doing.

Yeah, we've seen this happen at least a couple of times, though
it was only during this past cycle that we (I anyway) entirely
understood what was happening.

The patches we committed in January (4823621db, 9d23c15a0, 50bebc1ae)
to improve our PG_SYSROOT selection heuristics should theoretically
improve the situation ... though I admit I won't have a lot of
confidence in them till we've been through a couple more rounds of
asynchronous-XCode-and-macOS releases.  Still, I feel that we
ought to leave that code alone until we see how it does.

            regards, tom lane

On Tue, Mar 30, 2021 at 7:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Personally I'm mostly concerned about making it easy for new
> > contributors to get a working dev system going on a super common
> > platform without dealing with hard-to-diagnose errors, than people who
> > actually want a different target as a deliberate choice.  Do I
> > understand correctly that there a period of time each year when major
> > upgrades come out of sync and lots of people finish up running a
> > toolchain and OS with this problem for a while due to the default
> > target not matching?   If so I wonder if other projects are running
> > into this with AC_REPLACE_FUNCS and what they're doing.
>
> Yeah, we've seen this happen at least a couple of times, though
> it was only during this past cycle that we (I anyway) entirely
> understood what was happening.
>
> The patches we committed in January (4823621db, 9d23c15a0, 50bebc1ae)
> to improve our PG_SYSROOT selection heuristics should theoretically
> improve the situation ... though I admit I won't have a lot of
> confidence in them till we've been through a couple more rounds of
> asynchronous-XCode-and-macOS releases.  Still, I feel that we
> ought to leave that code alone until we see how it does.

I mean, we know that it will still break under a number of common
circumstances so I think we should be fixing the root cause(improper
detection of target deployment API availability in probes) in some
way as this will probably continue to be an issue otherwise, we already
know that improving PG_SYSROOT selection can not fix the root issue
but rather tries to workaround it in a way that is pretty much guaranteed
to be brittle.

Is there any approach to fixing the root cause of this issue that you think
would be acceptable?

>
>                         regards, tom lane

On Mon, Jun 21, 2021 at 4:32 PM Sandeep Thakkar
<sandeep.thakkar@enterprisedb.com> wrote:
> Do we see any solution to this issue? or using the older SDK is the way to go?

> On Thu, May 20, 2021 at 2:04 PM Dave Page <dpage@pgadmin.org> wrote:
>> The ability to target older releases with a newer SDK is essential for packages such as the EDB PostgreSQL
installersand the pgAdmin community installers. It's very difficult (sometimes impossible) to get older OS versions on
newmachines now - Apple make it very hard to download old versions of macOS (some can be found, others not), and they
won'talways work on newer hardware anyway so it's really not feasible to have all the build machines running the oldest
versionthat needs to be supported. 
>>
>> FYI, the pgAdmin and PG installer buildfarms have -mmacosx-version-min=10.12 in CFLAGS etc. to handle this, which is
synonymouswith MACOSX_DEPLOYMENT_TARGET. We've been successfully building packages that way for a decade or more. 

I'm not personally against the proposed change.  I'll admit there is
something annoying about Apple's environment working in a way that
doesn't suit traditional configure macros that have been the basis of
portable software for a few decades, but when all's said and done,
configure is a Unix wars era way to make things work across all the
Unixes, and most of them are long gone, configure itself is on the way
out, and Apple's still here, so...

On a more practical note, rereading Tom's objection and Dave's
counter-objection, I'm left wondering if people would be happy with a
manual control for this, something you can pass to configure to stop
it from using pwritev/preadv even if detected.  That would at least
localise the effects, avoiding the sorts of potential unintended
consequences Tom mentioned.

On 21.06.21 07:22, Thomas Munro wrote:
> I'm not personally against the proposed change.  I'll admit there is
> something annoying about Apple's environment working in a way that
> doesn't suit traditional configure macros that have been the basis of
> portable software for a few decades, but when all's said and done,
> configure is a Unix wars era way to make things work across all the
> Unixes, and most of them are long gone, configure itself is on the way
> out, and Apple's still here, so...

I think this change is perfectly appropriate (modulo some small cleanups).

The objection was that you cannot reliably use AC_CHECK_FUNCS (and 
therefore AC_REPLACE_FUNCS) anymore, but that has always been true, 
since AC_CHECK_FUNCS doesn't handle macros, compiler built-ins, and 
functions that are not declared, and any other situation where looking 
for a symbol in a library is not the same as checking whether the symbol 
actual works for your purpose.  This is not too different from the long 
transition from "does this header file exists" to "can I compile this 
header file".

So in fact the correct way forward would be to get rid of all uses of 
AC_CHECK_FUNCS and related, and then this problem goes away by itself.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> I think this change is perfectly appropriate (modulo some small cleanups).

I think there are a couple of issues here.

1. People who are already using MACOSX_DEPLOYMENT_TARGET to control
their builds would like to keep on doing so, but the AC_CHECK_FUNCS
probe doesn't work with that.  James' latest patch seems like a
reasonable fix for that (it's a lot less invasive than where we
started from).  There is a worry about side-effects on other
platforms, but I don't see an answer to that that's better than
"throw it on the buildfarm and see if anything breaks".

However ...

2. We'd really like to use preadv/pwritev where available.  I
maintain that MACOSX_DEPLOYMENT_TARGET is not only not the right
approach to that, but it's actually counterproductive.  It forces
you to build for the lowest common denominator, ie the oldest macOS
release you want to support.  Even when running on a release that
has pwritev, your build will never use it.

As far as I can tell, the only way to really deal with #2 is to
perform a runtime dlsym() probe to see whether pwritev exists, and
then fall back to our src/port/ implementation if not.  This does
not look particularly hard (especially since the lookup code only
needs to work on macOS), though I admit I've not tried to code it.

What's unclear to me at the moment is whether #1 and #2 interact,
ie is there still any point in changing configure's probes if
we put in a runtime check on Darwin?  I think that we might want
to pay no attention to what the available header files say about
pwritev, as long as we can get the correct 'struct iovec'
declaration from them.

            regards, tom lane

On Tue, Jul 6, 2021 at 2:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> > I think this change is perfectly appropriate (modulo some small cleanups).
>
> I think there are a couple of issues here.
>
> 1. People who are already using MACOSX_DEPLOYMENT_TARGET to control
> their builds would like to keep on doing so, but the AC_CHECK_FUNCS
> probe doesn't work with that.  James' latest patch seems like a
> reasonable fix for that (it's a lot less invasive than where we
> started from).  There is a worry about side-effects on other
> platforms, but I don't see an answer to that that's better than
> "throw it on the buildfarm and see if anything breaks".
>
> However ...
>
> 2. We'd really like to use preadv/pwritev where available.  I
> maintain that MACOSX_DEPLOYMENT_TARGET is not only not the right
> approach to that, but it's actually counterproductive.  It forces
> you to build for the lowest common denominator, ie the oldest macOS
> release you want to support.  Even when running on a release that
> has pwritev, your build will never use it.
>
> As far as I can tell, the only way to really deal with #2 is to
> perform a runtime dlsym() probe to see whether pwritev exists, and
> then fall back to our src/port/ implementation if not.  This does
> not look particularly hard (especially since the lookup code only
> needs to work on macOS), though I admit I've not tried to code it.

Maybe we just need to do something like this?:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg06499.html

>
> What's unclear to me at the moment is whether #1 and #2 interact,
> ie is there still any point in changing configure's probes if
> we put in a runtime check on Darwin?  I think that we might want
> to pay no attention to what the available header files say about
> pwritev, as long as we can get the correct 'struct iovec'
> declaration from them.
>
>                         regards, tom lane

James Hilliard <james.hilliard1@gmail.com> writes:
> On Tue, Jul 6, 2021 at 2:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As far as I can tell, the only way to really deal with #2 is to
>> perform a runtime dlsym() probe to see whether pwritev exists, and
>> then fall back to our src/port/ implementation if not.  This does
>> not look particularly hard (especially since the lookup code only
>> needs to work on macOS), though I admit I've not tried to code it.

> Maybe we just need to do something like this?:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg06499.html

Hm.  I don't trust __builtin_available too much --- does it exist
on really old macOS?  dlsym() seems less likely to create problems
in that respect.  Of course, if there are scenarios where
__builtin_available knows that the symbol is available but doesn't
work, then we might have to use it.

In any case, I don't like their superstructure around the probe.
What I'd prefer is a function pointer that initially points to
the probe code, and then we change it to point at either pwritev
or the pg_pwritev emulation.  Certainly failing with ENOSYS is
exactly what *not* to do.

            regards, tom lane

On 06.07.21 22:34, Tom Lane wrote:
> 2. We'd really like to use preadv/pwritev where available.

A couple of things that I haven't seen made clear in this thread yet:

- Where is the availability boundary for preadv/pwritev on macOS?

- What is the impact of having vs. not having these functions?

> I
> maintain that MACOSX_DEPLOYMENT_TARGET is not only not the right
> approach to that, but it's actually counterproductive.  It forces
> you to build for the lowest common denominator, ie the oldest macOS
> release you want to support.  Even when running on a release that
> has pwritev, your build will never use it.

I think this is just the way that building backward-compatible binaries 
on macOS (and Windows) works.  You have to pick a target that is old 
enough to capture enough of your audience but not too old to miss out on 
interesting new OS features.  People who build GUI applications for 
macOS, iOS, etc. face this trade-off all the time; for POSIX-level 
programming things just move slower so that the questions present 
themselves less often.  I don't think we need to go out of our way to 
fight this system.  This is something users will have opted into after 
all.  Users who want Linux-style rebuilt-for-every-release binaries have 
those options available on macOS as well.

On Tue, Jul 13, 2021 at 7:39 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> On 06.07.21 22:34, Tom Lane wrote:
> > 2. We'd really like to use preadv/pwritev where available.
>
> A couple of things that I haven't seen made clear in this thread yet:
>
> - Where is the availability boundary for preadv/pwritev on macOS?

Big Sur (11) added these.

> - What is the impact of having vs. not having these functions?

The impact is very low.  PG14 only uses pg_pwritev() to fill in new
WAL files as a sort of a warm-up exercise, and it'll happy use lseek()
+ writev() instead.  In future proposals they would be used to do
general scatter/gather I/O for data files as I showed in another
email[1], but that's way off and far from certain, and even then it's
just a matter of avoiding an lseek() call on vectored I/O.  As for how
long Apple will support 10.15, they don't seem to publish a roadmap,
but people seem to say the pattern would have security updates ending
some time in 2022 (?).  I don't know if EDB targets macOS older than
Apple supports, but given the very low impact and all these time
frames it seems OK  to just not use the new syscalls on macOS for a
couple more years at least, whatever mechanism is chosen for that.

Clearly there is a more general question though, which is "should we
buy into Apple's ABI management system or not", and I don't have a
strong opinion on that.  One thing I do know is that
pthread_barrier_XXX changed from option to required in a recentish
POSIX update so I expect the question to come up again eventually.

[1] https://www.postgresql.org/message-id/CA%2BhUKGK-563RQWQQF4NLajbQk%2B65gYHdb1q%3D7p3Ob0Uvrxoa9g%40mail.gmail.com

Thomas Munro <thomas.munro@gmail.com> writes:
> Clearly there is a more general question though, which is "should we
> buy into Apple's ABI management system or not", and I don't have a
> strong opinion on that.

Well, I definitely don't wish to clutter our core code with any
explicit dependencies on MACOSX_DEPLOYMENT_TARGET.  However,
if we can work around the issue by switching from AC_REPLACE_FUNCS
to AC_CHECK_DECLS, maybe we should just do that and quit arguing
about it.  It seems like there's not a lot of enthusiasm for my idea
about installing a run-time probe, so I won't push for that.

> One thing I do know is that
> pthread_barrier_XXX changed from option to required in a recentish
> POSIX update so I expect the question to come up again eventually.

Yeah, we can expect that the issue will arise again, which is why
I was so unhappy with the rather-invasive patches we started with.

            regards, tom lane