Re: pgsql: Add PSQL_WATCH_PAGER for psql's \watch command. - Mailing list pgsql-committers

Thomas Munro <thomas.munro@gmail.com> writes:
> I decided to try to make it work properly on that OS instead of the
> hacky solution now that I have access.  I took your last patch, moved
> -D_POSIX_PTHREAD_SEMANTICS into CFLAGS in src/template/solaris,

Aha!  Yeah, just defining _POSIX_PTHREAD_SEMANTICS unconditionally
on Solaris seems like a good idea.  However, messing with the template
file is not the best way to inject that, because if the user specifies
CFLAGS then the template file's setting is ignored.  It's better to
handle it in the part of configure.ac that automatically adds flags
onto whatever the user or template file gave.  Also, being a -D
switch, it really oughta go into CPPFLAGS not CFLAGS.

I've tested the attached on the GCC farm's Solaris machine (which
I believe is the same machine wrasse runs on), and it seems OK.
I think it's ready to go.

> I can't find any evidence on the 'net that any other OS
> cares about _POSIX_PTHREAD_SEMANTICS (every reference says it's a
> Solaris thing) so this should just remove some useless clutter,

Agreed; if we do find any other platforms that need it, we can extend
the PORTNAME check used here.

> Another thing
> I noticed is that config/ax_pthread.m4 (something from autoconf that
> we don't want to modify) will add another copy of
> -D_POSIX_PTHREAD_SEMANTICS, but that's already the case.

Ah, yeah, I see:

$ grep POSIX src/Makefile.global
PTHREAD_CFLAGS = -D_POSIX_PTHREAD_SEMANTICS -pthread -D_REENTRANT -D_THREAD_SAFE
CPPFLAGS =  -D_POSIX_PTHREAD_SEMANTICS
Seems pretty harmless, and anyway we have to make sure this gets in there
whether or not --enable-thread-safety is given.

            regards, tom lane

diff --git a/config/thread_test.c b/config/thread_test.c
index 784f4fe8ce..e2a9e62f49 100644
--- a/config/thread_test.c
+++ b/config/thread_test.c
@@ -43,10 +43,6 @@
 #include <winsock2.h>
 #endif

-/* Test for POSIX.1c 2-arg sigwait() and fail on single-arg version */
-#include <signal.h>
-int            sigwait(const sigset_t *set, int *sig);
-

 #define        TEMP_FILENAME_1 "thread_test.1"
 #define        TEMP_FILENAME_2 "thread_test.2"
diff --git a/configure b/configure
index 1ea28a0d67..c85eb1bf55 100755
--- a/configure
+++ b/configure
@@ -7194,6 +7194,12 @@ $as_echo "#define PROFILE_PID_DIR 1" >>confdefs.h
   fi
 fi

+# On Solaris, we need this #define to get POSIX-conforming versions
+# of many interfaces (sigwait, getpwuid_r, ...).
+if test "$PORTNAME" = "solaris"; then
+  CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"
+fi
+
 # We already have this in Makefile.win32, but configure needs it too
 if test "$PORTNAME" = "win32"; then
   CPPFLAGS="$CPPFLAGS -I$srcdir/src/include/port/win32"
@@ -11296,9 +11302,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
     # set thread flags

 # Some platforms use these, so just define them.  They can't hurt if they
-# are not supported.  For example, on Solaris -D_POSIX_PTHREAD_SEMANTICS
-# enables 5-arg getpwuid_r, among other things.
-PTHREAD_CFLAGS="$PTHREAD_CFLAGS -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS"
+# are not supported.
+PTHREAD_CFLAGS="$PTHREAD_CFLAGS -D_REENTRANT -D_THREAD_SAFE"

 # Check for *_r functions
 _CFLAGS="$CFLAGS"
@@ -15861,9 +15866,11 @@ $as_echo "#define HAVE_FSEEKO 1" >>confdefs.h
 fi


-# posix_fadvise() is a no-op on Solaris, so don't incur function overhead
-# by calling it, 2009-04-02
-# http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/port/gen/posix_fadvise.c
+# Make sure there's a declaration for sigwait(), then make sure
+# that it conforms to the POSIX standard (there seem to still be
+# some platforms out there with pre-POSIX sigwait()).  On Solaris,
+# _POSIX_PTHREAD_SEMANTICS affects the result, but we already
+# added that to CPPFLAGS.
 # The Clang compiler raises a warning for an undeclared identifier that matches
 # a compiler builtin function.  All extant Clang versions are affected, as of
 # Clang 3.6.0.  Test a builtin known to every version.  This problem affects the
@@ -15952,6 +15959,62 @@ case $ac_cv_c_decl_report in
   *) ac_c_decl_warn_flag= ;;
 esac

