Re: Skylake-S warning

Hi,

On 2018-10-05 10:29:55 -0700, Andres Freund wrote:
> - remove the volatiles from GetSnapshotData(). As we've, for quite a
> while now, made sure both lwlocks and spinlocks are proper barriers
> they're not needed.

Attached is a patch that removes all the volatiles from procarray.c and
varsup.c. I'd started to just remove them from GetSnapshotData(), but
that doesn't seem particularly consistent.

I actually think there's a bit of a correctness problem with the
previous state - the logic in GetNewTransactionId() relies on volatile
guaranteeing memory ordering, which it doesn't do:

* Use volatile pointer to prevent code rearrangement; other backends
* could be examining my subxids info concurrently, and we don't want
* them to see an invalid intermediate state, such as incrementing
* nxids before filling the array entry. Note we are assuming that
* TransactionId and int fetch/store are atomic.
*/
volatile PGPROC *myproc = MyProc;
volatile PGXACT *mypgxact = MyPgXact;

if (!isSubXact)
mypgxact->xid = xid;
else
{
int nxids = mypgxact->nxids;

if (nxids < PGPROC_MAX_CACHED_SUBXIDS)
{
myproc->subxids.xids[nxids] = xid;
mypgxact->nxids = nxids + 1;
}

I've replaced that with a write barrier / read barrier. I strongly
suspect this isn't a problem on the write side in practice (due to the
dependent read), but the read side looks much less clear to me. I think
explicitly using barriers is much saner these days.

Comments?

> - reduce the largely redundant flag tests. With the previous change done
> the compiler should be able to do so, but there's no reason to not
> start from somewhere sane. I'm kinda wondering about backpatching
> this part.

Done.

Greetings,

Andres Freund