Re: [HACKERS] A misconception about the meaning of 'volatile' in GetNewTransactionId?

On Sun, Apr 30, 2017 at 1:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
>> I was reading xact.c and noticed this block:
>> ...
>> Isn't this insufficient on non-TSO systems like POWER and Arm?
>
> Yeah, I think you're right. That code probably predates our support
> for memory barriers, so "volatile" was the best we could do at the
> time --- but as you say, it doesn't fix hardware-level rearrangements.

Here is an experimental patch, for discussion only, to drop some
apparently useless volatile qualifiers and introduce a write barrier
when extending the array and a corresponding read barrier when
scanning or copying the array from other processes.

I wonder about this code that shrinks the array:

#define XidCacheRemove(i) \
do { \
MyProc->subxids.xids[i] =
MyProc->subxids.xids[MyPgXact->nxids - 1]; \
MyPgXact->nxids--; \
} while (0)

If a concurrent process saw the decremented nxids value before seeing
the effect of xids[i] = xids[final], then it would miss an arbitrary
running subtransaction (not the aborting subtransaction being removed
from the array, but whichever xid had the bad luck to be in final
position). In the patch I added pg_write_barrier(), but I suspect
that that might be not really a problem because of higher level
interlocking that I'm missing, because this code makes no mention of
the problem and doesn't (ab)use volatile qualifiers like the code that
extends the array (so it has neither compiler barrier/volatile nor
memory barrier so could be broken even on TSO assumptions at the whim
of the compiler if my guess were right about that).

--
Thomas Munro
http://www.enterprisedb.com