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