+ac_fn_c_check_decl "$LINENO" "sigwait" "ac_cv_have_decl_sigwait" "#include <signal.h>
+"
+if test "x$ac_cv_have_decl_sigwait" = xyes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_SIGWAIT $ac_have_decl
+_ACEOF
+
+if test "x$ac_cv_have_decl_sigwait" = xyes; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for POSIX-conforming sigwait declaration" >&5
+$as_echo_n "checking for POSIX-conforming sigwait declaration... " >&6; }
+if ${pgac_cv_have_posix_decl_sigwait+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+      #include <signal.h>
+      int sigwait(const sigset_t *set, int *sig);
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_have_posix_decl_sigwait=yes
+else
+  pgac_cv_have_posix_decl_sigwait=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_have_posix_decl_sigwait" >&5
+$as_echo "$pgac_cv_have_posix_decl_sigwait" >&6; }
+fi
+if test "x$pgac_cv_have_posix_decl_sigwait" = xyes; then
+
+$as_echo "#define HAVE_POSIX_DECL_SIGWAIT 1" >>confdefs.h
+
+else
+  # On non-Windows, libpq requires POSIX sigwait() for thread safety.
+  if test "$enable_thread_safety" = yes -a "$PORTNAME" != "win32"; then
+    as_fn_error $? "POSIX-conforming sigwait is required to enable thread safety." "$LINENO" 5
+  fi
+fi
+
+# posix_fadvise() is a no-op on Solaris, so don't incur function overhead
+# by calling it, 2009-04-02
+# http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/port/gen/posix_fadvise.c
 if test "$PORTNAME" != "solaris"; then :

 for ac_func in posix_fadvise
diff --git a/configure.ac b/configure.ac
index 57336e1fb6..099c4a5a45 100644
--- a/configure.ac
+++ b/configure.ac
@@ -610,6 +610,12 @@ if test "$enable_profiling" = yes && test "$ac_cv_prog_cc_g" = yes; then
   fi
 fi

+# On Solaris, we need this #define to get POSIX-conforming versions
+# of many interfaces (sigwait, getpwuid_r, ...).
+if test "$PORTNAME" = "solaris"; then
+  CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"
+fi
+
 # We already have this in Makefile.win32, but configure needs it too
 if test "$PORTNAME" = "win32"; then
   CPPFLAGS="$CPPFLAGS -I$srcdir/src/include/port/win32"
@@ -1122,9 +1128,8 @@ AS_IF([test "$enable_thread_safety" = yes -a "$PORTNAME" != "win32"],
 AX_PTHREAD    # set thread flags

 # Some platforms use these, so just define them.  They can't hurt if they
-# are not supported.  For example, on Solaris -D_POSIX_PTHREAD_SEMANTICS
-# enables 5-arg getpwuid_r, among other things.
-PTHREAD_CFLAGS="$PTHREAD_CFLAGS -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS"
+# are not supported.
+PTHREAD_CFLAGS="$PTHREAD_CFLAGS -D_REENTRANT -D_THREAD_SAFE"

 # Check for *_r functions
 _CFLAGS="$CFLAGS"
@@ -1741,6 +1746,33 @@ PGAC_CHECK_BUILTIN_FUNC([__builtin_popcount], [unsigned int x])
 # in case it finds that _LARGEFILE_SOURCE has to be #define'd for that.
 AC_FUNC_FSEEKO

+# Make sure there's a declaration for sigwait(), then make sure
+# that it conforms to the POSIX standard (there seem to still be
+# some platforms out there with pre-POSIX sigwait()).  On Solaris,
+# _POSIX_PTHREAD_SEMANTICS affects the result, but we already
+# added that to CPPFLAGS.
+AC_CHECK_DECLS(sigwait, [], [], [#include <signal.h>])
+if test "x$ac_cv_have_decl_sigwait" = xyes; then
+  AC_CACHE_CHECK([for POSIX-conforming sigwait declaration],
+    [pgac_cv_have_posix_decl_sigwait],
+    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
+      #include <signal.h>
+      int sigwait(const sigset_t *set, int *sig);
+      ],
+      [])],
+      [pgac_cv_have_posix_decl_sigwait=yes],
+      [pgac_cv_have_posix_decl_sigwait=no])])
+fi
+if test "x$pgac_cv_have_posix_decl_sigwait" = xyes; then
+  AC_DEFINE(HAVE_POSIX_DECL_SIGWAIT, 1,
+            [Define to 1 if you have a POSIX-conforming sigwait declaration.])
+else
+  # On non-Windows, libpq requires POSIX sigwait() for thread safety.
+  if test "$enable_thread_safety" = yes -a "$PORTNAME" != "win32"; then
+    AC_MSG_ERROR([POSIX-conforming sigwait is required to enable thread safety.])
+  fi
+fi
+
 # posix_fadvise() is a no-op on Solaris, so don't incur function overhead
 # by calling it, 2009-04-02
 # http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/port/gen/posix_fadvise.c
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index d704c4220c..49d4c0e3ce 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4899,7 +4899,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
     FILE       *pagerpipe = NULL;
     int            title_len;
     int            res = 0;
