Re: Skylake-S warning

On Sat, Nov 10, 2018 at 6:01 PM Andres Freund <andres@anarazel.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