|From:||Andres Freund <andres(at)anarazel(dot)de>|
|To:||Daniel Wood <hexexpert(at)comcast(dot)net>|
|Subject:||Re: Skylake-S warning|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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;
mypgxact->xid = xid;
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.
> - 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.
|Next Message||David Rowley||2018-11-10 09:29:09||Re: Performance improvements of INSERTs to a partitioned table|
|Previous Message||Amit Kapila||2018-11-10 03:57:43||Re: Undo logs|