On Sat, Jul 04, 2015 at 08:07:49PM -0400, Noah Misch wrote:
> On Sun, Jul 05, 2015 at 12:54:43AM +0200, Andres Freund wrote:
> > On 2015-07-04 18:40:41 -0400, Noah Misch wrote:
> > > (1) "IBM XL C/C++ for AIX, V12.1 (5765-J02, 5725-C72)".  Getting it 
> > > working
> > > required the attached patch.
> > Will you apply? Having the ability to test change seems to put you in a
> > much better spot then me.
> I will.

That commit (0d32d2e) permitted things to compile and usually pass tests, but
I missed the synchronization bug.  Since 2015-10-01, the buildfarm has seen
sixteen duplicate-catalog-OID failures.  The compiler has always been xlC, and
the branch has been HEAD or 9.5.  Examples:


These suggested OidGenLock wasn't doing its job.  I've seen similar symptoms
around WALInsertLocks with "IBM XL C/C++ for Linux, V13.1.2 (5725-C73,
5765-J08)" for ppc64le.  The problem is generic-xlc.h
pg_atomic_compare_exchange_u32_impl() issuing __isync() before
__compare_and_swap().  __isync() shall follow __compare_and_swap(); see our
own s_lock.h, its references, and other projects' usage:


This patch's test case would have failed about half the time under today's
generic-xlc.h.  Fast machines run it in about 1s.  A test that detects the bug
99% of the time would run far longer, hence this compromise.

Archaeology enthusiasts may enjoy the bug's changing presentation over time.
LWLockAcquire() exposed it after commit ab5194e redesigned lwlock.c in terms
of atomics.  Assert builds were unaffected for commits in [ab5194e, bbfd7ed);
atomics.h asserts fattened code enough to mask the bug.  However, removing the
AssertPointerAlignment() from pg_atomic_fetch_or_u32() would have sufficed to
surface it in assert builds.  All builds demonstrated the bug once bbfd7ed
introduced xlC pg_attribute_noreturn(), which elicits a simpler instruction
sequence around the ExceptionalCondition() call embedded in each Assert.

diff --git a/src/bin/pgbench/.gitignore b/src/bin/pgbench/.gitignore
index aae819e..983a3cd 100644
--- a/src/bin/pgbench/.gitignore
+++ b/src/bin/pgbench/.gitignore
@@ -1,3 +1,4 @@
diff --git a/src/bin/pgbench/Makefile b/src/bin/pgbench/Makefile
index 18fdf58..e9b1b74 100644
--- 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
+       $(prove_check)
+       $(prove_installcheck)
diff --git a/src/bin/pgbench/t/001_pgbench.pl b/src/bin/pgbench/t/001_pgbench.pl
new file mode 100644
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');
+           'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; '
+         . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);');
+my $script = $node->basedir . '/pgbench_script';
+       'INSERT INTO oid_tbl SELECT FROM generate_series(1,1000);');
+       [   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 
index 1e8a11e..f24e3af 100644
--- a/src/include/port/atomics/generic-xlc.h
+++ b/src/include/port/atomics/generic-xlc.h
@@ -41,18 +41,22 @@ pg_atomic_compare_exchange_u32_impl(volatile 
pg_atomic_uint32 *ptr,
*expected, uint32 newval)
+        * XXX: __compare_and_swap is defined to take signed parameters, but 
+        * 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 
         * the __isync built-in function at the start of any critical sections."
+        *
+        * The critical section begins immediately after __compare_and_swap().
-       /*
-        * XXX: __compare_and_swap is defined to take signed parameters, but 
-        * shouldn't matter since we don't perform any arithmetic operations.
-        */
-       return __compare_and_swap((volatile int*)&ptr->value,
-                                                         (int *)expected, 
+       return ret;
@@ -69,10 +73,12 @@ static inline bool
 pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
*expected, uint64 newval)
+       bool            ret = __compare_and_swaplp((volatile long*)&ptr->value,
   (long *)expected, (long)newval);
-       return __compare_and_swaplp((volatile long*)&ptr->value,
-                                                               (long 
*)expected, (long)newval);;
+       return ret;
diff --git a/src/tools/msvc/clean.bat b/src/tools/msvc/clean.bat
index feb0fe5..d21692f 100755
--- 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 
 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
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to