Horton

Re: [HACKERS] Deadlock in XLogInsert at AIX

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Noah Misch <noah@leadboat.com>
Cc: Michael Paquier <michael.paquier@gmail.com>, Andres Freund <andres@anarazel.de>, Heikki Linnakangas <hlinnaka@iki.fi>, Konstantin Knizhnik <k.knizhnik@postgrespro.ru>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Bernd Helmle <mailings@oopsware.de>
Date: 2019-10-09T17:15:29Z
Lists: pgsql-hackers 
Noah Misch <noah@leadboat.com> writes:
> On Mon, Oct 07, 2019 at 03:06:35PM -0400, Tom Lane wrote:
>> This still fails on Apple's compilers. ...

> Thanks for testing.  That error boils down to "need to use some other
> register".  The second operand of addi is one of the ppc instruction operands
> that can hold a constant zero or a register number[1], so the proper
> constraint is "b".  I've made it so and added a comment.

Ah-hah.  This version does compile and pass check-world for me.

> I should probably
> update s_lock.h, too, in a later patch.  I don't know how it has
> mostly-avoided this failure mode, but its choice of constraint could explain
> https://postgr.es/m/flat/36E70B06-2C5C-11D8-A096-0005024EF27F%40ifrance.com

Indeed.  It's a bit astonishing that more people haven't hit that.
This should be back-patched.


Now that the patch passes mechanical checks, I took a closer look,
and there are some things I don't like:

* I still think that the added configure test is a waste of build cycles.
It'd be sufficient to test "#ifdef HAVE__BUILTIN_CONSTANT_P" where you
are testing HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P, because our previous
buildfarm go-round with this showed that all supported compilers
interpret "i" this way.

* I really dislike building the asm calls with macros as you've done
here.  The macros violate project style, and are not remotely general-
purpose, because they have hard-wired references to variables that are
not in their argument lists.  While that could be fixed with more
arguments, I don't think that the approach is readable or maintainable
--- it's impossible for example to understand the register constraints
without looking simultaneously at the calls and the macro definition.
And, as we've seen in this "b" issue, the interactions between the chosen
instruction types and the constraints are subtle enough to make me wonder
whether you won't need even more arguments to allow some of the other
constraints to be variable.  I think it'd be far better just to write out
the asm in-line and accept the not-very-large amount of code duplication
you'd get.

* src/tools/pginclude/headerscheck needs the same adjustment as you
made in cpluspluscheck.

			regards, tom lane

Commits

  1. For all ppc compilers, implement compare_exchange and fetch_add with asm.

  2. For PowerPC instruction "addi", use constraint "b".

  3. For all ppc compilers, implement pg_atomic_fetch_add_ with inline asm.

  4. Replace xlc __fetch_and_add() with inline asm.

  5. Test pg_atomic_fetch_add_ with variable addend and 16-bit edge cases.