Re: pgbench: option delaying queries till connections establishment?
Hello Dave, I am running pgbench with the following pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench --protocol=simple Without pgbouncer I get around 5k TPS with pgbouncer I get around 15k TPS Looking at the code connection initiation time should not be part of the calculation so I' puzzled why pgbouncer is making such a dramatic difference ? Turns out that for this specific test, pg is faster with a pooler. This does not tell "why". Does the pooler prepares statements, whereas "simple" does not? -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
> > I've recently run into something I am having difficulty understanding. > > I am running pgbench with the following > pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench > --protocol=simple > > Without pgbouncer I get around 5k TPS > with pgbouncer I get around 15k TPS > > Looking at the code connection initiation time should not be part of the > calculation so I' puzzled why pgbouncer is making such a dramatic > difference ? > > Dave > Turns out that for this specific test, pg is faster with a pooler. Dave Cramer, [May 16, 2023 at 9:49:24 AM]: turns out having a connection pool helps. First run is without a pool, second with pgbench=# select mean_exec_time, stddev_exec_time, calls, total_exec_time, min_exec_time, max_exec_time from pg_stat_statements where queryid=-531095336438083412; mean_exec_time | stddev_exec_time | calls | total_exec_time | min_exec_time | max_exec_time ++---+---+--+--- 0.46726998 | 2.2758508661446535 | 200 | 93.453997 | 0.0466160005 | 17.434766 (1 row) pgbench=# select pg_stat_statements_reset(); pg_stat_statements_reset -- (1 row) pgbench=# select mean_exec_time, stddev_exec_time, calls, total_exec_time, min_exec_time, max_exec_time from pg_stat_statements where queryid=-531095336438083412; mean_exec_time| stddev_exec_time | calls | total_exec_time | min_exec_time | max_exec_time -+--+---++---+--- 0.066401864 | 0.021800404695481574 | 200 | 13.2803734 | 0.034006 | 0.226696 (1 row)
Re: pgbench: option delaying queries till connections establishment?
Dave Cramer www.postgres.rocks On Tue, 16 May 2023 at 07:27, Andres Freund wrote: > Hi, > > I am trying to run a few benchmarks measuring the effects of patch to > make GetSnapshotData() faster in the face of larger numbers of > established connections. > > Before the patch connection establishment often is very slow due to > contention. The first few connections are fast, but after that it takes > increasingly long. The first few connections constantly hold > ProcArrayLock in shared mode, which then makes it hard for new > connections to acquire it exclusively (I'm addressing that to a > significant degree in the patch FWIW). > > But for a fair comparison of the runtime effects I'd like to only > compare the throughput for when connections are actually usable, > otherwise I end up benchmarking few vs many connections, which is not > useful. And because I'd like to run the numbers for a lot of different > numbers of connections etc, I can't just make each run several hour > longs to make the initial minutes not matter much. > > Therefore I'd like to make pgbench wait till it has established all > connections, before they run queries. > > Does anybody else see this as being useful? > > If so, should this be done unconditionally? A new option? Included in an > existing one somehow? > > Greetings, Andres Freund > I've recently run into something I am having difficulty understanding. I am running pgbench with the following pgbench -h localhost -c 100 -j 100 -t 2 -S -s 1000 pgbench -U pgbench --protocol=simple Without pgbouncer I get around 5k TPS with pgbouncer I get around 15k TPS Looking at the code connection initiation time should not be part of the calculation so I' puzzled why pgbouncer is making such a dramatic difference ? Dave
Re: pgbench: option delaying queries till connections establishment?
Hello Thomas, I must say that I'm not a big fan of the macro-based all-in-capitals API for threads because it exposes some platform specific uglyness (eg THREAD_FUNC_CC) and it does not look much like clean C code when used. I liked the previous partial pthread implementation better, even if it was not the real thing, obviously. But we were using macros already, to support --disable-thread-safety builds. Yep, but the look and feel was still C code. I just changed them to upper case and dropped the 'p', because I didn't like to pretend to do POSIX threads, but do it so badly. Hmmm. From the source code point of view it was just like actually using posix threads, even if the underlying machinery was not quite that on some systems. I value looking at "beautiful" and "standard" code if possible, even if there is some cheating involved, compared to exposing macros. I made some effort to remove the pretty ugly and inefficient INSTR_TIME macros from pgbench, replaced with straightforward arithmetic and inlined functions. Now some other macros just crept back in:-) Anyway, this is just "les goûts et les couleurs" (just a matter of taste), as we say here. Perhaps we should drop --disable-thread-safety soon, and perhaps it is nearly time to create a good thread abstraction in clean C code, for use in the server and here? Then we won't need any ugly macros. +1. ISTM that with the current approach threads are always used on Windows, i.e. pgbench does not comply to "ENABLE_THREAD_SAFETY" configuration on that platform. Not sure whether this is an issue that need to be addressed, though. The idea of that option, as I understand it, is that in ancient times there were Unix systems with no threads (that's of course the reason PostgreSQL is the way it is). I don't think that was ever the case for Windows NT, and we have no build option for that on Windows AFAICS. Ok, fine with me. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
On Sat, Mar 13, 2021 at 9:08 PM Fabien COELHO wrote: > I must say that I'm not a big fan of the macro-based all-in-capitals API > for threads because it exposes some platform specific uglyness (eg > THREAD_FUNC_CC) and it does not look much like clean C code when used. I > liked the previous partial pthread implementation better, even if it was > not the real thing, obviously. But we were using macros already, to support --disable-thread-safety builds. I just changed them to upper case and dropped the 'p', because I didn't like to pretend to do POSIX threads, but do it so badly. Perhaps we should drop --disable-thread-safety soon, and perhaps it is nearly time to create a good thread abstraction in clean C code, for use in the server and here? Then we won't need any ugly macros. > ISTM that with the current approach threads are always used on Windows, > i.e. pgbench does not comply to "ENABLE_THREAD_SAFETY" configuration on > that platform. Not sure whether this is an issue that need to be > addressed, though. The idea of that option, as I understand it, is that in ancient times there were Unix systems with no threads (that's of course the reason PostgreSQL is the way it is). I don't think that was ever the case for Windows NT, and we have no build option for that on Windows AFAICS.
Re: pgbench: option delaying queries till connections establishment?
Hello Thomas, David Rowley kindly tested this for me on Windows and told me how to fix one of the macros that had incorrect error checking on that OS. So here's a new version. I'm planning to commit 0001 and 0002 soon, if there are no objections. 0003 needs some more review. I made a few mostly cosmetic changes, pgindented and pushed all these patches. Thanks a lot for pushing all that, and fixing issues raised by buildfarm animals pretty unexpected and strange failures… I must say that I'm not a big fan of the macro-based all-in-capitals API for threads because it exposes some platform specific uglyness (eg THREAD_FUNC_CC) and it does not look much like clean C code when used. I liked the previous partial pthread implementation better, even if it was not the real thing, obviously. ISTM that with the current approach threads are always used on Windows, i.e. pgbench does not comply to "ENABLE_THREAD_SAFETY" configuration on that platform. Not sure whether this is an issue that need to be addressed, though. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Thomas Munro writes: > On Sat, Mar 13, 2021 at 4:59 PM Tom Lane wrote: >> Looks reasonable by eyeball. If you'd push it, I can launch >> a gaur run right away. > Done. gaur's gotten through "make" and "make check" cleanly. Unfortunately I expect it will fail at the pg_amcheck test before it reaches pgbench. But for the moment it's reasonable to assume we're good here. Thanks! regards, tom lane
Re: pgbench: option delaying queries till connections establishment?
On Sat, Mar 13, 2021 at 4:59 PM Tom Lane wrote: > Thomas Munro writes: > > Thanks. This seems to work for me on a Mac. I confirmed with nm that > > we don't define or reference any pthread_XXX symbols with > > --disable-thread-safety, and we do otherwise, and the pgbench tests > > pass either way. > > Looks reasonable by eyeball. If you'd push it, I can launch > a gaur run right away. Done.
Re: pgbench: option delaying queries till connections establishment?
Thomas Munro writes: > Thanks. This seems to work for me on a Mac. I confirmed with nm that > we don't define or reference any pthread_XXX symbols with > --disable-thread-safety, and we do otherwise, and the pgbench tests > pass either way. Looks reasonable by eyeball. If you'd push it, I can launch a gaur run right away. regards, tom lane
Re: pgbench: option delaying queries till connections establishment?
On Sat, Mar 13, 2021 at 4:09 PM Tom Lane wrote: > OK, cool. I don't think it's hard, just do > > if test "$enable_thread_safety" = yes; then > AC_REPLACE_FUNCS(pthread_barrier_wait) > fi > > Probably this check should be likewise conditional: > > AC_SEARCH_LIBS(pthread_barrier_wait, pthread) Thanks. This seems to work for me on a Mac. I confirmed with nm that we don't define or reference any pthread_XXX symbols with --disable-thread-safety, and we do otherwise, and the pgbench tests pass either way. 0001-Fix-new-pthread-code-to-respect-disable-thread-safet.patch Description: Binary data
Re: pgbench: option delaying queries till connections establishment?
Thomas Munro writes: > On Sat, Mar 13, 2021 at 3:47 PM Tom Lane wrote: >> Was any thought given to being able to opt out of this patchset >> to support that configure option? > Oops. The pgbench code was tested under --disable-thread-safety, but > it didn't occur to me that the AC_REPLACE_FUNCS search for > pthread_barrier_wait should also be conditional on that; I will now go > and try to figure out how to do that. OK, cool. I don't think it's hard, just do if test "$enable_thread_safety" = yes; then AC_REPLACE_FUNCS(pthread_barrier_wait) fi Probably this check should be likewise conditional: AC_SEARCH_LIBS(pthread_barrier_wait, pthread) regards, tom lane
Re: pgbench: option delaying queries till connections establishment?
On Sat, Mar 13, 2021 at 3:47 PM Tom Lane wrote: > Checking the man pages, it seems that this ancient HPUX version > is using some pre-POSIX API spec in which pthread_cond_init takes a > pthread_condattr_t rather than a pointer to pthread_condattr_t. > Similarly for pthread_mutex_init. Wow. > While it's likely that we could work around that, it's my > opinion that we shouldn't have to, because gaur is building with > --disable-thread-safety. If that switch has any meaning at all, > it should be that we don't try to use thread infrastructure. > Was any thought given to being able to opt out of this patchset > to support that configure option? Oops. The pgbench code was tested under --disable-thread-safety, but it didn't occur to me that the AC_REPLACE_FUNCS search for pthread_barrier_wait should also be conditional on that; I will now go and try to figure out how to do that.
Re: pgbench: option delaying queries till connections establishment?
Thomas Munro writes: > On Mon, Mar 8, 2021 at 3:18 PM Thomas Munro wrote: >> David Rowley kindly tested this for me on Windows and told me how to >> fix one of the macros that had incorrect error checking on that OS. >> So here's a new version. I'm planning to commit 0001 and 0002 soon, >> if there are no objections. 0003 needs some more review. > I made a few mostly cosmetic changes, pgindented and pushed all these patches. So, gaur is not too happy with this: ccache gcc -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -I../../src/port -DFRONTEND -I../../src/include -D_USE_CTYPE_MACROS -D_XOPEN_SOURCE_EXTENDED -I/usr/local/libxml2-2.6.23/include/libxml2 -I/usr/local/ssl-1.0.1e/include -c -o strlcat.o strlcat.c pthread_barrier_wait.c: In function 'pthread_barrier_init': pthread_barrier_wait.c:24:2: error: incompatible type for argument 2 of 'pthread_cond_init' /usr/include/pthread.h:378:5: note: expected 'pthread_condattr_t' but argument is of type 'void *' pthread_barrier_wait.c:26:2: error: incompatible type for argument 2 of 'pthread_mutex_init' /usr/include/pthread.h:354:5: note: expected 'pthread_mutexattr_t' but argument is of type 'void *' make[2]: *** [pthread_barrier_wait.o] Error 1 Checking the man pages, it seems that this ancient HPUX version is using some pre-POSIX API spec in which pthread_cond_init takes a pthread_condattr_t rather than a pointer to pthread_condattr_t. Similarly for pthread_mutex_init. While it's likely that we could work around that, it's my opinion that we shouldn't have to, because gaur is building with --disable-thread-safety. If that switch has any meaning at all, it should be that we don't try to use thread infrastructure. Was any thought given to being able to opt out of this patchset to support that configure option? regards, tom lane
Re: pgbench: option delaying queries till connections establishment?
On Mon, Mar 8, 2021 at 3:18 PM Thomas Munro wrote: > David Rowley kindly tested this for me on Windows and told me how to > fix one of the macros that had incorrect error checking on that OS. > So here's a new version. I'm planning to commit 0001 and 0002 soon, > if there are no objections. 0003 needs some more review. I made a few mostly cosmetic changes, pgindented and pushed all these patches.
Re: pgbench: option delaying queries till connections establishment?
On Fri, Mar 5, 2021 at 6:22 PM Thomas Munro wrote:
> On Thu, Mar 4, 2021 at 10:44 PM Thomas Munro wrote:
> > v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch
>
> Here's a better version of that part. I don't yet know if it actually
> works on Windows...
David Rowley kindly tested this for me on Windows and told me how to
fix one of the macros that had incorrect error checking on that OS.
So here's a new version. I'm planning to commit 0001 and 0002 soon,
if there are no objections. 0003 needs some more review.
From 1aa4ca844b90d76c08f806505bf5350674e701e4 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Sat, 2 Jan 2021 15:05:06 +1300
Subject: [PATCH v12 1/4] Add missing pthread_barrier_t.
Supply a simple implementation of the missing pthread_barrier_t type and
functions, for macOS.
Discussion: https://postgr.es/m/20200227180100.zyvjwzcpiokfsqm2%40alap3.anarazel.de
---
configure | 69 +
configure.ac| 2 +
src/include/pg_config.h.in | 3 ++
src/include/port/pg_pthread.h | 41
src/port/pthread_barrier_wait.c | 66 +++
src/tools/msvc/Solution.pm | 1 +
6 files changed, 182 insertions(+)
create mode 100644 src/include/port/pg_pthread.h
create mode 100644 src/port/pthread_barrier_wait.c
diff --git a/configure b/configure
index ce9ea36999..fad817bb38 100755
--- a/configure
+++ b/configure
@@ -11635,6 +11635,62 @@ if test "$ac_res" != no; then :
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing pthread_barrier_wait" >&5
+$as_echo_n "checking for library containing pthread_barrier_wait... " >&6; }
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+/* Override any GCC internal prototype to avoid an error.
+ Use char because int might match the return type of a GCC
+ builtin and then its argument prototype would still apply. */
+#ifdef __cplusplus
+extern "C"
+#endif
+char pthread_barrier_wait ();
+int
+main ()
+{
+return pthread_barrier_wait ();
+ ;
+ return 0;
+}
+_ACEOF
+for ac_lib in '' pthread; do
+ if test -z "$ac_lib"; then
+ac_res="none required"
+ else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib $ac_func_search_save_LIBS"
+ fi
+ if ac_fn_c_try_link "$LINENO"; then :
+ ac_cv_search_pthread_barrier_wait=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+ if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ break
+fi
+done
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+
+else
+ ac_cv_search_pthread_barrier_wait=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_pthread_barrier_wait" >&5
+$as_echo "$ac_cv_search_pthread_barrier_wait" >&6; }
+ac_res=$ac_cv_search_pthread_barrier_wait
+if test "$ac_res" != no; then :
+ test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
+
+fi
+
# Solaris:
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing fdatasync" >&5
$as_echo_n "checking for library containing fdatasync... " >&6; }
@@ -15883,6 +15939,19 @@ esac
fi
+ac_fn_c_check_func "$LINENO" "pthread_barrier_wait" "ac_cv_func_pthread_barrier_wait"
+if test "x$ac_cv_func_pthread_barrier_wait" = xyes; then :
+ $as_echo "#define HAVE_PTHREAD_BARRIER_WAIT 1" >>confdefs.h
+
+else
+ case " $LIBOBJS " in
+ *" pthread_barrier_wait.$ac_objext "* ) ;;
+ *) LIBOBJS="$LIBOBJS pthread_barrier_wait.$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
diff --git a/configure.ac b/configure.ac
index f54f65febe..0ed53571dd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1143,6 +1143,7 @@ AC_SEARCH_LIBS(getopt_long, [getopt gnugetopt])
AC_SEARCH_LIBS(shm_open, rt)
AC_SEARCH_LIBS(shm_unlink, rt)
AC_SEARCH_LIBS(clock_gettime, [rt posix4])
+AC_SEARCH_LIBS(pthread_barrier_wait, pthread)
# Solaris:
AC_SEARCH_LIBS(fdatasync, [rt posix4])
# Required for thread_test.c on Solaris
@@ -1743,6 +1744,7 @@ AC_REPLACE_FUNCS(m4_normalize([
mkdtemp
pread
preadv
+ pthread_barrier_wait
pwrite
pwritev
random
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 04dc330119..7a7cc21d8d 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -424,6 +424,9 @@
/* Define if you have POSIX threads libraries and header files. */
#undef HAVE_PTHREAD
+/* Define to 1 if you have the `pthread_barrier_wait' function. */
+#undef HAVE_PTHREAD_BARRIER_WAIT
+
/* Define to 1 if you have the `pthread_is_threaded_np' function. */
#undef HAVE_PTHREAD_IS_THREADED_NP
diff --git a/src/include/port/pg_pthread.h b/src/include/port/pg_pthread.h
new file mode 100644
index
Re: pgbench: option delaying queries till connections establishment?
On Thu, Mar 4, 2021 at 10:44 PM Thomas Munro wrote:
> v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch
Here's a better version of that part. I don't yet know if it actually
works on Windows...
From 3aa63dfc086ab1f687ed668091a6bda8bf270fa7 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Sat, 2 Jan 2021 15:05:06 +1300
Subject: [PATCH v11 1/6] Add missing pthread_barrier_t.
Supply a simple implementation of the missing pthread_barrier_t type and
functions, for macOS.
Discussion: https://postgr.es/m/20200227180100.zyvjwzcpiokfsqm2%40alap3.anarazel.de
---
configure | 69 +
configure.ac| 2 +
src/include/pg_config.h.in | 3 ++
src/include/port/pg_pthread.h | 41
src/port/pthread_barrier_wait.c | 66 +++
src/tools/msvc/Solution.pm | 1 +
6 files changed, 182 insertions(+)
create mode 100644 src/include/port/pg_pthread.h
create mode 100644 src/port/pthread_barrier_wait.c
diff --git a/configure b/configure
index ce9ea36999..fad817bb38 100755
--- a/configure
+++ b/configure
@@ -11635,6 +11635,62 @@ if test "$ac_res" != no; then :
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing pthread_barrier_wait" >&5
+$as_echo_n "checking for library containing pthread_barrier_wait... " >&6; }
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+/* Override any GCC internal prototype to avoid an error.
+ Use char because int might match the return type of a GCC
+ builtin and then its argument prototype would still apply. */
+#ifdef __cplusplus
+extern "C"
+#endif
+char pthread_barrier_wait ();
+int
+main ()
+{
+return pthread_barrier_wait ();
+ ;
+ return 0;
+}
+_ACEOF
+for ac_lib in '' pthread; do
+ if test -z "$ac_lib"; then
+ac_res="none required"
+ else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib $ac_func_search_save_LIBS"
+ fi
+ if ac_fn_c_try_link "$LINENO"; then :
+ ac_cv_search_pthread_barrier_wait=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+ if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ break
+fi
+done
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+
+else
+ ac_cv_search_pthread_barrier_wait=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_pthread_barrier_wait" >&5
+$as_echo "$ac_cv_search_pthread_barrier_wait" >&6; }
+ac_res=$ac_cv_search_pthread_barrier_wait
+if test "$ac_res" != no; then :
+ test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
+
+fi
+
# Solaris:
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing fdatasync" >&5
$as_echo_n "checking for library containing fdatasync... " >&6; }
@@ -15883,6 +15939,19 @@ esac
fi
+ac_fn_c_check_func "$LINENO" "pthread_barrier_wait" "ac_cv_func_pthread_barrier_wait"
+if test "x$ac_cv_func_pthread_barrier_wait" = xyes; then :
+ $as_echo "#define HAVE_PTHREAD_BARRIER_WAIT 1" >>confdefs.h
+
+else
+ case " $LIBOBJS " in
+ *" pthread_barrier_wait.$ac_objext "* ) ;;
+ *) LIBOBJS="$LIBOBJS pthread_barrier_wait.$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
diff --git a/configure.ac b/configure.ac
index f54f65febe..0ed53571dd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1143,6 +1143,7 @@ AC_SEARCH_LIBS(getopt_long, [getopt gnugetopt])
AC_SEARCH_LIBS(shm_open, rt)
AC_SEARCH_LIBS(shm_unlink, rt)
AC_SEARCH_LIBS(clock_gettime, [rt posix4])
+AC_SEARCH_LIBS(pthread_barrier_wait, pthread)
# Solaris:
AC_SEARCH_LIBS(fdatasync, [rt posix4])
# Required for thread_test.c on Solaris
@@ -1743,6 +1744,7 @@ AC_REPLACE_FUNCS(m4_normalize([
mkdtemp
pread
preadv
+ pthread_barrier_wait
pwrite
pwritev
random
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 04dc330119..7a7cc21d8d 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -424,6 +424,9 @@
/* Define if you have POSIX threads libraries and header files. */
#undef HAVE_PTHREAD
+/* Define to 1 if you have the `pthread_barrier_wait' function. */
+#undef HAVE_PTHREAD_BARRIER_WAIT
+
/* Define to 1 if you have the `pthread_is_threaded_np' function. */
#undef HAVE_PTHREAD_IS_THREADED_NP
diff --git a/src/include/port/pg_pthread.h b/src/include/port/pg_pthread.h
new file mode 100644
index 00..5222cdce6e
--- /dev/null
+++ b/src/include/port/pg_pthread.h
@@ -0,0 +1,41 @@
+/*-
+ *
+ * Declarations for missing POSIX thread components.
+ *
+ * Currently this supplies an implementation of pthread_barrier_t for the
+ * benefit of
Re: pgbench: option delaying queries till connections establishment?
On Wed, Mar 3, 2021 at 6:23 PM Thomas Munro wrote:
> On Sun, Jan 31, 2021 at 1:18 AM Fabien COELHO wrote:
> > I think it would be much more consistent to move all the thread complement
> > stuff there directly: Currently (v8) the windows implementation is in
> > pgbench and the MacOS implementation in port, which is quite messy.
>
> Hmm. Well this is totally subjective, but here's how I see this after
> thinking about it a bit more: macOS does actually have POSIX threads,
> except for this tiny missing piece, so it's OK to write a toy
> implementation that is reasonably conformant, and put it in there
> using the usual AC_REPLACE_FUNCS machinery. It will go away when
> Apple eventually adds a real one. Windows does not, and here we're
> writing a very partial toy implementation that is far from conformant.
> I think that's OK for pgbench's purposes, for now, but I'd prefer to
> keep it inside pgbench.c. I think at some point in the (hopefully not
> too distant) future, we'll start working on thread support for the
> backend, and then I think we'll probably come up with our own
> abstraction over Windows and POSIX threads, rather than trying to use
> POSIX API wrappers from Windows, so I don't really want this stuff in
> the port library. Does this make some kind of sense?
Here is an attempt to move things in that direction. It compiles
tests OK on Unix including macOS with and without
--disable-thread-safety, and it compiles on Windows (via CI) but I
don't yet know if it works there.
v10-0001-Add-missing-pthread_barrier_t.patch
Same as v8. Adds the missing pthread_barrier_t support for macOS
only. Based on the traditional configure symbol probe for now. It's
possible that we'll later decide to use declarations to be more
future-proof against Apple's API versioning strategy, but I don't have
one of those weird cross-version compiler setups to investigate that
(see complaints from James Hilliard about the way we deal with
pwrite()).
v10-0002-pgbench-Refactor-the-way-we-do-thread-portabilit.patch
New. Abandons the concept of doing a fake pthread API on Windows in
pgbench.c, in favour of a couple of tiny local macros that abstract
over POSIX, Windows and threadless builds. This results in less code,
and also fixes some minor problems I spotted in pre-existing code:
it's not OK to use (pthread_t) 0 as a special value, or to compare
pthread_t values with ==, or to assume that pthread APIs set errno.
v10-0003-pgbench-Improve-time-measurement-code.patch
Your original A patch, rebased over the above. I haven't reviewed
this one. It lacks a commit message.
v10-0004-pgbench-Synchronize-client-threads.patch
Adds in the barriers.
From 1459914c729e1157a932254bf7483f1ef7ac6408 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Sat, 2 Jan 2021 15:05:06 +1300
Subject: [PATCH v10 1/5] Add missing pthread_barrier_t.
Supply a simple implementation of the missing pthread_barrier_t type and
functions, for macOS.
Discussion: https://postgr.es/m/20200227180100.zyvjwzcpiokfsqm2%40alap3.anarazel.de
---
configure | 69 +
configure.ac| 2 +
src/include/pg_config.h.in | 3 ++
src/include/port/pg_pthread.h | 41
src/port/pthread_barrier_wait.c | 66 +++
src/tools/msvc/Solution.pm | 1 +
6 files changed, 182 insertions(+)
create mode 100644 src/include/port/pg_pthread.h
create mode 100644 src/port/pthread_barrier_wait.c
diff --git a/configure b/configure
index ce9ea36999..fad817bb38 100755
--- a/configure
+++ b/configure
@@ -11635,6 +11635,62 @@ if test "$ac_res" != no; then :
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing pthread_barrier_wait" >&5
+$as_echo_n "checking for library containing pthread_barrier_wait... " >&6; }
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+/* Override any GCC internal prototype to avoid an error.
+ Use char because int might match the return type of a GCC
+ builtin and then its argument prototype would still apply. */
+#ifdef __cplusplus
+extern "C"
+#endif
+char pthread_barrier_wait ();
+int
+main ()
+{
+return pthread_barrier_wait ();
+ ;
+ return 0;
+}
+_ACEOF
+for ac_lib in '' pthread; do
+ if test -z "$ac_lib"; then
+ac_res="none required"
+ else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib $ac_func_search_save_LIBS"
+ fi
+ if ac_fn_c_try_link "$LINENO"; then :
+ ac_cv_search_pthread_barrier_wait=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+ if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ break
+fi
+done
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+
+else
+ ac_cv_search_pthread_barrier_wait=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo
Re: pgbench: option delaying queries till connections establishment?
On Sun, Jan 31, 2021 at 1:18 AM Fabien COELHO wrote: > I think it would be much more consistent to move all the thread complement > stuff there directly: Currently (v8) the windows implementation is in > pgbench and the MacOS implementation in port, which is quite messy. Hmm. Well this is totally subjective, but here's how I see this after thinking about it a bit more: macOS does actually have POSIX threads, except for this tiny missing piece, so it's OK to write a toy implementation that is reasonably conformant, and put it in there using the usual AC_REPLACE_FUNCS machinery. It will go away when Apple eventually adds a real one. Windows does not, and here we're writing a very partial toy implementation that is far from conformant. I think that's OK for pgbench's purposes, for now, but I'd prefer to keep it inside pgbench.c. I think at some point in the (hopefully not too distant) future, we'll start working on thread support for the backend, and then I think we'll probably come up with our own abstraction over Windows and POSIX threads, rather than trying to use POSIX API wrappers from Windows, so I don't really want this stuff in the port library. Does this make some kind of sense? > Attached is a patch set which does that. I cannot test it neither on > Windows nor on MacOS. Path 1 & 2 are really independent. No worries. For some reason I have a lot of computers; I'll try to get this (or rather a version with the Windows stuff moved back) passing on all of them soon, with the aim of making it committable.
Re: pgbench: option delaying queries till connections establishment?
Hello Thomas,
3 . Decide if it's sane for the Windows-based emulation to be in here
too, or if it should stay in pgbench.c. Or alternatively, if we're
emulating pthread stuff on Windows, why not also put the other pthread
emulation stuff from pgbench.c into a "ports" file; that seems
premature and overkill for your project. I dunno.
I decided to solve only the macOS problem for now. So in this
version, the A and B patches are exactly as you had them in your v7,
except that B includes “port/pg_pthread.h” instead of .
Maybe it’d make sense to move the Win32 pthread emulation stuff out of
pgbench.c into src/port too (the pre-existing stuff, and the new
barrier stuff you added), but that seems like a separate patch, one
that I’m not best placed to write, and it’s not clear to me that we’ll
want to be using pthread APIs as our main abstraction if/when thread
usage increases in the PG source tree anyway. Other opinions welcome.
I think it would be much more consistent to move all the thread complement
stuff there directly: Currently (v8) the windows implementation is in
pgbench and the MacOS implementation in port, which is quite messy.
Attached is a patch set which does that. I cannot test it neither on
Windows nor on MacOS. Path 1 & 2 are really independent.
--
Fabien.diff --git a/configure b/configure
index e202697bbf..54c5a7963f 100755
--- a/configure
+++ b/configure
@@ -11668,6 +11668,62 @@ if test "$ac_res" != no; then :
fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing pthread_barrier_wait" >&5
+$as_echo_n "checking for library containing pthread_barrier_wait... " >&6; }
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+
+/* Override any GCC internal prototype to avoid an error.
+ Use char because int might match the return type of a GCC
+ builtin and then its argument prototype would still apply. */
+#ifdef __cplusplus
+extern "C"
+#endif
+char pthread_barrier_wait ();
+int
+main ()
+{
+return pthread_barrier_wait ();
+ ;
+ return 0;
+}
+_ACEOF
+for ac_lib in '' pthread; do
+ if test -z "$ac_lib"; then
+ac_res="none required"
+ else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib $ac_func_search_save_LIBS"
+ fi
+ if ac_fn_c_try_link "$LINENO"; then :
+ ac_cv_search_pthread_barrier_wait=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+ if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+ break
+fi
+done
+if ${ac_cv_search_pthread_barrier_wait+:} false; then :
+
+else
+ ac_cv_search_pthread_barrier_wait=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_pthread_barrier_wait" >&5
+$as_echo "$ac_cv_search_pthread_barrier_wait" >&6; }
+ac_res=$ac_cv_search_pthread_barrier_wait
+if test "$ac_res" != no; then :
+ test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
+
+fi
+
# Solaris:
{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing fdatasync" >&5
$as_echo_n "checking for library containing fdatasync... " >&6; }
@@ -15853,6 +15909,19 @@ esac
fi
+ac_fn_c_check_func "$LINENO" "pthread_barrier_wait" "ac_cv_func_pthread_barrier_wait"
+if test "x$ac_cv_func_pthread_barrier_wait" = xyes; then :
+ $as_echo "#define HAVE_PTHREAD_BARRIER_WAIT 1" >>confdefs.h
+
+else
+ case " $LIBOBJS " in
+ *" pthread_barrier_wait.$ac_objext "* ) ;;
+ *) LIBOBJS="$LIBOBJS pthread_barrier_wait.$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
diff --git a/configure.ac b/configure.ac
index a5ad072ee4..23ad861b27 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1152,6 +1152,7 @@ AC_SEARCH_LIBS(getopt_long, [getopt gnugetopt])
AC_SEARCH_LIBS(shm_open, rt)
AC_SEARCH_LIBS(shm_unlink, rt)
AC_SEARCH_LIBS(clock_gettime, [rt posix4])
+AC_SEARCH_LIBS(pthread_barrier_wait, pthread)
# Solaris:
AC_SEARCH_LIBS(fdatasync, [rt posix4])
# Required for thread_test.c on Solaris
@@ -1734,6 +1735,7 @@ AC_REPLACE_FUNCS(m4_normalize([
mkdtemp
pread
preadv
+ pthread_barrier_wait
pwrite
pwritev
random
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index a4a3f40048..0ac0e186ea 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -66,6 +66,7 @@
#include "libpq-fe.h"
#include "pgbench.h"
#include "portability/instr_time.h"
+#include "port/pg_pthread.h"
#ifndef M_PI
#define M_PI 3.14159265358979323846
@@ -109,26 +110,6 @@ typedef struct socket_set
#endif /* POLL_USING_SELECT */
-/*
- * Multi-platform pthread implementations
- */
-
-#ifdef WIN32
-/* Use native win32 threads on Windows */
-typedef struct win32_pthread *pthread_t;
-typedef int pthread_attr_t;
-
-static int
Re: pgbench: option delaying queries till connections establishment?
On Sat, Jan 9, 2021 at 8:13 AM Thomas Munro wrote: > On Sun, Jan 3, 2021 at 9:49 AM Fabien COELHO wrote: > > > Just for fun, the attached 0002 patch is a quick prototype of a > > > replacement for that stuff that seems to work OK on a Mac here. (I'm > > > not sure if the Windows part makes sense or works.) > > > > Thanks! That will definitely help because I do not have a Mac. I'll do > > some cleanup. > > I think the main things to clean up are: I’m supposed to be on vacation this week, but someone left a shiny new Arm Mac laptop near me, so here’s a cleaned up version. > 1. pthread_barrier_init() should check for errors from > pthread_cond_init() and pthread_mutex_init(), and return -1. Done. > 2. pthread_barrier_destroy() should call pthread_cond_destroy() and > pthread_mutex_destroy(). Done. > 3 . Decide if it's sane for the Windows-based emulation to be in here > too, or if it should stay in pgbench.c. Or alternatively, if we're > emulating pthread stuff on Windows, why not also put the other pthread > emulation stuff from pgbench.c into a "ports" file; that seems > premature and overkill for your project. I dunno. I decided to solve only the macOS problem for now. So in this version, the A and B patches are exactly as you had them in your v7, except that B includes “port/pg_pthread.h” instead of . Maybe it’d make sense to move the Win32 pthread emulation stuff out of pgbench.c into src/port too (the pre-existing stuff, and the new barrier stuff you added), but that seems like a separate patch, one that I’m not best placed to write, and it’s not clear to me that we’ll want to be using pthread APIs as our main abstraction if/when thread usage increases in the PG source tree anyway. Other opinions welcome. > 4. cfbot shows that it's not building on Windows because > HAVE_PTHREAD_BARRIER_WAIT is missing from Solution.pm. Fixed, I think. v8-0001-A.patch Description: Binary data v8-0002-Add-missing-pthread_barrier_t.patch Description: Binary data v8-0003-B.patch Description: Binary data
Re: pgbench: option delaying queries till connections establishment?
On Sun, Jan 3, 2021 at 9:49 AM Fabien COELHO wrote: > > Just for fun, the attached 0002 patch is a quick prototype of a > > replacement for that stuff that seems to work OK on a Mac here. (I'm > > not sure if the Windows part makes sense or works.) > > Thanks! That will definitely help because I do not have a Mac. I'll do > some cleanup. I think the main things to clean up are: 1. pthread_barrier_init() should check for errors from pthread_cond_init() and pthread_mutex_init(), and return -1. 2. pthread_barrier_destroy() should call pthread_cond_destroy() and pthread_mutex_destroy(). 3 . Decide if it's sane for the Windows-based emulation to be in here too, or if it should stay in pgbench.c. Or alternatively, if we're emulating pthread stuff on Windows, why not also put the other pthread emulation stuff from pgbench.c into a "ports" file; that seems premature and overkill for your project. I dunno. 4. cfbot shows that it's not building on Windows because HAVE_PTHREAD_BARRIER_WAIT is missing from Solution.pm. As far as I know, it's OK and common practice to ignore the return code from eg pthread_mutex_lock() and pthread_cond_wait(), with rationale along the lines that there'd have to be a programming error for them to fail in simple cases. Unfortunately, cfbot can only tell us that it's building OK on a Mac, but doesn't actually run the pgbench code to reach this stuff. It's not running check-world on that platform yet for the following asinine reason: connection to database failed: Unix-domain socket path "/private/var/folders/3y/l0z1x3693dl_8n0qybp4dqwhgn/T/cirrus-ci-build/src/bin/pg_upgrade/.s.PGSQL.58080" is too long (maximum 103 bytes)
Re: pgbench: option delaying queries till connections establishment?
It looks like macOS doesn't have pthread barriers (via cfbot 2021, now with more operating systems): Indeed:-( I'll look into that. Just for fun, the attached 0002 patch is a quick prototype of a replacement for that stuff that seems to work OK on a Mac here. (I'm not sure if the Windows part makes sense or works.) Thanks! That will definitely help because I do not have a Mac. I'll do some cleanup. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
It looks like macOS doesn't have pthread barriers (via cfbot 2021, now with more operating systems): Indeed:-( I'll look into that. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
On Sun, Nov 15, 2020 at 4:53 AM Fabien COELHO wrote: > In the attached version, I just comment out the call and add an > explanation about why it is commented out. If pg overall version > requirements are changed on windows, then it could be reinstated. It looks like macOS doesn't have pthread barriers (via cfbot 2021, now with more operating systems): pgbench.c:326:8: error: unknown type name 'pthread_barrier_t' static pthread_barrier_t barrier; ^ pgbench.c:6128:2: error: implicit declaration of function 'pthread_barrier_init' is invalid in C99 [-Werror,-Wimplicit-function-declaration] pthread_barrier_init(, NULL, nthreads); ^
Re: pgbench: option delaying queries till connections establishment?
I think the issue really is that, independent of PG lock contention, it'll take a while to establish all connections, and that starting to benchmark with only some connections established will create pretty pointless numbers. Yes. This is why I think that if we have some way to synchronize it should always be used, i.e. not an option. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Hi, On 2020-11-17 00:09:34 +0300, Marina Polyakova wrote: > Sorry I'm not familiar with the internal architecture of snapshots, locks > etc. in postgres, but I wanted to ask - what exact kind of patch for fair > lwlocks do you want to offer to the community? I applied the 6-th version of > the patch for fair lwlocks from [1] to the old master branch (see commit > [2]), started many threads in pgbench (-M prepared -c 1000 -j 500 -T 10 -P1 > -S) and I did not receive stable first progress reports, which IIUC are one > of the advantages of the discussed patch for the pgbench (see [3])... Thanks for running some benchmarks. I think it's quite unsurprising that you'd see skewed numbers initially, even with fairer locks. Just by virtue of pgbench starting threads and each thread immediately starting to perform work, you are bound to get odd pretty meaningless initial numbers. Even without contention, and when using fewer connections than there are CPUs. And especially when starting a larger number of connections, because the main pgbench thread will get fewer and fewer scheduler slices because of the threads running benchmarks already. Regards, Andres
Re: pgbench: option delaying queries till connections establishment?
Hi, On 2020-11-14 20:07:38 +0300, Alexander Korotkov wrote: > Hmm... Let's see the big picture. You've recently committed a > patchset, which greatly improved the performance of GetSnapshotData(). > And you're making further improvements in this direction. But you're > getting trouble in measuring the effect, because Postgres is still > stuck on ProcArrayLock. No, the problem was that I couldn't measure the before/after behaviour reliably, because not all connections actually ever get established *before* the GetSnapshotData() scability patchset. Which made the numbers pointless, because we'd often end up with e.g. 80 connections doing work pre-patch, and 800 post-patch; which obviously measures very different things. I think the issue really is that, independent of PG lock contention, it'll take a while to establish all connections, and that starting to benchmark with only some connections established will create pretty pointless numbers. > And in this thread you propose a workaround > for that implemented on the pgbench side. My very dumb idea is > following: should we finally give a chance to more fair lwlocks rather > than inventing workarounds? Perhaps - I just don't think it's related to this thread. And how you're going to address the overhead. Greetings, Andres Freund
Re: pgbench: option delaying queries till connections establishment?
Hello! On 2020-11-14 20:07, Alexander Korotkov wrote: Hmm... Let's see the big picture. You've recently committed a patchset, which greatly improved the performance of GetSnapshotData(). And you're making further improvements in this direction. But you're getting trouble in measuring the effect, because Postgres is still stuck on ProcArrayLock. And in this thread you propose a workaround for that implemented on the pgbench side. My very dumb idea is following: should we finally give a chance to more fair lwlocks rather than inventing workarounds? As I remember, your major argument against more fair lwlocks was the idea that we should fix lwlocks use-cases rather than lwlock mechanism themselves. But can we expect that we fix all the lwlocks use-case in any reasonable prospect? My guess is 'no'. Links 1. https://www.postgresql.org/message-id/CAPpHfdvJhO1qutziOp%3Ddy8TO8Xb4L38BxgKG4RPa1up1Lzh_UQ%40mail.gmail.com Sorry I'm not familiar with the internal architecture of snapshots, locks etc. in postgres, but I wanted to ask - what exact kind of patch for fair lwlocks do you want to offer to the community? I applied the 6-th version of the patch for fair lwlocks from [1] to the old master branch (see commit [2]), started many threads in pgbench (-M prepared -c 1000 -j 500 -T 10 -P1 -S) and I did not receive stable first progress reports, which IIUC are one of the advantages of the discussed patch for the pgbench (see [3])... [1] https://www.postgresql.org/message-id/CAPpHfduV3v3EG7K74-9htBZz_mpE993zGz-%3D2k5RNA3tqabUAA%40mail.gmail.com [2] https://github.com/postgres/postgres/commit/84d514887f9ca673ae688d00f8b544e70f1ab270 [3] https://www.postgresql.org/message-id/20200227185129.hikscyenomnlrord%40alap3.anarazel.de -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: pgbench: option delaying queries till connections establishment?
Hi! On Thu, Feb 27, 2020 at 9:01 PM Andres Freund wrote: > I am trying to run a few benchmarks measuring the effects of patch to > make GetSnapshotData() faster in the face of larger numbers of > established connections. > > Before the patch connection establishment often is very slow due to > contention. The first few connections are fast, but after that it takes > increasingly long. The first few connections constantly hold > ProcArrayLock in shared mode, which then makes it hard for new > connections to acquire it exclusively (I'm addressing that to a > significant degree in the patch FWIW). Hmm... Let's see the big picture. You've recently committed a patchset, which greatly improved the performance of GetSnapshotData(). And you're making further improvements in this direction. But you're getting trouble in measuring the effect, because Postgres is still stuck on ProcArrayLock. And in this thread you propose a workaround for that implemented on the pgbench side. My very dumb idea is following: should we finally give a chance to more fair lwlocks rather than inventing workarounds? As I remember, your major argument against more fair lwlocks was the idea that we should fix lwlocks use-cases rather than lwlock mechanism themselves. But can we expect that we fix all the lwlocks use-case in any reasonable prospect? My guess is 'no'. Links 1. https://www.postgresql.org/message-id/CAPpHfdvJhO1qutziOp%3Ddy8TO8Xb4L38BxgKG4RPa1up1Lzh_UQ%40mail.gmail.com -- Regards, Alexander Korotkov
Re: pgbench: option delaying queries till connections establishment?
Hello Marina,
1) It looks like pgbench will no longer support Windows XP due to the
function DeleteSynchronizationBarrier. From
https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-deletesynchronizationbarrier
Minimum supported client: Windows 8 [desktop apps only]
Minimum supported server: Windows Server 2012 [desktop apps only]
Thanks for the test and precise analysis!
Sigh.
I do not think that putting such version requirements are worth it just
for the sake of pgbench.
In the attached version, I just comment out the call and add an
explanation about why it is commented out. If pg overall version
requirements are changed on windows, then it could be reinstated.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 7180fedd65..f02721da0d 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -58,8 +58,10 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-tps = 85.184871 (including connections establishing)
-tps = 85.296346 (excluding connections establishing)
+latency average = 11.013 ms
+latency stddev = 7.351 ms
+initial connection time = 45.758 ms
+tps = 896.967014 (without initial connection establishing)
The first six lines report some of the most important parameter
@@ -68,8 +70,7 @@ tps = 85.296346 (excluding connections establishing)
and number of transactions per client); these will be equal unless the run
failed before completion. (In -T mode, only the actual
number of transactions is printed.)
- The last two lines report the number of transactions per second,
- figured with and without counting the time to start database sessions.
+ The last line reports the number of transactions per second.
@@ -2234,22 +2235,22 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-latency average = 15.844 ms
-latency stddev = 2.715 ms
-tps = 618.764555 (including connections establishing)
-tps = 622.977698 (excluding connections establishing)
+latency average = 10.870 ms
+latency stddev = 7.341 ms
+initial connection time = 30.954 ms
+tps = 907.949122 (without initial connection establishing)
statement latencies in milliseconds:
-0.002 \set aid random(1, 10 * :scale)
-0.005 \set bid random(1, 1 * :scale)
-0.002 \set tid random(1, 10 * :scale)
-0.001 \set delta random(-5000, 5000)
-0.326 BEGIN;
-0.603 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-0.454 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
-5.528 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
-7.335 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
-0.371 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
-1.212 END;
+0.001 \set aid random(1, 10 * :scale)
+0.001 \set bid random(1, 1 * :scale)
+0.001 \set tid random(1, 10 * :scale)
+0.000 \set delta random(-5000, 5000)
+0.046 BEGIN;
+0.151 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
+0.107 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
+4.241 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
+5.245 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
+0.102 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
+0.974 END;
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 3057665bbe..b8a3e28690 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -292,9 +292,9 @@ typedef struct SimpleStats
*/
typedef struct StatsData
{
- time_t start_time; /* interval start time, for aggregates */
- int64 cnt; /* number of transactions, including skipped */
- int64 skipped; /* number of transactions skipped under --rate
+ pg_time_usec_t start_time; /* interval start time, for aggregates */
+ int64 cnt; /* number of transactions, including skipped */
+ int64 skipped; /* number of transactions skipped under --rate
* and --latency-limit */
SimpleStats latency;
SimpleStats lag;
@@ -417,11 +417,11 @@ typedef struct
int nvariables; /* number of variables */
bool vars_sorted; /* are variables sorted by name? */
- /* various times about current transaction */
- int64 txn_scheduled; /* scheduled start time of transaction (usec) */
- int64 sleep_until; /* scheduled start time of next cmd (usec) */
- instr_time txn_begin; /* used for measuring schedule lag times */
- instr_time stmt_begin; /* used for measuring statement latencies */
+ /* various times about current transaction
Re: pgbench: option delaying queries till connections establishment?
Hello! On 2020-11-13 08:44, kuroda.hay...@fujitsu.com wrote: Dear Fabien, and this will wait till its time comes. In the mean time, I think that you should put the patch status as you see fit, independently of the other patch: it seems unlikely that they would be committed together, and I'll have to merge the remaining one anyway. OK. I found the related thread[1], and I understood you will submit another patch on the thread. PostgreSQL Patch Tester says all regression tests are passed, and I change the status to "Ready for committer." [1]: https://commitfest.postgresql.org/31/2827/ Thank you for discussing with me. Hayato Kuroda FUJITSU LIMITED From the mentioned thread [2]: While trying to test a patch that adds a synchronization barrier in pgbench [1] on Windows, Thanks for trying that, I do not have a windows setup for testing, and the sync code I wrote for Windows is basically blind coding:-( FYI: 1) It looks like pgbench will no longer support Windows XP due to the function DeleteSynchronizationBarrier. From https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-deletesynchronizationbarrier : Minimum supported client: Windows 8 [desktop apps only] Minimum supported server: Windows Server 2012 [desktop apps only] On Windows Server 2008 R2 (MSVC 2013) the 6-th version of the patch [1] has compiled without (new) warnings, but when running pgbench I got the following error: The procedure entry point DeleteSynchronizationBarrier could not be located in the dynamic link library KERNEL32.dll. IMO, it looks like either old Windows systems should not call new functions, or we should throw them a compilation error. (Update MIN_WINNT to 0x0602 = Windows 8 in src/include/port/win32.h?) In the second case it looks like the documentation should be updated too, see doc/src/sgml/installation.sgml: PostgreSQL can be expected to work on these operating systems: Linux (all recent distributions), Windows (XP and later), FreeBSD, OpenBSD, NetBSD, macOS, AIX, HP/UX, and Solaris. Other Unix-like systems may also work but are not currently being tested. In most cases, all CPU architectures supported by a given operating system will work. Look in below to see if there is information specific to your operating system, particularly if using an older system. <...> The native Windows port requires a 32 or 64-bit version of Windows 2000 or later. Earlier operating systems do not have sufficient infrastructure (but Cygwin may be used on those). MinGW, the Unix-like build tools, and MSYS, a collection of Unix tools required to run shell scripts like configure, can be downloaded from http://www.mingw.org/;>. Neither is required to run the resulting binaries; they are needed only for creating the binaries. [2] https://www.postgresql.org/message-id/e5a09b790db21356376b6e73673aa07c%40postgrespro.ru -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
RE: pgbench: option delaying queries till connections establishment?
Dear Fabien, > and this will wait till its time comes. In the mean time, I think that you > should put the patch status as you see fit, independently of the other > patch: it seems unlikely that they would be committed together, and I'll > have to merge the remaining one anyway. OK. I found the related thread[1], and I understood you will submit another patch on the thread. PostgreSQL Patch Tester says all regression tests are passed, and I change the status to "Ready for committer." [1]: https://commitfest.postgresql.org/31/2827/ Thank you for discussing with me. Hayato Kuroda FUJITSU LIMITED -Original Message- From: Fabien COELHO Sent: Wednesday, November 11, 2020 9:24 PM To: Kuroda, Hayato/黒田 隼人 Cc: Andres Freund ; Daniel Gustafsson ; pgsql-hack...@postgresql.org Subject: RE: pgbench: option delaying queries till connections establishment? Hello, >> I can remove the line, but I strongly believe that reporting performance >> figures if some client connection failed thus the bench could not run as >> prescribed is a bad behavior. The good news is that it is probably quite >> unlikely. So I'd prefer to keep it and possibly submit a patch to change >> the behavior. > > I agree such a situation is very bad, and I understood you have a plan to > submit patches for fix it. If so leaving lines as a TODO is OK. Thanks. >> Should be this one: https://commitfest.postgresql.org/30/2624/ > > This discussion is still on-going, but I can see that the starting time > may be delayed for looking up all pgbench-variables. Yep, that's it. > (I think the status of this thread might be wrong. it should be > 'Needs review,' but now 'Waiting on Author.') I changed it to "Needs review". > This patch is mostly good and can change a review status soon, > however, I think it should wait that related patch. Hmmm. > Please discuss how to fix it with Tom, I would not have the presumption to pressure Tom's agenda in any way! > and this will commit soon. and this will wait till its time comes. In the mean time, I think that you should put the patch status as you see fit, independently of the other patch: it seems unlikely that they would be committed together, and I'll have to merge the remaining one anyway. -- Fabien.
RE: pgbench: option delaying queries till connections establishment?
Hello, I can remove the line, but I strongly believe that reporting performance figures if some client connection failed thus the bench could not run as prescribed is a bad behavior. The good news is that it is probably quite unlikely. So I'd prefer to keep it and possibly submit a patch to change the behavior. I agree such a situation is very bad, and I understood you have a plan to submit patches for fix it. If so leaving lines as a TODO is OK. Thanks. Should be this one: https://commitfest.postgresql.org/30/2624/ This discussion is still on-going, but I can see that the starting time may be delayed for looking up all pgbench-variables. Yep, that's it. (I think the status of this thread might be wrong. it should be 'Needs review,' but now 'Waiting on Author.') I changed it to "Needs review". This patch is mostly good and can change a review status soon, however, I think it should wait that related patch. Hmmm. Please discuss how to fix it with Tom, I would not have the presumption to pressure Tom's agenda in any way! and this will commit soon. and this will wait till its time comes. In the mean time, I think that you should put the patch status as you see fit, independently of the other patch: it seems unlikely that they would be committed together, and I'll have to merge the remaining one anyway. -- Fabien.
RE: pgbench: option delaying queries till connections establishment?
Dear Fabien, > Usually I run many pgbench through scripts, so I'm probably not there to > check a lone stderr failure at the beginning if performance figures are > actually reported. > I can remove the line, but I strongly believe that reporting performance > figures if some client connection failed thus the bench could not run as > prescribed is a bad behavior. The good news is that it is probably quite > unlikely. So I'd prefer to keep it and possibly submit a patch to change > the behavior. I agree such a situation is very bad, and I understood you have a plan to submit patches for fix it. If so leaving lines as a TODO is OK. > Should be this one: https://commitfest.postgresql.org/30/2624/ This discussion is still on-going, but I can see that the starting time may be delayed for looking up all pgbench-variables. (I think the status of this thread might be wrong. it should be 'Needs review,' but now 'Waiting on Author.') This patch is mostly good and can change a review status soon, however, I think it should wait that related patch. Please discuss how to fix it with Tom, and this will commit soon. Hayato Kuroda FUJITSU LIMITED
RE: pgbench: option delaying queries till connections establishment?
Hello, Indeed. I took your next patch with an added explanation. I'm unclear whether proceeding makes much sense though, that is some thread would run the test and other would have aborted. Hmmm. Your comment looks good, thanks. In the previous version pgbench starts benchmarking even if some connections fail. And users can notice the connection failure by stderr output. Hence the current specification may be enough. Usually I run many pgbench through scripts, so I'm probably not there to check a lone stderr failure at the beginning if performance figures are actually reported. If you agree, please remove the following lines: ``` +* It is unclear whether it is worth doing anything rather than +* coldly exiting with an error message. ``` I can remove the line, but I strongly believe that reporting performance figures if some client connection failed thus the bench could not run as prescribed is a bad behavior. The good news is that it is probably quite unlikely. So I'd prefer to keep it and possibly submit a patch to change the behavior. ISTM that there is another patch in the queue which needs barriers to delay some initialization so as to fix a corner case bug, in which case the behavior would be mandatory. The current submission could add an option to skip the barrier synchronization, but I'm not enthousiastic to add an option and remove it shortly later. Could you tell me which patch you mention? Basically I agree what you say, but I want to check it. Should be this one: https://commitfest.postgresql.org/30/2624/, -- Fabien.
RE: pgbench: option delaying queries till connections establishment?
Dear Fabien, > Indeed. I scanned the file but did not find other places that needed > updating. > Yes. > Not sure either. I'm not for having too many braces anyway, so I removed > them. I checked your fixes and I think it's OK. Finally, please move the doc fixes to patch B in order to separate patches completely. > Indeed. I took your next patch with an added explanation. I'm unclear > whether proceeding makes much sense though, that is some thread would run > the test and other would have aborted. Hmmm. Your comment looks good, thanks. In the previous version pgbench starts benchmarking even if some connections fail. And users can notice the connection failure by stderr output. Hence the current specification may be enough. If you agree, please remove the following lines: ``` +* It is unclear whether it is worth doing anything rather than +* coldly exiting with an error message. ``` > ISTM that there is another patch in the queue which needs barriers to > delay some initialization so as to fix a corner case bug, in which case > the behavior would be mandatory. The current submission could add an > option to skip the barrier synchronization, but I'm not enthousiastic to > add an option and remove it shortly later. Could you tell me which patch you mention? Basically I agree what you say, but I want to check it. Hayato Kuroda FUJITSU LIMITED
RE: pgbench: option delaying queries till connections establishment?
Hello,
Please complete fixes for the documentation. At least the following sentence
should be fixed:
```
The last two lines report the number of transactions per second, figured with
and without counting the time to start database sessions.
```
Indeed. I scanned the file but did not find other places that needed
updating.
-starting vacuum...end.
I think any other options should be disabled in the example, therefore please
leave this line.
Yes.
+ for (int i = 0; i < nstate; i++)
+ {
+ state[i].state = CSTATE_CHOOSE_SCRIPT;
+ }
I'm not sure but I think braces should be removed in our coding conventions.
Not sure either. I'm not for having too many braces anyway, so I removed
them.
+ /* GO */
+ pthread_barrier_wait();
The current implementation is too simple. If nthreads >= 2 and connection fails
in the one thread,
the other one will wait forever.
Some special treatments are needed in the `done` code block and here.
Indeed. I took your next patch with an added explanation. I'm unclear
whether proceeding makes much sense though, that is some thread would run
the test and other would have aborted. Hmmm.
(that is, it can be disabled)
On reflection, I'm not sure I see a use case for not running the barrier
if it is available.
If the start point changes and there is no way to disable this feature,
the backward compatibility will be completely violated.
It means that tps cannot be compared to older versions under the same
conditions,
and It may hide performance-related issues.
I think it's not good.
ISTM that there is another patch in the queue which needs barriers to
delay some initialization so as to fix a corner case bug, in which case
the behavior would be mandatory. The current submission could add an
option to skip the barrier synchronization, but I'm not enthousiastic to
add an option and remove it shortly later.
Moreover, the "compatibility" is with nothing which does not make much
sense. When testing with many threads and clients, the current
implementation make threads start when they are ready, which means that
you can have threads issuing transactions while others are not yet
connected or not even started, so that the actually measured performance
is quite awkward for a short bench. ISTM that allowing such a backward
compatible strange behavior does not serve pg users. If the user want the
old unreliable behavior, they can use a old pgbench, and obtain unreliable
figures.
For these two reasons, I'm inclined not to add an option to skip these
barriers, but this can be debatted further.
Attached 2 updated patches.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 7180fedd65..f02721da0d 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -58,8 +58,10 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-tps = 85.184871 (including connections establishing)
-tps = 85.296346 (excluding connections establishing)
+latency average = 11.013 ms
+latency stddev = 7.351 ms
+initial connection time = 45.758 ms
+tps = 896.967014 (without initial connection establishing)
The first six lines report some of the most important parameter
@@ -68,8 +70,7 @@ tps = 85.296346 (excluding connections establishing)
and number of transactions per client); these will be equal unless the run
failed before completion. (In -T mode, only the actual
number of transactions is printed.)
- The last two lines report the number of transactions per second,
- figured with and without counting the time to start database sessions.
+ The last line reports the number of transactions per second.
@@ -2234,22 +2235,22 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-latency average = 15.844 ms
-latency stddev = 2.715 ms
-tps = 618.764555 (including connections establishing)
-tps = 622.977698 (excluding connections establishing)
+latency average = 10.870 ms
+latency stddev = 7.341 ms
+initial connection time = 30.954 ms
+tps = 907.949122 (without initial connection establishing)
statement latencies in milliseconds:
-0.002 \set aid random(1, 10 * :scale)
-0.005 \set bid random(1, 1 * :scale)
-0.002 \set tid random(1, 10 * :scale)
-0.001 \set delta random(-5000, 5000)
-0.326 BEGIN;
-0.603 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-0.454 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
-5.528 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
-7.335 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
-0.371 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid,
RE: pgbench: option delaying queries till connections establishment?
Dear Fabien; > The current implementation is too simple. If nthreads >= 2 and connection > fails in the one thread, > the other one will wait forever. I attached the very preliminary patch for solving the problem. Even if threads fail to connect, the others can go through the barrier. But I think this implementation is not good... Best Regards, Hayato Kuroda FUJITSU LIMITED temp_fix.patch Description: temp_fix.patch
RE: pgbench: option delaying queries till connections establishment?
Dear Fabien, Andres
I think your idea is good, hence I put some comments as a reviewer.
I focus on only the linux code because I'm not familiar with the Windows
system. Sorry.
[For patch A]
Please complete fixes for the documentation. At least the following sentence
should be fixed:
```
The last two lines report the number of transactions per second, figured with
and without counting the time to start database sessions.
```
> -starting vacuum...end.
I think any other options should be disabled in the example, therefore please
leave this line.
> + /* explicitly initialize the state machines */
> + for (int i = 0; i < nstate; i++)
> + {
> + state[i].state = CSTATE_CHOOSE_SCRIPT;
> + }
I'm not sure but I think braces should be removed in our coding conventions.
Other changes are being reviewed now.
[For patch B]
> + /* GO */
> + pthread_barrier_wait();
The current implementation is too simple. If nthreads >= 2 and connection fails
in the one thread,
the other one will wait forever.
Some special treatments are needed in the `done` code block and here.
[others]
> > (that is, it can be disabled)
>
> On reflection, I'm not sure I see a use case for not running the barrier
> if it is available.
If the start point changes and there is no way to disable this feature,
the backward compatibility will be completely violated.
It means that tps cannot be compared to older versions under the same
conditions,
and It may hide performance-related issues.
I think it's not good.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
-Original Message-
From: Fabien COELHO
Sent: Saturday, July 4, 2020 3:34 PM
To: Daniel Gustafsson
Cc: Andres Freund ; pgsql-hack...@postgresql.org
Subject: Re: pgbench: option delaying queries till connections establishment?
>> * First patch reworks time measurements in pgbench.
>> * Second patch adds a barrier before starting the bench
>>
>> It applies on top of the previous one. The initial imbalance due to
>> thread creation times is smoothed.
>
> The usecs patch fails to apply to HEAD, can you please submit a rebased
> version
> of this patchset. Also, when doing so, can you please rename the patches such
> that sort alphabetically in the order in which they are intended to be
> applied.
> The CFBot patch tester will otherwise try to apply them out of order which
> cause errors.
Ok. Attached.
--
Fabien.
Re: pgbench: option delaying queries till connections establishment?
* First patch reworks time measurements in pgbench.
* Second patch adds a barrier before starting the bench
It applies on top of the previous one. The initial imbalance due to
thread creation times is smoothed.
The usecs patch fails to apply to HEAD, can you please submit a rebased version
of this patchset. Also, when doing so, can you please rename the patches such
that sort alphabetically in the order in which they are intended to be applied.
The CFBot patch tester will otherwise try to apply them out of order which
cause errors.
Ok. Attached.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 8e728dc094..3c5ef9c27e 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -58,8 +58,10 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-tps = 85.184871 (including connections establishing)
-tps = 85.296346 (excluding connections establishing)
+latency average = 11.013 ms
+latency stddev = 7.351 ms
+initial connection time = 45.758 ms
+tps = 896.967014 (without initial connection establishing)
The first six lines report some of the most important parameter
@@ -2226,7 +2228,6 @@ END;
For the default script, the output will look similar to this:
-starting vacuum...end.
transaction type: builtin: TPC-B (sort of)
scaling factor: 1
query mode: simple
@@ -2234,22 +2235,22 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-latency average = 15.844 ms
-latency stddev = 2.715 ms
-tps = 618.764555 (including connections establishing)
-tps = 622.977698 (excluding connections establishing)
+latency average = 10.870 ms
+latency stddev = 7.341 ms
+initial connection time = 30.954 ms
+tps = 907.949122 (without initial connection establishing)
statement latencies in milliseconds:
-0.002 \set aid random(1, 10 * :scale)
-0.005 \set bid random(1, 1 * :scale)
-0.002 \set tid random(1, 10 * :scale)
-0.001 \set delta random(-5000, 5000)
-0.326 BEGIN;
-0.603 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-0.454 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
-5.528 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
-7.335 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
-0.371 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
-1.212 END;
+0.001 \set aid random(1, 10 * :scale)
+0.001 \set bid random(1, 1 * :scale)
+0.001 \set tid random(1, 10 * :scale)
+0.000 \set delta random(-5000, 5000)
+0.046 BEGIN;
+0.151 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
+0.107 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
+4.241 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
+5.245 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
+0.102 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
+0.974 END;
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 08a5947a9e..46be67adaf 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -291,9 +291,9 @@ typedef struct SimpleStats
*/
typedef struct StatsData
{
- time_t start_time; /* interval start time, for aggregates */
- int64 cnt; /* number of transactions, including skipped */
- int64 skipped; /* number of transactions skipped under --rate
+ pg_time_usec_t start_time; /* interval start time, for aggregates */
+ int64 cnt; /* number of transactions, including skipped */
+ int64 skipped; /* number of transactions skipped under --rate
* and --latency-limit */
SimpleStats latency;
SimpleStats lag;
@@ -416,11 +416,11 @@ typedef struct
int nvariables; /* number of variables */
bool vars_sorted; /* are variables sorted by name? */
- /* various times about current transaction */
- int64 txn_scheduled; /* scheduled start time of transaction (usec) */
- int64 sleep_until; /* scheduled start time of next cmd (usec) */
- instr_time txn_begin; /* used for measuring schedule lag times */
- instr_time stmt_begin; /* used for measuring statement latencies */
+ /* various times about current transaction in microseconds */
+ pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
+ pg_time_usec_t sleep_until; /* scheduled start time of next cmd */
+ pg_time_usec_t txn_begin; /* used for measuring schedule lag times */
+ pg_time_usec_t stmt_begin; /* used for measuring statement latencies */
bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */
@@ -449,13
Re: pgbench: option delaying queries till connections establishment?
> On 17 May 2020, at 11:55, Fabien COELHO wrote: > I have split the patch. > > * First patch reworks time measurements in pgbench. > * Second patch adds a barrier before starting the bench > > It applies on top of the previous one. The initial imbalance due to thread > creation times is smoothed. The usecs patch fails to apply to HEAD, can you please submit a rebased version of this patchset. Also, when doing so, can you please rename the patches such that sort alphabetically in the order in which they are intended to be applied. The CFBot patch tester will otherwise try to apply them out of order which cause errors. cheers ./daniel
Re: pgbench: option delaying queries till connections establishment?
Hello,
I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.
Hm. I'm not convinced it's a good idea for pgbench to do its own thing
here.
I really think that the refactoring part is a good thing because cloc and
cost is reduced (time arithmetic is an ugly pain with instr_time).
I have split the patch.
* First patch reworks time measurements in pgbench.
It creates a convenient pg_time_usec_t and use it everywhere, getting rid
of "instr_time_t". The code is somehow simplified wrt what time are taken
and what they mean.
Instead of displaying 2 tps at the end, which is basically insane, it
shows one tps for --connect, which includes reconnection times, and one
tps for the usual one connection at startup which simply ignores the
initial connection time.
This (mostly) refactoring reduces the cloc.
* Second patch adds a barrier before starting the bench
It applies on top of the previous one. The initial imbalance due to thread
creation times is smoothed.
I may add a --start-on option afterwards so that several pgbench (running
on distinct hosts) can be synchronized, which would be implemented as a
delay inserted by thread 0 before the barrier.
The windows implementation is more or less blind, if someone can confirm
that it works, it would be nice.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index f5a51d3732..26b4c4f61c 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -58,8 +58,10 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-tps = 85.184871 (including connections establishing)
-tps = 85.296346 (excluding connections establishing)
+latency average = 11.013 ms
+latency stddev = 7.351 ms
+initial connection time = 45.758 ms
+tps = 896.967014 (without initial connection establishing)
The first six lines report some of the most important parameter
@@ -2228,7 +2230,6 @@ END;
For the default script, the output will look similar to this:
-starting vacuum...end.
transaction type: builtin: TPC-B (sort of)
scaling factor: 1
query mode: simple
@@ -2236,22 +2237,22 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-latency average = 15.844 ms
-latency stddev = 2.715 ms
-tps = 618.764555 (including connections establishing)
-tps = 622.977698 (excluding connections establishing)
+latency average = 10.870 ms
+latency stddev = 7.341 ms
+initial connection time = 30.954 ms
+tps = 907.949122 (without initial connection establishing)
statement latencies in milliseconds:
-0.002 \set aid random(1, 10 * :scale)
-0.005 \set bid random(1, 1 * :scale)
-0.002 \set tid random(1, 10 * :scale)
-0.001 \set delta random(-5000, 5000)
-0.326 BEGIN;
-0.603 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-0.454 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
-5.528 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
-7.335 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
-0.371 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
-1.212 END;
+0.001 \set aid random(1, 10 * :scale)
+0.001 \set bid random(1, 1 * :scale)
+0.001 \set tid random(1, 10 * :scale)
+0.000 \set delta random(-5000, 5000)
+0.046 BEGIN;
+0.151 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
+0.107 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
+4.241 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
+5.245 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
+0.102 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
+0.974 END;
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 08a5947a9e..46be67adaf 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -291,9 +291,9 @@ typedef struct SimpleStats
*/
typedef struct StatsData
{
- time_t start_time; /* interval start time, for aggregates */
- int64 cnt; /* number of transactions, including skipped */
- int64 skipped; /* number of transactions skipped under --rate
+ pg_time_usec_t start_time; /* interval start time, for aggregates */
+ int64 cnt; /* number of transactions, including skipped */
+ int64 skipped; /* number of transactions skipped under --rate
* and --latency-limit */
SimpleStats latency;
SimpleStats lag;
@@ -416,11 +416,11 @@ typedef struct
int nvariables; /* number of
Re: pgbench: option delaying queries till connections establishment?
Hallo Andres,
Slight aside: Have you ever looked at moving pgbench to non-blocking
connection establishment? It seems weird to use non-blocking everywhere
but connection establishment.
Attached an attempt at doing that, mostly done for fun. It seems to be a
little slower on a local socket.
What do you think?
Maybe it would be worth having it with an option?
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..83ac2235a5 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -102,8 +102,9 @@ typedef struct socket_set
typedef struct socket_set
{
- int maxfd; /* largest FD currently set in fds */
- fd_set fds;
+ int maxfd; /* largest FD currently set in reads or writes */
+ fd_set reads;
+ fd_set writes;
} socket_set;
#endif /* POLL_USING_SELECT */
@@ -318,15 +319,32 @@ typedef enum
/*
* The client must first choose a script to execute. Once chosen, it can
* either be throttled (state CSTATE_PREPARE_THROTTLE under --rate), start
- * right away (state CSTATE_START_TX) or not start at all if the timer was
- * exceeded (state CSTATE_FINISHED).
+ * right away (state CSTATE_START_TX or CSTATE_CONNECT on --connect) or
+ * not start at all if the timer was exceeded (state CSTATE_FINISHED).
*/
CSTATE_CHOOSE_SCRIPT,
/*
- * CSTATE_START_TX performs start-of-transaction processing. Establishes
- * a new connection for the transaction in --connect mode, records the
- * transaction start time, and proceed to the first command.
+ * In CSTATE_PREPARE_THROTTLE state, we calculate when to begin the next
+ * transaction, and advance to CSTATE_THROTTLE. CSTATE_THROTTLE state
+ * sleeps until that moment, then advances to CSTATE_CONNECT (-C) or
+ * CSTATE_START_TX (not -C), or CSTATE_FINISHED if the next transaction
+ * would start beyond the end of the run.
+ */
+ CSTATE_PREPARE_THROTTLE,
+ CSTATE_THROTTLE,
+
+ /*
+ * CSTATE_CONNECT Establishes a connection asynchronously before starting
+ * a transaction, under -C. The state is then CSTATE_CONNECTING till the
+ * connection is established, and then CSTATE_START_TX.
+ */
+ CSTATE_CONNECT,
+ CSTATE_CONNECTING,
+
+ /*
+ * CSTATE_START_TX performs start-of-transaction processing.
+ * It records the transaction start time, and proceed to the first command.
*
* Note: once a script is started, it will either error or run till its
* end, where it may be interrupted. It is not interrupted while running,
@@ -335,16 +353,6 @@ typedef enum
*/
CSTATE_START_TX,
- /*
- * In CSTATE_PREPARE_THROTTLE state, we calculate when to begin the next
- * transaction, and advance to CSTATE_THROTTLE. CSTATE_THROTTLE state
- * sleeps until that moment, then advances to CSTATE_START_TX, or
- * CSTATE_FINISHED if the next transaction would start beyond the end of
- * the run.
- */
- CSTATE_PREPARE_THROTTLE,
- CSTATE_THROTTLE,
-
/*
* We loop through these states, to process each command in the script:
*
@@ -401,6 +409,7 @@ typedef struct
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
+ PostgresPollingStatusType poll_state; /* async connection status */
/*
* Separate randomness for each client. This is used for random functions
@@ -421,6 +430,7 @@ typedef struct
int64 sleep_until; /* scheduled start time of next cmd (usec) */
instr_time txn_begin; /* used for measuring schedule lag times */
instr_time stmt_begin; /* used for measuring statement latencies */
+ instr_time conn_begin; /* start of asynchronous connection under -C */
bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */
@@ -607,9 +617,9 @@ static void setalarm(int seconds);
static socket_set *alloc_socket_set(int count);
static void free_socket_set(socket_set *sa);
static void clear_socket_set(socket_set *sa);
-static void add_socket_to_set(socket_set *sa, int fd, int idx);
+static void add_socket_to_set(socket_set *sa, int fd, int idx, bool reading);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
-static bool socket_has_input(socket_set *sa, int fd, int idx);
+static bool socket_is_ready(socket_set *sa, int fd, int idx);
/* callback functions for our flex lexer */
@@ -1165,6 +1175,57 @@ tryExecuteStatement(PGconn *con, const char *sql)
PQclear(res);
}
+#define PARAMS_ARRAY_SIZE 7
+
+/* set connection parameters */
+static void
+setPQconnectionParams(const char *keywords[PARAMS_ARRAY_SIZE],
+ const char *values[PARAMS_ARRAY_SIZE],
+ const char *password)
+{
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = login;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbName;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] =
Re: pgbench: option delaying queries till connections establishment?
Hello Andres,
I've changed the performance calculations depending on -C or not. Ramp-up
effects are smoothed.
I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.
Hm. I'm not convinced it's a good idea for pgbench to do its own thing
here.
Given the unjustifiable heterogeneousness it induces and the simpler code
after the move, I think it is much better. Pgbench cloc is smaller after
barrier are added (4655 to 4650) thanks to that and a few other code
simplifications. Removing all INSTR_TIME_* costly macros is a relief in
itself…
+static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused,
int nthreads);
How about using 'struct unknown_type *unused' instead of "unused"?
Haven't done it because I found no other instances in pg, and anyway this
code is only used once locally and NULL is passed.
+static pthread_barrier_t
+ start_barrier, /* all threads are started */
+ bench_barrier; /* benchmarking ready to start */
We don't really need two barriers here.
Indeed. Down to one.
/* print out results */
Given that we're changing the output (for the better) of pgbench again,
I wonder if we should add the pgbench version to the benchmark
output.
Not sure about it, but done anyway.
+pg_time_usec_t total_delay,/* benchmarking
time */
+pg_time_usec_t conn_total_delay, /* is_connect */
+pg_time_usec_t conn_elapsed_delay, /* !is_connect
*/
+int64 latency_late)
I'm not a fan of naming these 'delay'. To me that doesn't sounds like
it's about the time the total benchmark has taken.
I have used '_duration', and tried to clarify some field and variable
names depending on what data they actually hold.
+ printf("tps = %f (including reconnection times)\n", tps);
+ printf("tps = %f (without initial connection establishing)\n",
tps);
Keeping these separate makes sense to me, they're just so wildly
different.
Yep. I've added a comment about that.
+static inline pg_time_usec_t
+pg_time_get_usec(void)
For me the function name sounds like you're getting the usec out of a
pg_time. Not that it's getting a new timestamp.
I've used "pg_time_now()".
+#define PG_TIME_SET_CURRENT_LAZY(t)\
+ if ((t) == 0) \
+ (t) = pg_time_get_usec()
I'd make it an inline function instead of this.
Done "pg_time_now_lazy()"
I have also simplified the code around thread creation & join because it
was a mess: thread 0 was run in the middle of the stat collection loop…
I have updated the doc with actual current output, but excluding the
version display which would have to be changed between releases.
--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4c48a58ed2..b77c1089d8 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -55,8 +55,10 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-tps = 85.184871 (including connections establishing)
-tps = 85.296346 (excluding connections establishing)
+latency average = 11.013 ms
+latency stddev = 7.351 ms
+initial connection time = 45.758 ms
+tps = 896.967014 (without initial connection establishing)
The first six lines report some of the most important parameter
@@ -1835,7 +1837,6 @@ END;
For the default script, the output will look similar to this:
-starting vacuum...end.
transaction type: builtin: TPC-B (sort of)
scaling factor: 1
query mode: simple
@@ -1843,22 +1844,22 @@ number of clients: 10
number of threads: 1
number of transactions per client: 1000
number of transactions actually processed: 1/1
-latency average = 15.844 ms
-latency stddev = 2.715 ms
-tps = 618.764555 (including connections establishing)
-tps = 622.977698 (excluding connections establishing)
+latency average = 10.870 ms
+latency stddev = 7.341 ms
+initial connection time = 30.954 ms
+tps = 907.949122 (without initial connection establishing)
statement latencies in milliseconds:
-0.002 \set aid random(1, 10 * :scale)
-0.005 \set bid random(1, 1 * :scale)
-0.002 \set tid random(1, 10 * :scale)
-0.001 \set delta random(-5000, 5000)
-0.326 BEGIN;
-0.603 UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-0.454 SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
-5.528 UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
-7.335 UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
-0.371 INSERT INTO pgbench_history (tid, bid, aid, delta, mtime)
Re: pgbench: option delaying queries till connections establishment?
Hello Andres, Slight aside: Have you ever looked at moving pgbench to non-blocking connection establishment? It seems weird to use non-blocking everywhere but connection establishment. Nope. If there is some interest, why not. The reason for not doing it is that the typical use case is just to connect once at the beginning so that connections do not matter anyway. Now with -C it makes sense. I've changed the performance calculations depending on -C or not. Ramp-up effects are smoothed. I've merged all time-related stuff (time_t, instr_time, int64) to use a unique type (pg_time_usec_t) and set of functions/macros, which simplifies the code somehow. Hm. I'm not convinced it's a good idea for pgbench to do its own thing here. Having 3 time types (in fact, 4, double is used as well for some calculations) in just one file to deal with time does not help much to understand the code, and there is quite a few line to translate from one to the other. +static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads); How about using 'struct unknown_type *unused' instead of "unused"? Because the void *unused will accept everything... Never encountered this pattern. It does not seem to be used anywhere in pg sources. I'd be afraid that some compilers would complain. I can try anyway. +/* Thread synchronization barriers */ +static pthread_barrier_t + start_barrier, /* all threads are started */ + bench_barrier; /* benchmarking ready to start */ + We don't really need two barriers here. The way that pthread barriers are defined is that they 'reset' after all the threads have arrived. You can argue we still want that, but ... Yes, one barrier could be reused. /* print out results */ static void -printResults(StatsData *total, instr_time total_time, -instr_time conn_total_time, int64 latency_late) +printResults(StatsData *total, Given that we're changing the output (for the better) of pgbench again, I wonder if we should add the pgbench version to the benchmark output. Dunno. Maybe. Otherwise it seems easy to end up e.g. seeing a performance difference between pg12 and pg14, where all that's actually happening is a different output because each run used the respective pgbench version. Yep. +pg_time_usec_t total_delay,/* benchmarking time */ +pg_time_usec_t conn_total_delay, /* is_connect */ +pg_time_usec_t conn_elapsed_delay, /* !is_connect */ +int64 latency_late) I'm not a fan of naming these 'delay'. To me that doesn't sounds like it's about the time the total benchmark has taken. Hmmm… I'd like to differentiate variable names which contain timestamp versus those which contain intervals, given that it is the same underlying type. That said, I'm not very happy with "delay" either. What would you suggest? +pg_time_get_usec(void) For me the function name sounds like you're getting the usec out of a pg_time. Not that it's getting a new timestamp. Ok, I'll think of something else, possibly "pg_now"? "pg_time_now"? +#define PG_TIME_SET_CURRENT_LAZY(t)\ + if ((t) == 0) \ + (t) = pg_time_get_usec() + +#define PG_TIME_GET_DOUBLE(t) (0.01 * (t)) #endif /* INSTR_TIME_H */ I'd make it an inline function instead of this. I did it that way because it was already done with defines on instr_time, but I'm fine with inline. I'll try to look at it over the week-end. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Hi,
On 2020-03-01 22:16:06 +0100, Fabien COELHO wrote:
>
> Hello Andres,
>
> > FWIW, leaving windows, error handling, and other annoyances aside, this
> > can be implemented fairly simply. See below.
>
> Attached an attempt at improving things.
Awesome!
> I've put 2 barriers: one so that all threads are up, one when all
> connections are setup and the bench is ready to go.
I'd done similarly locally.
Slight aside: Have you ever looked at moving pgbench to non-blocking
connection establishment? It seems weird to use non-blocking everywhere
but connection establishment.
> I've done a blind attempt at implementing the barrier stuff on windows.
Neat.
> I've changed the performance calculations depending on -C or not. Ramp-up
> effects are smoothed.
> I've merged all time-related stuff (time_t, instr_time, int64) to use a
> unique type (pg_time_usec_t) and set of functions/macros, which simplifies
> the code somehow.
Hm. I'm not convinced it's a good idea for pgbench to do its own thing
here.
>
> #ifdef WIN32
> +#define PTHREAD_BARRIER_SERIAL_THREAD (-1)
> +
> /* Use native win32 threads on Windows */
> typedef struct win32_pthread *pthread_t;
> typedef int pthread_attr_t;
> +typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
>
> static int pthread_create(pthread_t *thread, pthread_attr_t *attr, void
> *(*start_routine) (void *), void *arg);
> static int pthread_join(pthread_t th, void **thread_return);
> +
> +static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused,
> int nthreads);
> +static int pthread_barrier_wait(pthread_barrier_t *barrier);
> +static int pthread_barrier_destroy(pthread_barrier_t *barrier);
How about using 'struct unknown_type *unused' instead of "unused"?
Because the void *unused will accept everything...
> +/* Thread synchronization barriers */
> +static pthread_barrier_t
> + start_barrier, /* all threads are started */
> + bench_barrier; /* benchmarking ready to start */
> +
We don't really need two barriers here. The way that pthread barriers
are defined is that they 'reset' after all the threads have arrived. You
can argue we still want that, but ...
> @@ -5165,20 +5151,16 @@ printSimpleStats(const char *prefix, SimpleStats *ss)
>
> /* print out results */
> static void
> -printResults(StatsData *total, instr_time total_time,
> - instr_time conn_total_time, int64 latency_late)
> +printResults(StatsData *total,
Given that we're changing the output (for the better) of pgbench again,
I wonder if we should add the pgbench version to the benchmark
output. Otherwise it seems easy to end up e.g. seeing a performance
difference between pg12 and pg14, where all that's actually happening is
a different output because each run used the respective pgbench version.
> + pg_time_usec_t total_delay,/* benchmarking
> time */
> + pg_time_usec_t conn_total_delay, /* is_connect */
> + pg_time_usec_t conn_elapsed_delay, /* !is_connect
> */
> + int64 latency_late)
I'm not a fan of naming these 'delay'. To me that doesn't sounds like
it's about the time the total benchmark has taken.
> @@ -5239,8 +5220,16 @@ printResults(StatsData *total, instr_time total_time,
> 0.001 * total->lag.sum / total->cnt, 0.001 *
> total->lag.max);
> }
>
> - printf("tps = %f (including connections establishing)\n", tps_include);
> - printf("tps = %f (excluding connections establishing)\n", tps_exclude);
> + if (is_connect)
> + {
> + printf("average connection time = %.3f ms\n", 0.001 *
> conn_total_delay / total->cnt);
> + printf("tps = %f (including reconnection times)\n", tps);
> + }
> + else
> + {
> + printf("initial connection time = %.3f ms\n", 0.001 *
> conn_elapsed_delay);
> + printf("tps = %f (without initial connection establishing)\n",
> tps);
> + }
Keeping these separate makes sense to me, they're just so wildly
different.
> +/*
> + * Simpler convenient interface
> + *
> + * The instr_time type is expensive when dealing with time arithmetic.
> + * Define a type to hold microseconds on top of this, suitable for
> + * benchmarking performance measures, eg in "pgbench".
> + */
> +typedef int64 pg_time_usec_t;
> +
> +static inline pg_time_usec_t
> +pg_time_get_usec(void)
> +{
> + instr_time now;
> +
> + INSTR_TIME_SET_CURRENT(now);
> + return (pg_time_usec_t) INSTR_TIME_GET_MICROSEC(now);
> +}
For me the function name sounds like you're getting the usec out of a
pg_time. Not that it's getting a new timestamp.
> +#define PG_TIME_SET_CURRENT_LAZY(t) \
> + if ((t) == 0) \
> + (t) = pg_time_get_usec()
> +
> +#define PG_TIME_GET_DOUBLE(t) (0.01 * (t))
> #endif
Re: pgbench: option delaying queries till connections establishment?
Hello Andres,
FWIW, leaving windows, error handling, and other annoyances aside, this
can be implemented fairly simply. See below.
Attached an attempt at improving things.
I've put 2 barriers: one so that all threads are up, one when all
connections are setup and the bench is ready to go.
I've done a blind attempt at implementing the barrier stuff on windows.
I've changed the performance calculations depending on -C or not. Ramp-up
effects are smoothed.
I've merged all time-related stuff (time_t, instr_time, int64) to use a
unique type (pg_time_usec_t) and set of functions/macros, which simplifies
the code somehow.
I've tried to do some variable renaming to distinguish timestamps and
intervals.
This is work in progress.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b8864c6ae5..a16d9d49e1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -113,18 +113,29 @@ typedef struct socket_set
*/
#ifdef WIN32
+#define PTHREAD_BARRIER_SERIAL_THREAD (-1)
+
/* Use native win32 threads on Windows */
typedef struct win32_pthread *pthread_t;
typedef int pthread_attr_t;
+typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
static int pthread_create(pthread_t *thread, pthread_attr_t *attr, void *(*start_routine) (void *), void *arg);
static int pthread_join(pthread_t th, void **thread_return);
+
+static int pthread_barrier_init(pthread_barrier_t *barrier, void *unused, int nthreads);
+static int pthread_barrier_wait(pthread_barrier_t *barrier);
+static int pthread_barrier_destroy(pthread_barrier_t *barrier);
#elif defined(ENABLE_THREAD_SAFETY)
/* Use platform-dependent pthread capability */
#include
#else
/* No threads implementation, use none (-j 1) */
#define pthread_t void *
+#define pthread_barrier_t void *
+#define pthread_barrier_init(a, b, c) /* ignore */
+#define pthread_barrier_wait(a) /* ignore */
+#define pthread_barrier_destroy(a) /* ignore */
#endif
@@ -291,7 +302,7 @@ typedef struct SimpleStats
*/
typedef struct StatsData
{
- time_t start_time; /* interval start time, for aggregates */
+ pg_time_usec_t start_time; /* interval start time, for aggregates */
int64 cnt; /* number of transactions, including skipped */
int64 skipped; /* number of transactions skipped under --rate
* and --latency-limit */
@@ -310,6 +321,11 @@ typedef struct RandomState
/* Various random sequences are initialized from this one. */
static RandomState base_random_sequence;
+/* Thread synchronization barriers */
+static pthread_barrier_t
+ start_barrier, /* all threads are started */
+ bench_barrier; /* benchmarking ready to start */
+
/*
* Connection state machine states.
*/
@@ -417,10 +433,10 @@ typedef struct
bool vars_sorted; /* are variables sorted by name? */
/* various times about current transaction */
- int64 txn_scheduled; /* scheduled start time of transaction (usec) */
- int64 sleep_until; /* scheduled start time of next cmd (usec) */
- instr_time txn_begin; /* used for measuring schedule lag times */
- instr_time stmt_begin; /* used for measuring statement latencies */
+ pg_time_usec_t txn_scheduled; /* scheduled start time of transaction (usec) */
+ pg_time_usec_t sleep_until; /* scheduled start time of next cmd (usec) */
+ pg_time_usec_t txn_begin; /* used for measuring schedule lag times (usec) */
+ pg_time_usec_t stmt_begin; /* used for measuring statement latencies (usec) */
bool prepared[MAX_SCRIPTS]; /* whether client prepared the script */
@@ -452,10 +468,13 @@ typedef struct
FILE *logfile; /* where to log, or NULL */
/* per thread collected stats */
- instr_time start_time; /* thread start time */
- instr_time conn_time;
+ pg_time_usec_t start_time; /* thread start (creation) time */
+ pg_time_usec_t started_time; /* thread is running after start barrier */
+ pg_time_usec_t bench_start_time; /* thread is really benchmarking */
+ pg_time_usec_t conn_delay; /* cumulated connection and deconnection delays (usec) */
+
StatsData stats;
- int64 latency_late; /* executed but late transactions */
+ int64 latency_late; /* count executed but late transactions */
} TState;
#define INVALID_THREAD ((pthread_t) 0)
@@ -594,10 +613,10 @@ static void setIntValue(PgBenchValue *pv, int64 ival);
static void setDoubleValue(PgBenchValue *pv, double dval);
static bool evaluateExpr(CState *st, PgBenchExpr *expr,
PgBenchValue *retval);
-static ConnectionStateEnum executeMetaCommand(CState *st, instr_time *now);
+static ConnectionStateEnum executeMetaCommand(CState *st, pg_time_usec_t *now);
static void doLog(TState *thread, CState *st,
StatsData *agg, bool skipped, double latency, double lag);
-static void processXactStats(TState *thread, CState *st, instr_time *now,
+static void processXactStats(TState *thread, CState *st, pg_time_usec_t *now,
bool skipped, StatsData *agg);
static void append_fillfactor(char *opts, int len);
static
Re: pgbench: option delaying queries till connections establishment?
Hi, On 2020-02-29 15:29:19 +0100, Fabien COELHO wrote: > Pgbench is more or less designed to run a long hopefully steady-state > benchmark, so that the initial connection setup is always negligeable. Not > complying with this hypothesis quite often leads to weird results. I don't think this is a good starting point. Sure, a longer run will yield more precise results, and one needs more than just an instantaneous measurement. But in a lot of cases we want to use pgbench to measure a lot of different variations, making it infeasible for each run to be all that long. Of course whether that's feasible depends on the workload (e.g. readonly runs can be shorter than read/write runs). Also note that in the case that made me look at this, you'd have to run the test for *weeks* to drown out the performance difference that's solely caused by difference in how long individual connects are established. Partially because the "excluding connection establishing" number is entirely broken, but also because fewer connections having been established changes the performance so much. I think we should also consider making pgbench actually use non-blocking connection establishment. It seems pretty weird that that's the one libpq operation where we don't? In particular for -C, with -c > -j, that makes the results pretty meaningless. > Adding a synchronization barrier should be simple enough, I thought about it > in the past. > > However, I'd still be wary that it is no silver bullet: if you start a lot > of threads compared to the number of available cores, pgbench would > basically overload the system, and you would experience a lot of waiting > time which reflects that the client code has not got enough cpu time. > Basically you would be testing the OS process/thread management performance. Sure, that's possible. But I don't see what that has to do with the barrier? Also, most scripts actually have client/server interaction... Greetings, Andres Freund
Re: pgbench: option delaying queries till connections establishment?
Hello Kyotaro-san, I think this also shows that "including/excluding connections establishing" as well as some of the other stats reported pretty bogus. In the 'before' case a substantial numer of the connections had not yet been established until the end of the test run! I see it useful. In most cases we don't care connection time of pgbench. Especially in the mentioned case the result is just bogus. I think the reason for "including/excluding connection establishing" is not that people wants to see how long connection took to establish but that how long the substantial work took. If each client did run with continuously re-establishing new connections the connection time would be useful, but actually all the connections are established at once at the beginning. So FWIW I prefer that the barrier is applied by default Yep. (that is, it can be disabled) On reflection, I'm not sure I see a use case for not running the barrier if it is available. and the progress time starts at the time all clients has been established. Yep, the start time should be set after the connection barrier, and possibly before a start barrier to ensure that no transaction has started before the start time: although performance measures are expected to be slightly false because of how they are measured (measuring takes time), from a benchmarking perspective the displayed result should be <= the actual performance. Now, again, if long benchmarks are run, which for a db should more or less always be the case, this should not matter much. starting vacuum...end. + time to established 5000 connections: 1323ms Yep, maybe showing the initial connection time separately. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
Hello Andres, Therefore I'd like to make pgbench wait till it has established all connections, before they run queries. Does anybody else see this as being useful? Yes, I think that having this behavior available would make sense. If so, should this be done unconditionally? Dunno. I should think about it. I'd say probably. Pgbench is more or less designed to run a long hopefully steady-state benchmark, so that the initial connection setup is always negligeable. Not complying with this hypothesis quite often leads to weird results. A new option? Maybe, if not unconditional. If there is an unconditional barrier, the excluding/including connection stuff does not make a lot of sense when not under -C, if it did make any sense before… Included in an existing one somehow? Which one would you suggest? Adding a synchronization barrier should be simple enough, I thought about it in the past. However, I'd still be wary that it is no silver bullet: if you start a lot of threads compared to the number of available cores, pgbench would basically overload the system, and you would experience a lot of waiting time which reflects that the client code has not got enough cpu time. Basically you would be testing the OS process/thread management performance. On my 4-core laptop, with a do-nothing script (\set i 0): sh> pgbench -T 10 -f nope.sql -P 1 -j 10 -c 10 latency average = 0.000 ms latency stddev = 0.049 ms tps = 21048841.630291 (including connections establishing) tps = 21075139.938887 (excluding connections establishing) sh> pgbench -T 10 -f nope.sql -P 1 -j 100 -c 100 latency average = 0.002 ms latency stddev = 0.470 ms tps = 23846777.241370 (including connections establishing) tps = 24132252.146257 (excluding connections establishing) Throughput is slightly better, latency average and variance explode because each thread is given stretches of cpu time to advance, then wait for the next round of cpu time. -- Fabien.
Re: pgbench: option delaying queries till connections establishment?
At Thu, 27 Feb 2020 10:51:29 -0800, Andres Freund wrote in > Hi, > > On 2020-02-27 10:01:00 -0800, Andres Freund wrote: > > If so, should this be done unconditionally? A new option? Included in an > > existing one somehow? > > FWIW, leaving windows, error handling, and other annoyances aside, this > can be implemented fairly simply. See below. > > As an example of the difference: > > Before: > andres@awork3:~/build/postgres/dev-optimize/vpath$ ./src/bin/pgbench/pgbench > -M prepared -c 5000 -j 100 -T 100 -P1 -S > starting vacuum...end. > progress: 100.4 s, 515307.4 tps, lat 1.374 ms stddev 7.739 > transaction type: > scaling factor: 30 > query mode: prepared > number of clients: 5000 > number of threads: 100 > duration: 100 s > number of transactions actually processed: 51728348 > latency average = 1.374 ms > latency stddev = 7.739 ms > tps = 513802.541226 (including connections establishing) > tps = 521342.427158 (excluding connections establishing) > > > Note that there's no progress report until the end. That's because the > main thread didn't get a connection until the other threads were done. > > > After: > > pgbench -M prepared -c 5000 -j 100 -T 100 -P1 -S > starting vacuum...end. > progress: 1.5 s, 9943.5 tps, lat 4.795 ms stddev 14.822 > progress: 2.0 s, 380312.6 tps, lat 1.728 ms stddev 15.461 > progress: 3.0 s, 478811.1 tps, lat 2.052 ms stddev 31.687 > progress: 4.0 s, 470804.6 tps, lat 1.941 ms stddev 24.661 > > > > I think this also shows that "including/excluding connections > establishing" as well as some of the other stats reported pretty > bogus. In the 'before' case a substantial numer of the connections had > not yet been established until the end of the test run! I see it useful. In most cases we don't care connection time of pgbench. Especially in the mentioned case the result is just bogus. I think the reason for "including/excluding connection establishing" is not that people wants to see how long connection took to establish but that how long the substantial work took. If each client did run with continuously re-establishing new connections the connection time would be useful, but actually all the connections are established at once at the beginning. So FWIW I prefer that the barrier is applied by default (that is, it can be disabled) and the progress time starts at the time all clients has been established. > starting vacuum...end. + time to established 5000 connections: 1323ms ! progress: 1.0 s, 33.5 tps, lat 2.795 ms stddev 14.822 ! progress: 2.0 s, 380312.6 tps, lat 1.728 ms stddev 15.461 ! progress: 3.0 s, 478811.1 tps, lat 2.052 ms stddev 31.687 ! progress: 4.0 s, 470804.6 tps, lat 1.941 ms stddev 24.661 > transaction type: > scaling factor: 30 > query mode: prepared > number of clients: 5000 > number of threads: 100 > duration: 100 s > number of transactions actually processed: 51728348 > latency average = 1.374 ms > latency stddev = 7.739 ms > tps = 513802.541226 (including connections establishing) > tps = 521342.427158 (excluding connections establishing) regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pgbench: option delaying queries till connections establishment?
Hi,
On 2020-02-27 10:01:00 -0800, Andres Freund wrote:
> If so, should this be done unconditionally? A new option? Included in an
> existing one somehow?
FWIW, leaving windows, error handling, and other annoyances aside, this
can be implemented fairly simply. See below.
As an example of the difference:
Before:
andres@awork3:~/build/postgres/dev-optimize/vpath$ ./src/bin/pgbench/pgbench -M
prepared -c 5000 -j 100 -T 100 -P1 -S
starting vacuum...end.
progress: 100.4 s, 515307.4 tps, lat 1.374 ms stddev 7.739
transaction type:
scaling factor: 30
query mode: prepared
number of clients: 5000
number of threads: 100
duration: 100 s
number of transactions actually processed: 51728348
latency average = 1.374 ms
latency stddev = 7.739 ms
tps = 513802.541226 (including connections establishing)
tps = 521342.427158 (excluding connections establishing)
Note that there's no progress report until the end. That's because the
main thread didn't get a connection until the other threads were done.
After:
pgbench -M prepared -c 5000 -j 100 -T 100 -P1 -S
starting vacuum...end.
progress: 1.5 s, 9943.5 tps, lat 4.795 ms stddev 14.822
progress: 2.0 s, 380312.6 tps, lat 1.728 ms stddev 15.461
progress: 3.0 s, 478811.1 tps, lat 2.052 ms stddev 31.687
progress: 4.0 s, 470804.6 tps, lat 1.941 ms stddev 24.661
I think this also shows that "including/excluding connections
establishing" as well as some of the other stats reported pretty
bogus. In the 'before' case a substantial numer of the connections had
not yet been established until the end of the test run!
diff --git i/src/bin/pgbench/pgbench.c w/src/bin/pgbench/pgbench.c
index 1159757acb0..1a82c6a290e 100644
--- i/src/bin/pgbench/pgbench.c
+++ w/src/bin/pgbench/pgbench.c
@@ -310,6 +310,8 @@ typedef struct RandomState
/* Various random sequences are initialized from this one. */
static RandomState base_random_sequence;
+pthread_barrier_t conn_barrier;
+
/*
* Connection state machine states.
*/
@@ -6110,6 +6112,8 @@ main(int argc, char **argv)
/* start threads */
#ifdef ENABLE_THREAD_SAFETY
+pthread_barrier_init(_barrier, NULL, nthreads);
+
for (i = 0; i < nthreads; i++)
{
TState *thread = [i];
@@ -6265,6 +6269,8 @@ threadRun(void *arg)
INSTR_TIME_SET_CURRENT(thread->conn_time);
INSTR_TIME_SUBTRACT(thread->conn_time, thread->start_time);
+pthread_barrier_wait(_barrier);
+
/* explicitly initialize the state machines */
for (i = 0; i < nstate; i++)
{
Greetings,
Andres Freund