-#ifndef WIN32
+#ifdef HAVE_POSIX_DECL_SIGWAIT
     sigset_t    sigalrm_sigchld_sigint;
     sigset_t    sigalrm_sigchld;
     sigset_t    sigint;
@@ -4913,7 +4913,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
         return false;
     }

-#ifndef WIN32
+#ifdef HAVE_POSIX_DECL_SIGWAIT
     sigemptyset(&sigalrm_sigchld_sigint);
     sigaddset(&sigalrm_sigchld_sigint, SIGCHLD);
     sigaddset(&sigalrm_sigchld_sigint, SIGALRM);
@@ -4952,7 +4952,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
      * PAGER environment variables, because traditional pagers probably won't
      * be very useful for showing a stream of results.
      */
-#ifndef WIN32
+#ifdef HAVE_POSIX_DECL_SIGWAIT
     pagerprog = getenv("PSQL_WATCH_PAGER");
 #endif
     if (pagerprog && myopt.topt.pager)
@@ -5023,7 +5023,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
         if (pagerpipe && ferror(pagerpipe))
             break;

-#ifdef WIN32
+#ifndef HAVE_POSIX_DECL_SIGWAIT

         /*
          * Set up cancellation of 'watch' via SIGINT.  We redo this each time
@@ -5059,7 +5059,8 @@ do_watch(PQExpBuffer query_buf, double sleep)
         {
             int            signal_received;

-            if (sigwait(&sigalrm_sigchld_sigint, &signal_received) < 0)
+            errno = sigwait(&sigalrm_sigchld_sigint, &signal_received);
+            if (errno != 0)
             {
                 /* Some other signal arrived? */
                 if (errno == EINTR)
@@ -5091,7 +5092,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
         restore_sigpipe_trap();
     }

-#ifndef WIN32
+#ifdef HAVE_POSIX_DECL_SIGWAIT
     /* Disable the interval timer. */
     memset(&interval, 0, sizeof(interval));
     setitimer(ITIMER_REAL, &interval, NULL);
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5f36f0d1c6..2931530f33 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -110,7 +110,7 @@ log_locus_callback(const char **filename, uint64 *lineno)
     }
 }

-#ifndef WIN32
+#ifdef HAVE_POSIX_DECL_SIGWAIT
 static void
 empty_signal_handler(SIGNAL_ARGS)
 {
@@ -309,7 +309,7 @@ main(int argc, char *argv[])

     psql_setup_cancel_handler();

-#ifndef WIN32
+#ifdef HAVE_POSIX_DECL_SIGWAIT

     /*
      * do_watch() needs signal handlers installed (otherwise sigwait() will
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index d69d461ff2..15ffdd895a 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -158,6 +158,10 @@
    don't. */
 #undef HAVE_DECL_RTLD_NOW

+/* Define to 1 if you have the declaration of `sigwait', and to 0 if you
+   don't. */
+#undef HAVE_DECL_SIGWAIT
+
 /* Define to 1 if you have the declaration of `strlcat', and to 0 if you
    don't. */
 #undef HAVE_DECL_STRLCAT
@@ -414,6 +418,9 @@
 /* Define to 1 if you have the <poll.h> header file. */
 #undef HAVE_POLL_H

+/* Define to 1 if you have a POSIX-conforming sigwait declaration. */
+#undef HAVE_POSIX_DECL_SIGWAIT
+
 /* Define to 1 if you have the `posix_fadvise' function. */
 #undef HAVE_POSIX_FADVISE

diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 294b968dcd..c967743467 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -249,6 +249,7 @@ sub GenerateFiles
         HAVE_DECL_PWRITEV                           => 0,
         HAVE_DECL_RTLD_GLOBAL                       => 0,
         HAVE_DECL_RTLD_NOW                          => 0,
+        HAVE_DECL_SIGWAIT                           => 0,
         HAVE_DECL_STRLCAT                           => undef,
         HAVE_DECL_STRLCPY                           => undef,
         HAVE_DECL_STRNLEN                           => 1,
@@ -332,6 +333,7 @@ sub GenerateFiles
         HAVE_PAM_PAM_APPL_H         => undef,
         HAVE_POLL                   => undef,
         HAVE_POLL_H                 => undef,
+        HAVE_POSIX_DECL_SIGWAIT     => undef,
         HAVE_POSIX_FADVISE          => undef,
         HAVE_POSIX_FALLOCATE        => undef,
         HAVE_PPC_LWARX_MUTEX_HINT   => undef,