On Sat, Mar 24, 2018 at 11:01 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
>
>
> TBH this is not a heavily redesigned version. There were two parts to the
> original patch:
>
> 1. Replacing 8-byte xl_prev with 2-byte xl_walid in order to reduce the size
> of the WAL record header
> 2. Changing XLogCtlInsert.CurrBytePos to use atomic operations in order to
> reduce the spinlock contention.
>
> Most people expressed concerns regarding 1, but there were none regarding 2.
> Now it's possible that the entire attention got diverted to 1 and nobody
> really studied 2 in detail. Or may be 2 is mostly non-contentious, given the
> results of micro benchmarks.
>
> So what I've done in v2 is to just deal with part 2 i.e. replace access to
> CurrBytePos with atomic operations, based on the following suggestion by
> Simon.
>
> On 2/1/18 19:21, Simon Riggs wrote:
>> If we really can't persuade you of that, it doesn't sink the patch. We
>> can have the WAL pointer itself - it wouldn't save space but it would
>> at least alleviate the spinlock.
>
> This gives us the same level of guarantee that xl_prev used to offer, yet
> help us use atomic operations. I'll be happy if we can look at that
> particular change and see if there are any holes there.
>
Leaving aside the arguments about process, the patch is pretty small
and fairly straightforward. Given the claimed performance gains that's
a nice bang for the buck.
I haven't seen any obvious holes, but this is surely a case for as
many eyeballs as possible.
Some of the comments read a little oddly. We should talk about what we
are doing or not doing, but not about what we used to do and don't do
any longer, ISTM. e.g. instead of explaining that we used to have a
spinlock and don't have one any longer, just explain why we don't need
a spinlock.
All the regression and TAP tests pass happily.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
