Call xlc __isync() after, not before, associated compare-and-swap.

Architecture reference material specifies this order, and s_lock.h
inline assembly agrees.  The former order failed to provide mutual
exclusion to lwlock.c and perhaps to other clients.  The two xlc
buildfarm members, hornet and mandrill, have failed sixteen times with
duplicate key errors involving pg_class_oid_index or pg_type_oid_index.
Back-patch to 9.5, where commit b64d92f1a5602c55ee8b27a7ac474f03b7aee340
introduced atomics.

Reviewed by Andres Freund and Tom Lane.

diff --git a/src/bin/pgbench/.gitignore b/src/bin/pgbench/.gitignore
index aae819ed70f9f4e7d8cdcd26ef0094b7d8d883e9..983a3cd7a6d81d6a441c36507998983ca76b037d 100644 (file)
--- a/src/bin/pgbench/.gitignore
+++ b/src/bin/pgbench/.gitignore
@@ -1,3 +1,4 @@
 /exprparse.c
 /exprscan.c
 /pgbench
+/tmp_check/

diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index 18fdf58d13e40531250ac708cb9aa5510a3d721e..e9b1b74abfaffba6d7ec35f9352d20167f8e6f63 100644 (file)
--- a/src/bin/pgbench/Makefile
+++ b/src/bin/pgbench/Makefile
@@ -40,3 +40,9 @@ clean distclean:
 
 maintainer-clean: distclean
    rm -f exprparse.c exprscan.c
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)

diff --git a/src/bin/pgbench/t/001_pgbench.pl b/src/bin/pgbench/t/001_pgbench.pl
new file mode 100644 (file)
index 0000000..88e83ab
--- /dev/null
+++ b/src/bin/pgbench/t/001_pgbench.pl
@@ -0,0 +1,25 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Test concurrent insertion into table with UNIQUE oid column.  DDL expects
+# GetNewOidWithIndex() to successfully avoid violating uniqueness for indexes
+# like pg_class_oid_index and pg_proc_oid_index.  This indirectly exercises
+# LWLock and spinlock concurrency.  This test makes a 5-MiB table.
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+$node->psql('postgres',
+       'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; '
+     . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);');
+my $script = $node->basedir . '/pgbench_script';
+append_to_file($script,
+   'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);');
+$node->command_like(
+   [   qw(pgbench --no-vacuum --client=5 --protocol=prepared
+         --transactions=25 --file), $script ],
+   qr{processed: 125/125},
+   'concurrent OID generation');

diff --git a/src/include/port/atomics/generic-xlc.h b/src/include/port/atomics/generic-xlc.h
index 1e8a11e291c983fab84557fff53b31f2b3929151..f24e3af5a7a8bd5b83a44a3fddb06d3db4637f6a 100644 (file)
--- a/src/include/port/atomics/generic-xlc.h
+++ b/src/include/port/atomics/generic-xlc.h
@@ -40,19 +40,23 @@ static inline bool
 pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
                                    uint32 *expected, uint32 newval)
 {
+   /*
+    * XXX: __compare_and_swap is defined to take signed parameters, but that
+    * shouldn't matter since we don't perform any arithmetic operations.
+    */
+   bool        ret = __compare_and_swap((volatile int*)&ptr->value,
+                                        (int *)expected, (int)newval);
+
    /*
     * xlc's documentation tells us:
     * "If __compare_and_swap is used as a locking primitive, insert a call to
     * the __isync built-in function at the start of any critical sections."
+    *
+    * The critical section begins immediately after __compare_and_swap().
     */
    __isync();
 
-   /*
-    * XXX: __compare_and_swap is defined to take signed parameters, but that
-    * shouldn't matter since we don't perform any arithmetic operations.
-    */
-   return __compare_and_swap((volatile int*)&ptr->value,
-                             (int *)expected, (int)newval);
+   return ret;
 }
 
 #define PG_HAVE_ATOMIC_FETCH_ADD_U32
@@ -69,10 +73,12 @@ static inline bool
 pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
                                    uint64 *expected, uint64 newval)
 {
+   bool        ret = __compare_and_swaplp((volatile long*)&ptr->value,
+                                          (long *)expected, (long)newval);
+
    __isync();
 
-   return __compare_and_swaplp((volatile long*)&ptr->value,
-                               (long *)expected, (long)newval);;
+   return ret;
 }
 
 #define PG_HAVE_ATOMIC_FETCH_ADD_U64

diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat
index feb0fe5b85e462f657344360aec9586e36e45049..d21692ffc9b3524fe4a74134e7466cade363f8c2 100755 (executable)
--- a/src/tools/msvc/clean.bat
+++ b/src/tools/msvc/clean.bat
@@ -94,6 +94,7 @@ if exist src\bin\pg_basebackup\tmp_check rd /s /q src\bin\pg_basebackup\tmp_chec
 if exist src\bin\pg_config\tmp_check rd /s /q src\bin\pg_config\tmp_check
 if exist src\bin\pg_ctl\tmp_check rd /s /q src\bin\pg_ctl\tmp_check
 if exist src\bin\pg_rewind\tmp_check rd /s /q src\bin\pg_rewind\tmp_check
+if exist src\bin\pgbench\tmp_check rd /s /q src\bin\pgbench\tmp_check
 if exist src\bin\scripts\tmp_check rd /s /q src\bin\scripts\tmp_check
 
 REM Clean up datafiles built with contrib