Re: Skylake-S warning

On Sat, Nov 10, 2018 at 6:01 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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?

+1

I said the same over here:

https://www.postgresql.org/message-id/flat/CAEepm%3D1nff0x%3D7i3YQO16jLA2qw-F9O39YmUew4oq-xcBQBs0g%40mail.gmail.com

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