Re: Move PinBuffer and UnpinBuffer to atomics

Hello hackers!

Continuing the theme: http://www.postgresql.org/message-id/3368228.mTSz6V0Jsq@dinodell

This time, we fairly rewrote 'refcount' and 'usage_count' to atomic in
PinBuffer and UnpinBuffer (but save lock for buffer flags in Unpin).

In the same time it doesn't affect to correctness of buffer manager
because that variables already have LWLock on top of them (for partition of
hashtable). If someone pinned buffer after the call StrategyGetBuffer we just
try again (in BufferAlloc). Also in the code there is one more check before
deleting the old buffer, where changes can be rolled back. The other functions
where it is checked 'refcount' and 'usage_count' put exclusive locks.

Also stress test with 256 KB shared memory ended successfully.

Without patch we have 417523 TPS and with patch 965821 TPS for big x86 server.
All details here: https://gist.github.com/stalkerg/773a81b79a27b4d5d63f

Thank you.
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2015-09-11 13:23:24 +0300, YUriy Zhuravlev wrote:
> Continuing the theme: http://www.postgresql.org/message-id/3368228.mTSz6V0Jsq@dinodell

Please don't just start new threads for a new version of the patch.

> This time, we fairly rewrote 'refcount' and 'usage_count' to atomic in
> PinBuffer and UnpinBuffer (but save lock for buffer flags in Unpin).

Hm.

> In the same time it doesn't affect to correctness of buffer manager
> because that variables already have LWLock on top of them (for partition of
> hashtable).

Note that there's a pending patch that removes the buffer mapping locks
entirely.

> If someone pinned buffer after the call StrategyGetBuffer we just try
> again (in BufferAlloc). Also in the code there is one more check
> before deleting the old buffer, where changes can be rolled back. The
> other functions where it is checked 'refcount' and 'usage_count' put
> exclusive locks.

I don't think this is correct. This way we can leave the for (;;) loop
in BufferAlloc() thinking that the buffer is unused (and can't be further
pinned because of the held spinlock!) while it actually has been pinned
since by PinBuffer(). Additionally oldFlags can get out of sync there.

I don't think the approach of making some of the fields atomics but not
really caring about the rest is going to work. My suggestion is to add a
single 'state' 32bit atomic. This 32bit state is subdivided into:

10bit for flags,
3bit for usage_count,
16bit for refcount

then turn each operation that currently uses one of these fields into
corresponding accesses (just different values for flags, bit-shiftery &
mask for reading usage count, bit mask for reading refcount). The trick
then is to add a *new* flag value BM_LOCKED. This can then act as a sort
of a 'one bit' spinlock.

That should roughly look like (more or less pseudocode):

void
LockBufHdr(BufferDesc *desc)
{
int state = pg_atomic_read_u32(&desc->state);

for (;;)
{
/* wait till lock is free */
while (unlikely(state & BM_LOCKED))
{
pg_spin_delay();
state = pg_atomic_read_u32(&desc->state);

/* add exponential backoff? Should seldomly be contended tho. */
}

/* and try to get lock */
if (pg_atomic_compare_exchange_u32(&desc->state, &state, state | BM_LOCKED))
break;
}
}

static bool
PinBuffer(volatile BufferDesc *buf, BufferAccessStrategy strategy)
{
...
if (ref == NULL)
{
ReservePrivateRefCountEntry();
ref = NewPrivateRefCountEntry(b);
...

int state = pg_atomic_read_u32(&desc->state);
int oldstate = state;

while (true)
{

/* spin-wait till lock is free */
while (unlikely(state & BM_LOCKED))
{
pg_spin_delay();
state = pg_atomic_read_u32(&desc->state);
}

/* increase refcount */
state += 1;

/* increase usagecount unless already max */
if ((state & USAGE_COUNT_MASK) != BM_MAX_USAGE_COUNT)
state += BM_USAGE_COUNT_ONE;

result = (state & BM_VALID) != 0;

if (pg_atomic_compare_exchange_u32(&desc->state, &oldstate, state))
break;

/* get ready for next loop, oldstate has been updated by cas */
state = oldstate;
}
...
}

other callsites can either just plainly continue to use
LockBufHdr/UnlockBufHdr or converted similarly to PinBuffer().

Greetings,

Andres Freund

On 11 September 2015 at 22:23, YUriy Zhuravlev <u(dot)zhuravlev(at)postgrespro(dot)ru>
wrote:

>
> Without patch we have 417523 TPS and with patch 965821 TPS for big x86
> server.
> All details here: https://gist.github.com/stalkerg/773a81b79a27b4d5d63f
>
>
Impressive!

I've run this on a single CPU server and don't see any speedup, so I assume
I'm not getting enough contention.
As soon as our 4 socket machine is free I'll try a pgbench run with that.

Just for fun, what's the results if you use -M prepared ?

Regards

David Rowley

--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Training & Services

On Friday 11 September 2015 18:14:21 Andres Freund wrote:
> This way we can leave the for (;;) loop
> in BufferAlloc() thinking that the buffer is unused (and can't be further
> pinned because of the held spinlock!)

We lost lock after PinBuffer_Locked in BufferAlloc. Therefore, in essence,
nothing has changed.
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2015-09-11 19:33:26 +0300, YUriy Zhuravlev wrote:
> On Friday 11 September 2015 18:14:21 Andres Freund wrote:
> > This way we can leave the for (;;) loop
> > in BufferAlloc() thinking that the buffer is unused (and can't be further
> > pinned because of the held spinlock!)
>
> We lost lock after PinBuffer_Locked in BufferAlloc. Therefore, in essence,
> nothing has changed.

The relevant piece of code is:
/*
* Need to lock the buffer header too in order to change its tag.
*/
LockBufHdr(buf);

/*
* Somebody could have pinned or re-dirtied the buffer while we were
* doing the I/O and making the new hashtable entry. If so, we can't
* recycle this buffer; we must undo everything we've done and start
* over with a new victim buffer.
*/
oldFlags = buf->flags;
if (buf->refcount == 1 && !(oldFlags & BM_DIRTY))
break;

UnlockBufHdr(buf);
BufTableDelete(&newTag, newHash);
if ((oldFlags & BM_TAG_VALID) &&
oldPartitionLock != newPartitionLock)
LWLockRelease(oldPartitionLock);
LWLockRelease(newPartitionLock);
UnpinBuffer(buf, true);
}

/*
* Okay, it's finally safe to rename the buffer.
*
* Clearing BM_VALID here is necessary, clearing the dirtybits is just
* paranoia. We also reset the usage_count since any recency of use of
* the old content is no longer relevant. (The usage_count starts out at
* 1 so that the buffer can survive one clock-sweep pass.)
*/
buf->tag = newTag;
buf->flags &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED | BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT);
if (relpersistence == RELPERSISTENCE_PERMANENT)
buf->flags |= BM_TAG_VALID | BM_PERMANENT;
else
buf->flags |= BM_TAG_VALID;
buf->usage_count = 1;

UnlockBufHdr(buf);

so unless I'm missing something, no, we haven't lost the lock.

Greetings,

Andres Freund

On Friday 11 September 2015 18:37:00 you wrote:
> so unless I'm missing something, no, we haven't lost the lock.
This section is protected by like LWLockAcquire(newPartitionLock,
LW_EXCLUSIVE); before it (and we can't get this buffer from hash table).

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2015-09-11 19:46:02 +0300, YUriy Zhuravlev wrote:
> On Friday 11 September 2015 18:37:00 you wrote:
> > so unless I'm missing something, no, we haven't lost the lock.
> This section is protected by like LWLockAcquire(newPartitionLock,
> LW_EXCLUSIVE); before it (and we can't get this buffer from hash table).

a) As I said upthread there's a patch to remove these locks entirely
b) It doesn't matter anyway. Not every pin goes through the buffer
mapping table. StrategyGetBuffer(), SyncOneBuffer(), ...

Greetings,

Andres Freund

On Friday 11 September 2015 18:50:35 you wrote:
> a) As I said upthread there's a patch to remove these locks entirely
It is very interesting. Could you provide a link? And it's not very good,
since there is a bottleneck PinBuffer / UnpinBuffer instead of LWLocks.
> b) It doesn't matter anyway. Not every pin goes through the buffer
> mapping table. StrategyGetBuffer(), SyncOneBuffer(), ...
StrategyGetBuffer call only from BufferAlloc .
SyncOneBuffer not problem too because:
PinBuffer_Locked(bufHdr);
LWLockAcquire(bufHdr->content_lock, LW_SHARED);
And please read comment before LockBufHdr(bufHdr) in SyncOneBuffer.

We checked all functions with refcount and usage_count.

Thanks! ^_^
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2015-09-14 13:16:46 +0300, YUriy Zhuravlev wrote:
> On Friday 11 September 2015 18:50:35 you wrote:

> > a) As I said upthread there's a patch to remove these locks entirely

> It is very interesting. Could you provide a link?

http://archives.postgresql.org/message-id/CA%2BTgmoYE4t-Pt%2Bv08kMO5u_XN-HNKBWtfMgcUXEGBrQiVgdV9Q%40mail.gmail.com

> And it's not very good,
> since there is a bottleneck PinBuffer / UnpinBuffer instead of
> LWLocks.

Where the bottleneck is entirely depends on your workload. If you have a
high cache replacement ratio the mapping partition locks are frequently
going to be held exclusively.

> > b) It doesn't matter anyway. Not every pin goes through the buffer
> > mapping table. StrategyGetBuffer(), SyncOneBuffer(), ...

> StrategyGetBuffer call only from BufferAlloc .

It gets called without buffer mapping locks held. And it can (and
frequently will!) access all the buffers in the buffer pool.

> SyncOneBuffer not problem too because:

> PinBuffer_Locked(bufHdr);

Which you made ineffective because PinBuffer() doesn't take a lock
anymore. Mutual exclusion through locks only works if all participants
take the locks.

> We checked all functions with refcount and usage_count.

Adding lockless behaviour by just taking out locks without analyzing the
whole isn't going to fly. You either need to provide backward
compatibility (a LockBuffer that provides actual exclusion) or you
actually need to go carefully through all relevant code and make it
lock-free.

I pointed out how you can actually make this safely lock-free giving you
the interesting code.

Greetings,

Andres Freund

On 2015-09-14 17:41:42 +0200, Andres Freund wrote:
> I pointed out how you can actually make this safely lock-free giving you
> the interesting code.

And here's an actual implementation of that approach. It's definitely
work-in-progress and could easily be optimized further. Don't have any
big machines to play around with right now tho.

Andres

On Tuesday 15 September 2015 04:06:25 Andres Freund wrote:
> And here's an actual implementation of that approach. It's definitely
> work-in-progress and could easily be optimized further. Don't have any
> big machines to play around with right now tho.
Thanks. Interesting.
We had a version like your patch. But this is only half the work. Example:
state = pg_atomic_read_u32(&buf->state);
if ((state & BUF_REFCOUNT_MASK) == 0
&& (state & BUF_USAGECOUNT_MASK) == 0)
After the first command somebody can change buf->state and local state not
actual.
In this embodiment, there is no significant difference between the two
patches. For honest work will need used the CAS for all IF statement.

Thanks! Hope for understanding. ^_^
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Saturday 12 September 2015 04:15:43 David Rowley wrote:
> I've run this on a single CPU server and don't see any speedup, so I assume
> I'm not getting enough contention.
> As soon as our 4 socket machine is free I'll try a pgbench run with that.
Excellent! Will wait.

> Just for fun, what's the results if you use -M prepared ?
Unfortunately now we can not check. :(

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Sep 14, 2015 at 9:06 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2015-09-14 17:41:42 +0200, Andres Freund wrote:
>> I pointed out how you can actually make this safely lock-free giving you
>> the interesting code.
>
> And here's an actual implementation of that approach. It's definitely
> work-in-progress and could easily be optimized further. Don't have any
> big machines to play around with right now tho.

Are you confident this is faster across all workloads? Pin/Unpin are
probably faster but this comes at a cost of extra atomic ops during
the clock sweep loop. I wonder if this will degrade results under
heavy contention.

Also, I'm curious about your introduction of __builtin_expect()
macros. Did you measure any gain from them? I bet there are other
places they could be used -- for example the mvcc hint bit checks on
xmin.

merlin

On 2015-09-15 12:51:24 +0300, YUriy Zhuravlev wrote:
> We had a version like your patch. But this is only half the work. Example:
> state = pg_atomic_read_u32(&buf->state);
> if ((state & BUF_REFCOUNT_MASK) == 0
> && (state & BUF_USAGECOUNT_MASK) == 0)

> After the first command somebody can change buf->state and local state not
> actual.

No, they can't in a a relevant manner. We hold the buffer header lock.

> In this embodiment, there is no significant difference between the two
> patches. For honest work will need used the CAS for all IF statement.

What?

> Thanks! Hope for understanding. ^_^

There's pretty little understanding left at this point. You're posting
things for review and you seem completely unwilling to actually respond
to points raised.

Greetings,

Andres Freund

On 2015-09-15 08:07:57 -0500, Merlin Moncure wrote:
> Are you confident this is faster across all workloads?

No. This is a proof of concept I just wrote & posted because I didn't
see the patch moving in the right direction. But I do think it can be
made faster in all relevant workloads.

> Pin/Unpin are probably faster but this comes at a cost of extra atomic
> ops during the clock sweep loop. I wonder if this will degrade
> results under heavy contention.

I think it's actually going to be faster under contention, and the
situation where it's slower is uncontended workloads where you a very
very low cache hit ratio.

> Also, I'm curious about your introduction of __builtin_expect()
> macros. Did you measure any gain from them?

I introduced them because I was bothered by the generated assembler ;)

But a bit more seriously, I do think there's some benefit in influencing
the code like that. I personally also find they *increase* readability
in cases like this where the likely() branch should be taken just about
all the time.

> I bet there are other places they could be used -- for example the
> mvcc hint bit checks on xmin.

I don't think those are good candidates, there's too many cases where
it's common to have the majority of cases go the other way.

Greetings,

Andres Freund

On Tue, Sep 15, 2015 at 9:56 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2015-09-15 08:07:57 -0500, Merlin Moncure wrote:
>> Also, I'm curious about your introduction of __builtin_expect()
>> macros. Did you measure any gain from them?
>
> I introduced them because I was bothered by the generated assembler ;)
>
> But a bit more seriously, I do think there's some benefit in influencing
> the code like that. I personally also find they *increase* readability
> in cases like this where the likely() branch should be taken just about
> all the time.

right. For posterity, I agree with this.

>> I bet there are other places they could be used -- for example the
>> mvcc hint bit checks on xmin.
>
> I don't think those are good candidates, there's too many cases where
> it's common to have the majority of cases go the other way.

Maybe, but, consider that penalty vs win is asymmetric. If the hint
bit isn't set, you're doing a lot of other work anyways such that the
branch penalty falls away to noise while if you win the benefits are
significant against the tight tuple scan loop.

Anyways, as it pertains to *this* patch, +1 for adding that feature.

merlin

On Tuesday 15 September 2015 16:50:44 Andres Freund wrote:
> No, they can't in a a relevant manner. We hold the buffer header lock.
I'm sorry, I did not notice of a LockBufHdr.

In this embodiment, your approach seems to be very similar to s_lock. Cycle in
PinBuffer behaves like s_lock.
In LockBufHdr:
if (pg_atomic_compare_exchange_u32(&desc->state, &state, state | BM_LOCKED))

conflict with:
while (unlikely(state & BM_LOCKED))
from PinBuffer.
Thus your patch does not remove the problem of competition for PinBuffer.
We will try check your patch this week.

>You're posting
>things for review and you seem completely unwilling to actually respond
>to points raised.
I think we're just talking about different things.
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2015-09-15 19:43:28 +0300, YUriy Zhuravlev wrote:
> On Tuesday 15 September 2015 16:50:44 Andres Freund wrote:
> > No, they can't in a a relevant manner. We hold the buffer header lock.
> I'm sorry, I did not notice of a LockBufHdr.
>
> In this embodiment, your approach seems to be very similar to s_lock. Cycle in
> PinBuffer behaves like s_lock.

> In LockBufHdr:
> if (pg_atomic_compare_exchange_u32(&desc->state, &state, state | BM_LOCKED))
>
> conflict with:
> while (unlikely(state & BM_LOCKED))
> from PinBuffer.
> Thus your patch does not remove the problem of competition for PinBuffer.
> We will try check your patch this week.

That path is only taken if somebody else has already locked the buffer
(e.g. BufferAlloc()). If you have contention in PinBuffer() your
workload will be mostly cache resident and neither PinBuffer() nor
UnpinBuffer() set BM_LOCKED.

Greetings,

Andres Freund

> That path is only taken if somebody else has already locked the buffer
> (e.g. BufferAlloc()). If you have contention in PinBuffer() your
> workload will be mostly cache resident and neither PinBuffer() nor
> UnpinBuffer() set BM_LOCKED.
Thanks. Now I understand everything. It might work.
We will be tested.

>your workload
Simple pgbench -S for NUMA.

--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2015-09-15 20:16:10 +0300, YUriy Zhuravlev wrote:
> We will be tested.

Did you have a chance to run some benchmarks?

Andres

On Thu, Sep 24, 2015 at 6:32 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2015-09-15 20:16:10 +0300, YUriy Zhuravlev wrote:
> > We will be tested.
>
> Did you have a chance to run some benchmarks?
>

Yes, we now have 60 physical cores intel server and we're running
benchmarks on it.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Sep 24, 2015 at 6:36 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Thu, Sep 24, 2015 at 6:32 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> On 2015-09-15 20:16:10 +0300, YUriy Zhuravlev wrote:
>> > We will be tested.
>>
>> Did you have a chance to run some benchmarks?
>>
>
> Yes, we now have 60 physical cores intel server and we're running
> benchmarks on it.
>

We got a consensus with Andres that we should commit the CAS version first
and look to other optimizations.
Refactored version of atomic state patch is attached. The changes are
following:
1) Macros are used for access refcount and usagecount.
2) likely/unlikely were removed. I think introducing of likely/unlikely
should be a separate patch since it touches portability. Also, I didn't see
any performance effect of this.
3) LockBufHdr returns the state after taking lock. Without using atomic
increments it still can save some loops on skip atomic value reading.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Oct 29, 2015 at 8:18 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Thu, Sep 24, 2015 at 6:36 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> On Thu, Sep 24, 2015 at 6:32 PM, Andres Freund <andres(at)anarazel(dot)de>
>> wrote:
>>
>>> On 2015-09-15 20:16:10 +0300, YUriy Zhuravlev wrote:
>>> > We will be tested.
>>>
>>> Did you have a chance to run some benchmarks?
>>>
>>
>> Yes, we now have 60 physical cores intel server and we're running
>> benchmarks on it.
>>
>
> We got a consensus with Andres that we should commit the CAS version first
> and look to other optimizations.
> Refactored version of atomic state patch is attached. The changes are
> following:
> 1) Macros are used for access refcount and usagecount.
> 2) likely/unlikely were removed. I think introducing of likely/unlikely
> should be a separate patch since it touches portability. Also, I didn't see
> any performance effect of this.
> 3) LockBufHdr returns the state after taking lock. Without using atomic
> increments it still can save some loops on skip atomic value reading.
>

pinunpin-cas-original-fix.patch is just original patch by Andres Freund
with fixed bug which causes hang.
Performance comparison on 72-cores Intel server in attached. On this
machine we see no regression in version of patch in previous letter.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

Thanks for benchmarking!

On 2015-10-30 16:28:22 +0300, Alexander Korotkov wrote:
> pinunpin-cas-original-fix.patch is just original patch by Andres Freund
> with fixed bug which causes hang.
> Performance comparison on 72-cores Intel server in attached. On this
> machine we see no regression in version of patch in previous letter.

So pinunpin-cas-original-fix is my version with a bug fixed, and
pinunpin-cas is what exactly? Your earlier version with the xadd +
cmpxchg?

The results look pretty good. Could you give a few more details about
the hardware and workload (i.e. cpu model number + scale)?

So the plan would be to finish cleaning this up into a committable
shape?

Greetings,

Andres Freund

Hi!

On Fri, Oct 30, 2015 at 5:12 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2015-10-30 16:28:22 +0300, Alexander Korotkov wrote:
> > pinunpin-cas-original-fix.patch is just original patch by Andres Freund
> > with fixed bug which causes hang.
> > Performance comparison on 72-cores Intel server in attached. On this
> > machine we see no regression in version of patch in previous letter.
>
> So pinunpin-cas-original-fix is my version with a bug fixed, and
> pinunpin-cas is what exactly? Your earlier version with the xadd +
> cmpxchg?
>

pinunpin-cas is still just cmpxchg with no xadd. It contain just minor
changes:

Refactored version of atomic state patch is attached. The changes are
> following:
> 1) Macros are used for access refcount and usagecount.
> 2) likely/unlikely were removed. I think introducing of likely/unlikely
> should be a separate patch since it touches portability. Also, I didn't see
> any performance effect of this.
> 3) LockBufHdr returns the state after taking lock. Without using atomic
> increments it still can save some loops on skip atomic value reading.

I compare them just to show there is no regression because of these changes.

> The results look pretty good. Could you give a few more details about
> the hardware and workload (i.e. cpu model number + scale)?
>

It is 4 socket Intel(R) Xeon(R) CPU E7-8890 v3 @ 2.50GHz, 2 Tb of memory,
all data in shared_buffers, 1000 scale factor, -M prepared, pgbench runs on
the same maching through unix socket.

> So the plan would be to finish cleaning this up into a committable
> shape?
>

Yes.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 10/29/2015 01:18 PM, Alexander Korotkov wrote:
> We got a consensus with Andres that we should commit the CAS version first
> and look to other optimizations.
> Refactored version of atomic state patch is attached. The changes are
> following:
> 1) Macros are used for access refcount and usagecount.
> 2) likely/unlikely were removed. I think introducing of likely/unlikely
> should be a separate patch since it touches portability. Also, I didn't see
> any performance effect of this.
> 3) LockBufHdr returns the state after taking lock. Without using atomic
> increments it still can save some loops on skip atomic value reading.
>

I have been testing this on a smaller system than yours - 2 socket
Intel(R) Xeon(R) CPU E5-2683 v3 w/ 2 x RAID10 SSD disks (data + xlog),
so focused on a smaller number of clients.

While I saw an improvement for the 'synchronous_commit = on' case -
there is a small regression for 'off', using -M prepared + Unix Domain
Socket. If that is something that should be considered right now.

Maybe it is worth to update the README to mention that the flags are
maintained in an atomic uint32 now.

BTW, there are two CommitFest entries for this submission:

https://commitfest.postgresql.org/7/370/
https://commitfest.postgresql.org/7/408/

Best regards,
Jesper

Hi,

On November 6, 2015 9:31:37 PM GMT+01:00, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com> wrote:
>I have been testing this on a smaller system than yours - 2 socket
>Intel(R) Xeon(R) CPU E5-2683 v3 w/ 2 x RAID10 SSD disks (data + xlog),
>so focused on a smaller number of clients.

Thanks for running tests!

>While I saw an improvement for the 'synchronous_commit = on' case -
>there is a small regression for 'off', using -M prepared + Unix Domain
>Socket. If that is something that should be considered right now.

What tests where you running, in which order? I presume it's a read/write pgbench? What scale, shared buffers?

I right now can't see any reason sc on/off should be relevant for the patch. Could it be an artifact of the order you ran tests in?

Did you initdb between tests? Pgbench -i? Restart the database?

Andres

---
Please excuse brevity and formatting - I am writing this on my mobile phone.

Hi,

On 11/06/2015 03:38 PM, Andres Freund wrote:
>> While I saw an improvement for the 'synchronous_commit = on' case -
>> there is a small regression for 'off', using -M prepared + Unix Domain
>> Socket. If that is something that should be considered right now.
>
> What tests where you running, in which order? I presume it's a read/write pgbench? What scale, shared buffers?
>

Scale is 3000, and shared buffer is 64Gb, effective is 160Gb.

Order was master/off -> master/on -> pinunpin/off -> pinunpin/on.

> I right now can't see any reason sc on/off should be relevant for the patch. Could it be an artifact of the order you ran tests in?
>

I was puzzled too, hence the post.

> Did you initdb between tests? Pgbench -i? Restart the database?

I didn't initdb / pgbench -i between the tests, so that it is likely it.

I'll redo.

Best regards,
Jesper

Hi,

On 11/06/2015 03:47 PM, Jesper Pedersen wrote:
>> Did you initdb between tests? Pgbench -i? Restart the database?
>
> I didn't initdb / pgbench -i between the tests, so that it is likely it.
>

Each graph has a full initdb + pgbench -i cycle now.

I know, I have a brown paper bag somewhere.

Best regards,
Jesper

On 2015-11-09 11:54:59 -0500, Jesper Pedersen wrote:
> Hi,
>
> On 11/06/2015 03:47 PM, Jesper Pedersen wrote:
> >>Did you initdb between tests? Pgbench -i? Restart the database?
> >
> >I didn't initdb / pgbench -i between the tests, so that it is likely it.
> >
>
> Each graph has a full initdb + pgbench -i cycle now.

That looks about as we'd expect: the lock-free pinning doesn't matter
and ssynchronous commit is beneficial. I think our bottlenecks in write
workloads are sufficiently elsewhere that it's unlikely that buffer pins
make a lot of difference.

You could try a readonly pgbench workload (i.e. -S), to see whether a
difference is visible there. For a pgbench -S workload it's more likely
that you only see significant contention on larger machines. If you've a
workload that touches more cached buffers, it'd be visible earlier.

> I know, I have a brown paper bag somewhere.

Why? This looks as expected, and the issues from the previous run were
easy to make mistakes?

Greetings,

Andres Freund

Hi,

On 11/09/2015 05:10 PM, Andres Freund wrote:
>> Each graph has a full initdb + pgbench -i cycle now.
>
> That looks about as we'd expect: the lock-free pinning doesn't matter
> and ssynchronous commit is beneficial. I think our bottlenecks in write
> workloads are sufficiently elsewhere that it's unlikely that buffer pins
> make a lot of difference.
>

Using

https://commitfest.postgresql.org/7/373/

shows that the CLog queue is max'ed out on the number of client connections.

> You could try a readonly pgbench workload (i.e. -S), to see whether a
> difference is visible there. For a pgbench -S workload it's more likely
> that you only see significant contention on larger machines. If you've a
> workload that touches more cached buffers, it'd be visible earlier.
>

Yeah, basically no difference between the 4 -S runs on this setup.

>> I know, I have a brown paper bag somewhere.
>
> Why? This looks as expected, and the issues from the previous run were
> easy to make mistakes?
>

I should have known to do the full cycle of initdb / pgbench -i in the
first place.

Best regards,
Jesper

Hi, Andres!

I'd like to share testing results on IBM E880
​ server​
.
​ At first, I'd like to mentioned that we did experiments with atomic
increment on power8, despite it doesn't have native atomic increment. Let
me explain why. CAS operation in power8 assembly looks like this.

.L1: lwarx 9,0,5
​ ​
cmpw 0,9,3
​ ​
bne- 0,.L2
​ ​
stwcx. 4,0,5
​ ​
bne- 0,.L1
.L2: isync

​So, it's a loop until value isn't changed between lwarx and stwcx. When
you put CAS inside loop it becomes loop inside loop. Atomic increment is a
single loop like CAS.

.L1: lwarx 9,0,5
​ ​
add 9,9,3
​ ​
stwcx. 9,0,5
​ ​
bne- 0,.L1
​ ​
isync

​This is why atomic increment *could be* cheaper than loop over CAS and, it
worth having experiments. ​Another idea is that we can put arbitrary logic
between lwarx and stwcx. Thus, we can implement PinBuffer using single loop
of lwarx and stwcx which could be better than loop of CAS.

​Tested patches are following:
1) pinunpin-cas.patch – moves pin/unpin buffer to atomic operations with
buffer state. PinBuffer uses loop of CAS. Same as upthread, but rebased
with current master.
2) pinunpin-increment.patch (based on pinunpin-cas.patch) – PinBuffer
changes state using atomic increment operation instead of loop of CAS. Both
refcount and usagecount are incremented at once. There is not enough bits
in 32 bit state to guarantee that usagecount doesn't overflow. I moved
usagecount to higher bits of state, thus overflow of usagecount doesn't
affect other bits. Therefore, this patch doesn't pretend to be correct.
However, we can use it to check if we should try moving this direction.
3) lwlock-increment.patch – LWLockAttemptLock change state using atomic
increment operation instead of loop of CAS. This patch does it for
LWLockAttemptLock like pinunpin-increment.patch does for PinBuffer.
Actually, this patch is not directly related to buffer manager. However,
it's nice to test loop of CAS vs atomic increment in different places.
4) ​pinunpin-ibm-asm.patch (based on pinunpin-cas.patch) – assembly
optimizations of PinBuffer and LWLockAttemptLock which makes both of them
use single loop of lwarx and stwcx.

​The following versions were compared in our benchmark.
​1) 9.4
2) 9.5
3) master
4) pinunpin-cas – ​pinunpin-cas.patch
5) pinunpin-increment – pinunpin-cas.patch + pinunpin-increment.patch
​6​
) pinunpin-cas-lwlock-increment – pinunpin-cas.patch +
lwlock-increment.patch
7​
) pinunpin-cas-lwlock-increment – pinunpin-cas.patch +
pinunpin-increment.patch
​ + ​
lwlock-increment.patch
​8) ​pinunpin-lwlock-asm – pinunpin-cas.patch + ​pinunpin-ibm-asm.patch

​See results in ibm-scalability.png. We can see that there is almost no
effect of pinunpin-increment.patch. So, atomic increment in PinBuffer
doesn't give benefit on power8. I will publish test results on Intel
platform a bit later, but I can say there is no much effect too. So, I
think, we can throw this idea away.
However, effect of lwlock-increment.patch is huge. It gives almost same
effect as assembly optimizations. I think it worth having separate thread
for discussing lwlock optimization. It's likely we can do something much
better than current lwlock-increment.patch does.
For this thread, I think we can focus on pinunpin-cas.patch without
thinking about atomic increment.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2015-12-08 12:53:49 +0300, Alexander Korotkov wrote:
> ​This is why atomic increment *could be* cheaper than loop over CAS and, it
> worth having experiments. ​Another idea is that we can put arbitrary logic
> between lwarx and stwcx. Thus, we can implement PinBuffer using single loop
> of lwarx and stwcx which could be better than loop of CAS.

You can't really put that much between an ll/sc - the hardware is only
able to track a very limited number of cacheline references.

> 3) lwlock-increment.patch – LWLockAttemptLock change state using atomic
> increment operation instead of loop of CAS. This patch does it for
> LWLockAttemptLock like pinunpin-increment.patch does for PinBuffer.
> Actually, this patch is not directly related to buffer manager. However,
> it's nice to test loop of CAS vs atomic increment in different places.

Yea, that's a worthwhile improvement. Actually it's how the first
versions of the lwlock patches worked - unfortunately I couldn't see big
differences on hardware I had available at the time.

There's some more trickyness required than what you have in your patch
(afaics at least). The problem is that when you 'optimistically'
increment by LW_VAL_SHARED and notice that there actually was another
locker, you possibly, until you've 'fixed' the state, are blocking new
exclusive lockers from acquiring the locks. So you additionally need to
do special handling in these cases, and check the queue more.

Greetings,

Andres Freund

On Tue, Dec 8, 2015 at 1:04 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2015-12-08 12:53:49 +0300, Alexander Korotkov wrote:
> > ​This is why atomic increment *could be* cheaper than loop over CAS and,
> it
> > worth having experiments. ​Another idea is that we can put arbitrary
> logic
> > between lwarx and stwcx. Thus, we can implement PinBuffer using single
> loop
> > of lwarx and stwcx which could be better than loop of CAS.
>
> You can't really put that much between an ll/sc - the hardware is only
> able to track a very limited number of cacheline references.
>

​I have some doubts about this, but I didn't find the place where it's​
​ explicitly documented. In the case of LWLockAttemptLock it not very much
between
lwarx/stwcx
​: 4 instructions while CAS have 2 instructions.
Could you please share some link to docs, if any?​

> > 3) lwlock-increment.patch – LWLockAttemptLock change state using atomic
> > increment operation instead of loop of CAS. This patch does it for
> > LWLockAttemptLock like pinunpin-increment.patch does for PinBuffer.
> > Actually, this patch is not directly related to buffer manager. However,
> > it's nice to test loop of CAS vs atomic increment in different places.
>
> Yea, that's a worthwhile improvement. Actually it's how the first
> versions of the lwlock patches worked - unfortunately I couldn't see big
> differences on hardware I had available at the time.
>
> There's some more trickyness required than what you have in your patch
> (afaics at least). The problem is that when you 'optimistically'
> increment by LW_VAL_SHARED and notice that there actually was another
> locker, you possibly, until you've 'fixed' the state, are blocking new
> exclusive lockers from acquiring the locks. So you additionally need to
> do special handling in these cases, and check the queue more.
>

​Agree. This patch need to be carefully verified. Current experiments just
show that it is promising direction for improvement. I'll come with better
version of this patch.

Also, after testing on large machines I have another observation to share.
For now, LWLock doesn't guarantee that exclusive lock would be ever
acquired (assuming each shared lock duration is finite). It because when
there is no exclusive lock, new shared locks aren't queued and LWLock state
is changed directly. Thus, process which tries to acquire exclusive lock
have to wait for gap in shared locks. But with high concurrency for shared
lock that could happen very rare, say never.

We did see this on big Intel machine in practice. pgbench -S gets shared
ProcArrayLock very frequently. Since some number of connections is
achieved, new connections hangs on getting exclusive ProcArrayLock. I think
we could do some workaround for this problem. For instance, when exclusive
lock waiter have some timeout it could set some special bit which prevents
others to get new shared locks.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Dec 8, 2015 at 3:56 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
> ​Agree. This patch need to be carefully verified. Current experiments just
> show that it is promising direction for improvement. I'll come with better
> version of this patch.
>
> Also, after testing on large machines I have another observation to share.
> For now, LWLock doesn't guarantee that exclusive lock would be ever
> acquired (assuming each shared lock duration is finite). It because when
> there is no exclusive lock, new shared locks aren't queued and LWLock state
> is changed directly. Thus, process which tries to acquire exclusive lock
> have to wait for gap in shared locks.
>

I think this has the potential to starve exclusive lockers in worst case.

> But with high concurrency for shared lock that could happen very rare, say
> never.
>
> We did see this on big Intel machine in practice. pgbench -S gets shared
> ProcArrayLock very frequently. Since some number of connections is
> achieved, new connections hangs on getting exclusive ProcArrayLock. I think
> we could do some workaround for this problem. For instance, when exclusive
> lock waiter have some timeout it could set some special bit which prevents
> others to get new shared locks.
>
>
I think timeout based solution would lead to giving priority to
exclusive lock waiters (assume a case where each of exclusive
lock waiter timesout one after another) and make shared lockers
wait and a timer based solution might turn out to be costly for
general cases where wait is not so long. Another way could be to
check if the Exclusive locker needs to go for repeated wait for a
couple of times, then we can set such a bit.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Dec 8, 2015 at 6:00 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Tue, Dec 8, 2015 at 3:56 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>>
>> ​Agree. This patch need to be carefully verified. Current experiments
>> just show that it is promising direction for improvement. I'll come with
>> better version of this patch.
>>
>> Also, after testing on large machines I have another observation to
>> share. For now, LWLock doesn't guarantee that exclusive lock would be ever
>> acquired (assuming each shared lock duration is finite). It because when
>> there is no exclusive lock, new shared locks aren't queued and LWLock state
>> is changed directly. Thus, process which tries to acquire exclusive lock
>> have to wait for gap in shared locks.
>>
>
> I think this has the potential to starve exclusive lockers in worst case.
>
>
>> But with high concurrency for shared lock that could happen very rare,
>> say never.
>>
>> We did see this on big Intel machine in practice. pgbench -S gets shared
>> ProcArrayLock very frequently. Since some number of connections is
>> achieved, new connections hangs on getting exclusive ProcArrayLock. I think
>> we could do some workaround for this problem. For instance, when exclusive
>> lock waiter have some timeout it could set some special bit which prevents
>> others to get new shared locks.
>>
>>
> I think timeout based solution would lead to giving priority to
> exclusive lock waiters (assume a case where each of exclusive
> lock waiter timesout one after another) and make shared lockers
> wait and a timer based solution might turn out to be costly for
> general cases where wait is not so long.
>

​Since all lwlock waiters are ordered in the queue, we can let only first
waiter to set this bit.​
Anyway, once bit is set, shared lockers would be added to the queue. They
would get the lock in queue order.

> Another way could be to
> check if the Exclusive locker needs to go for repeated wait for a
> couple of times, then we can set such a bit.
>

​I'm not sure what do you mean by repeated wait. Do you mean exclusive
locker was waked twice up by timeout? Because now, without timeout,
exclusive locker wouldn't be waked up until all shared locks are released.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Dec 9, 2015 at 2:17 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Tue, Dec 8, 2015 at 6:00 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Tue, Dec 8, 2015 at 3:56 PM, Alexander Korotkov <
>> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>>>
>>> ​Agree. This patch need to be carefully verified. Current experiments
>>> just show that it is promising direction for improvement. I'll come with
>>> better version of this patch.
>>>
>>> Also, after testing on large machines I have another observation to
>>> share. For now, LWLock doesn't guarantee that exclusive lock would be ever
>>> acquired (assuming each shared lock duration is finite). It because when
>>> there is no exclusive lock, new shared locks aren't queued and LWLock state
>>> is changed directly. Thus, process which tries to acquire exclusive lock
>>> have to wait for gap in shared locks.
>>>
>>
>> I think this has the potential to starve exclusive lockers in worst case.
>>
>>
>>> But with high concurrency for shared lock that could happen very rare,
>>> say never.
>>>
>>> We did see this on big Intel machine in practice. pgbench -S gets shared
>>> ProcArrayLock very frequently. Since some number of connections is
>>> achieved, new connections hangs on getting exclusive ProcArrayLock. I think
>>> we could do some workaround for this problem. For instance, when exclusive
>>> lock waiter have some timeout it could set some special bit which prevents
>>> others to get new shared locks.
>>>
>>>
>> I think timeout based solution would lead to giving priority to
>> exclusive lock waiters (assume a case where each of exclusive
>> lock waiter timesout one after another) and make shared lockers
>> wait and a timer based solution might turn out to be costly for
>> general cases where wait is not so long.
>>
>
> ​Since all lwlock waiters are ordered in the queue, we can let only first
> waiter to set this bit.​
>

Thats okay, but still every time an Exclusive locker woke up, the
threshold time for its wait might be already over and it will set the
bit. In theory, that looks okay, but as compare to current algorithm
it will make more shared lockers to be added into wait queue.

> Anyway, once bit is set, shared lockers would be added to the queue. They
> would get the lock in queue order.
>
>

Ye thats right, but I think in general the solution to this problem
should be don't let any Exclusive locker to starve and still allow
as many shared lockers as possible. I think here it is important
how we define starving, should it be based on time or something
else? I find timer based solution somewhat less suitable, but may
be it is okay, if there is no other better way.

> Another way could be to
>> check if the Exclusive locker needs to go for repeated wait for a
>> couple of times, then we can set such a bit.
>>
>
> ​I'm not sure what do you mean by repeated wait. Do you mean exclusive
> locker was waked twice up by timeout?
>

I mean to say once the Exclusive locker is woken up, it again
re-tries to acquire the lock as it does today, but if it finds that the
number of retries is greater than certain threshold (let us say 10),
then we sit the bit.

> Because now, without timeout, exclusive locker wouldn't be waked up until
> all shared locks are released.
>
>
Does LWLockWakeup() work that way? I thought it works such
that once an Exclusive locker is encountered in the wait queue, it
just wakes that and won't try to wake any further waiters.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Dec 10, 2015 at 9:26 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Wed, Dec 9, 2015 at 2:17 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> On Tue, Dec 8, 2015 at 6:00 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>>
>>> On Tue, Dec 8, 2015 at 3:56 PM, Alexander Korotkov <
>>> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>>>>
>>>> ​Agree. This patch need to be carefully verified. Current experiments
>>>> just show that it is promising direction for improvement. I'll come with
>>>> better version of this patch.
>>>>
>>>> Also, after testing on large machines I have another observation to
>>>> share. For now, LWLock doesn't guarantee that exclusive lock would be ever
>>>> acquired (assuming each shared lock duration is finite). It because when
>>>> there is no exclusive lock, new shared locks aren't queued and LWLock state
>>>> is changed directly. Thus, process which tries to acquire exclusive lock
>>>> have to wait for gap in shared locks.
>>>>
>>>
>>> I think this has the potential to starve exclusive lockers in worst case.
>>>
>>>
>>>> But with high concurrency for shared lock that could happen very rare,
>>>> say never.
>>>>
>>>> We did see this on big Intel machine in practice. pgbench -S gets
>>>> shared ProcArrayLock very frequently. Since some number of connections is
>>>> achieved, new connections hangs on getting exclusive ProcArrayLock. I think
>>>> we could do some workaround for this problem. For instance, when exclusive
>>>> lock waiter have some timeout it could set some special bit which prevents
>>>> others to get new shared locks.
>>>>
>>>>
>>> I think timeout based solution would lead to giving priority to
>>> exclusive lock waiters (assume a case where each of exclusive
>>> lock waiter timesout one after another) and make shared lockers
>>> wait and a timer based solution might turn out to be costly for
>>> general cases where wait is not so long.
>>>
>>
>> ​Since all lwlock waiters are ordered in the queue, we can let only first
>> waiter to set this bit.​
>>
>
> Thats okay, but still every time an Exclusive locker woke up, the
> threshold time for its wait might be already over and it will set the
> bit. In theory, that looks okay, but as compare to current algorithm
> it will make more shared lockers to be added into wait queue.
>
>
>> Anyway, once bit is set, shared lockers would be added to the queue. They
>> would get the lock in queue order.
>>
>>
>
> Ye thats right, but I think in general the solution to this problem
> should be don't let any Exclusive locker to starve and still allow
> as many shared lockers as possible. I think here it is important
> how we define starving, should it be based on time or something
> else? I find timer based solution somewhat less suitable, but may
> be it is okay, if there is no other better way.
>

​Yes, we probably should find something better.​

Another way could be to
>>> check if the Exclusive locker needs to go for repeated wait for a
>>> couple of times, then we can set such a bit.
>>>
>>
>> ​I'm not sure what do you mean by repeated wait. Do you mean exclusive
>> locker was waked twice up by timeout?
>>
>
> I mean to say once the Exclusive locker is woken up, it again
> re-tries to acquire the lock as it does today, but if it finds that the
> number of retries is greater than certain threshold (let us say 10),
> then we sit the bit.
>

​Yes, there is a cycle with retries in LWLockAcquire function. The case of
retry is when ​waiter is waked up, but someone other steal the lock before
him. Lock waiter is waked up by lock releaser only when lock becomes free.
But in the case of high concurrency for shared lock, it almost never
becomes free. So, exclusive locker would be never waked up. I'm pretty sure
this happens on big Intel machine while we do the benchmark. So, relying on
number of retries wouldn't work in this case.
I'll do the tests to verify if retries happens in our case.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2015-12-11 15:56:46 +0300, Alexander Korotkov wrote:
> On Thu, Dec 10, 2015 at 9:26 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >>>> We did see this on big Intel machine in practice. pgbench -S gets
> >>>> shared ProcArrayLock very frequently. Since some number of connections is
> >>>> achieved, new connections hangs on getting exclusive ProcArrayLock. I think
> >>>> we could do some workaround for this problem. For instance, when exclusive
> >>>> lock waiter have some timeout it could set some special bit which prevents
> >>>> others to get new shared locks.

> > Ye thats right, but I think in general the solution to this problem
> > should be don't let any Exclusive locker to starve and still allow
> > as many shared lockers as possible. I think here it is important
> > how we define starving, should it be based on time or something
> > else? I find timer based solution somewhat less suitable, but may
> > be it is okay, if there is no other better way.
> >
>
> ​Yes, we probably should find something better.​

> Another way could be to
> >>> check if the Exclusive locker needs to go for repeated wait for a
> >>> couple of times, then we can set such a bit.
> >>>
> >>
> >> ​I'm not sure what do you mean by repeated wait. Do you mean exclusive
> >> locker was waked twice up by timeout?
> >>
> >
> > I mean to say once the Exclusive locker is woken up, it again
> > re-tries to acquire the lock as it does today, but if it finds that the
> > number of retries is greater than certain threshold (let us say 10),
> > then we sit the bit.
> >
>
> ​Yes, there is a cycle with retries in LWLockAcquire function. The case of
> retry is when ​waiter is waked up, but someone other steal the lock before
> him. Lock waiter is waked up by lock releaser only when lock becomes free.
> But in the case of high concurrency for shared lock, it almost never
> becomes free. So, exclusive locker would be never waked up. I'm pretty sure
> this happens on big Intel machine while we do the benchmark. So, relying on
> number of retries wouldn't work in this case.
> I'll do the tests to verify if retries happens in our case.

I seriously doubt that making lwlocks fairer is the right way to go
here. In my testing the "unfairness" is essential to performance - the
number of context switches otherwise increases massively.

I think in this case its better to work on making the lock less
contended, rather than making micro-optimizations around the locking
behaviour.

Andres

On Fri, Dec 11, 2015 at 6:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2015-12-11 15:56:46 +0300, Alexander Korotkov wrote:
> >
> > Yes, there is a cycle with retries in LWLockAcquire function. The case
of
> > retry is when waiter is waked up, but someone other steal the lock
before
> > him. Lock waiter is waked up by lock releaser only when lock becomes
free.
> > But in the case of high concurrency for shared lock, it almost never
> > becomes free. So, exclusive locker would be never waked up. I'm pretty
sure
> > this happens on big Intel machine while we do the benchmark. So,
relying on
> > number of retries wouldn't work in this case.
> > I'll do the tests to verify if retries happens in our case.

makes sense and if retries never happen, then I think changing
LWLockRelease()
such that it should wake the waiters if there are waiters on a lock and it
has not
waked them for some threshold number of times or something like that might
work.

>
> I seriously doubt that making lwlocks fairer is the right way to go
> here. In my testing the "unfairness" is essential to performance - the
> number of context switches otherwise increases massively.
>

Agreed, if the change being discussed hurts in any kind of scenario, then
we should better not do it, OTOH the case described by Alexander seems
to be genuine and I have seen similar complaint by customer in the past
for another database I worked with and the reason for the problem is same.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Dec 13, 2015 at 11:05 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Dec 11, 2015 at 6:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>
>> On 2015-12-11 15:56:46 +0300, Alexander Korotkov wrote:
>> >
>> > Yes, there is a cycle with retries in LWLockAcquire function. The case
>> > of
>> > retry is when waiter is waked up, but someone other steal the lock
>> > before
>> > him. Lock waiter is waked up by lock releaser only when lock becomes
>> > free.
>> > But in the case of high concurrency for shared lock, it almost never
>> > becomes free. So, exclusive locker would be never waked up. I'm pretty
>> > sure
>> > this happens on big Intel machine while we do the benchmark. So, relying
>> > on
>> > number of retries wouldn't work in this case.
>> > I'll do the tests to verify if retries happens in our case.
>
> makes sense and if retries never happen, then I think changing
> LWLockRelease()
> such that it should wake the waiters if there are waiters on a lock and it
> has not
> waked them for some threshold number of times or something like that might
> work.
>
>>
>> I seriously doubt that making lwlocks fairer is the right way to go
>> here. In my testing the "unfairness" is essential to performance - the
>> number of context switches otherwise increases massively.
>>
>
> Agreed, if the change being discussed hurts in any kind of scenario, then
> we should better not do it, OTOH the case described by Alexander seems
> to be genuine and I have seen similar complaint by customer in the past
> for another database I worked with and the reason for the problem is same.

I have moved this patch to next CF..
--
Michael

On Thu, Dec 24, 2015 at 8:26 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Sun, Dec 13, 2015 at 11:05 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > On Fri, Dec 11, 2015 at 6:34 PM, Andres Freund <andres(at)anarazel(dot)de>
> wrote:
>

I was looking into this patch, overall patch looks good to me, I want to
know what is the current state of this patch, is there some pending task in
this patch ?

Patch was not applying on head so i have re based it and re based version is
attached in the mail.

I have done some performance testing also..

Summary:
---------------
1. In my test for readonly workload i have observed some improvement for
scale factor 1000.
2. I have also observed some regression with scale factor 300 (I can't say
it's actual regression or just run to run variance), I thought that may be
problem with lower scale factor so tested with scale factor 100 but with
s.f. 100 looks fine.

Machine Detail:
cpu : POWER8
cores: 24 (192 with HT)

Non Default Parameter:
------------------------
Shared Buffer= 30GB
max_wal_size= 10GB
max_connections=500

Test1:
pgbench -i -s 1000 postgres
pgbench -c$ -j$ -Mprepared -S postgres

Client Base Pached

1 19753 19493
32 344059 336773
64 495708 540425
128 564358 685212
256 466562 639059

Test2:
pgbench -i -s 300 postgres
pgbench -c$ -j$ -Mprepared -S postgres

Client Base Pached

1 20555 19404
32 375919 332670
64 509067 440680
128 431346 415121
256 380926 379176

Test3:
pgbench -i -s 100 postgres
pgbench -c$ -j$ -Mprepared -S postgres

Client Base Pached

1 20555 19404
32 375919 332670
64 509067 440680
128 431346 415121
256 380926 379176

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jan 19, 2016 at 7:31 PM, Dilip Kumar wrote:
> Test2:
> pgbench -i -s 300 postgres
> pgbench -c$ -j$ -Mprepared -S postgres
>
> Client Base Pached
>
> 1 20555 19404
> 32 375919 332670
> 64 509067 440680
> 128 431346 415121
> 256 380926 379176
>
> Test3:
> pgbench -i -s 100 postgres
> pgbench -c$ -j$ -Mprepared -S postgres
>
> Client Base Pached
>
> 1 20555 19404
> 32 375919 332670
> 64 509067 440680
> 128 431346 415121
> 256 380926 379176

It seems like you did a copy-paste of the results with s=100 and
s=300. Both are showing the exact same numbers.
--
Michael

On Tue, Jan 19, 2016 at 5:44 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> > Test3:
> > pgbench -i -s 100 postgres
> > pgbench -c$ -j$ -Mprepared -S postgres
> >
> > Client Base Pached
> >
> > 1 20555 19404
> > 32 375919 332670
> > 64 509067 440680
> > 128 431346 415121
> > 256 380926 379176
>
> It seems like you did a copy-paste of the results with s=100 and
> s=300. Both are showing the exact same numbers.

Oops, my mistake, re-pasting the correct results for s=100

pgbench -i -s 100 postgres
pgbench -c$ -j$ -Mprepared -S postgres

Client Base Pached

1 20548 20791
32 372633 355356
64 532052 552148
128 412755 478826
256 346701 372057

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi, Dilip!

On Tue, Jan 19, 2016 at 6:00 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> On Tue, Jan 19, 2016 at 5:44 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com> wrote:
>
>> > Test3:
>> > pgbench -i -s 100 postgres
>> > pgbench -c$ -j$ -Mprepared -S postgres
>> >
>> > Client Base Pached
>> >
>> > 1 20555 19404
>> > 32 375919 332670
>> > 64 509067 440680
>> > 128 431346 415121
>> > 256 380926 379176
>>
>> It seems like you did a copy-paste of the results with s=100 and
>> s=300. Both are showing the exact same numbers.
>
>
> Oops, my mistake, re-pasting the correct results for s=100
>
> pgbench -i -s 100 postgres
> pgbench -c$ -j$ -Mprepared -S postgres
>
> Client Base Pached
>
> 1 20548 20791
> 32 372633 355356
> 64 532052 552148
> 128 412755 478826
> 256 346701 372057
>

Could you please re-run these tests few times?
Just to be sure it's a reproducible regression with s=300 and not a
statistical error.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Jan 29, 2016 at 3:17 PM, Alexander Korotkov
<a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> Hi, Dilip!
>
> On Tue, Jan 19, 2016 at 6:00 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>>
>>
>> On Tue, Jan 19, 2016 at 5:44 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>
>>> > Test3:
>>> > pgbench -i -s 100 postgres
>>> > pgbench -c$ -j$ -Mprepared -S postgres
>>> >
>>> > Client Base Pached
>>> >
>>> > 1 20555 19404
>>> > 32 375919 332670
>>> > 64 509067 440680
>>> > 128 431346 415121
>>> > 256 380926 379176
>>>
>>> It seems like you did a copy-paste of the results with s=100 and
>>> s=300. Both are showing the exact same numbers.
>>
>>
>> Oops, my mistake, re-pasting the correct results for s=100
>>
>> pgbench -i -s 100 postgres
>> pgbench -c$ -j$ -Mprepared -S postgres
>>
>> Client Base Pached
>>
>> 1 20548 20791
>> 32 372633 355356
>> 64 532052 552148
>> 128 412755 478826
>> 256 346701 372057
>
>
> Could you please re-run these tests few times?
> Just to be sure it's a reproducible regression with s=300 and not a
> statistical error.

Probably want to run for at least 5 minutes via -T 300

merlin

On Sat, Jan 30, 2016 at 3:05 AM, Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:

> Probably want to run for at least 5 minutes via -T 300

Last time i run for 5 minutes and taken median of three runs, just missed
mentioning "-T 300" in the mail..

By looking at the results with scale factor 1000 and 100 i don't see any
reason why it will regress with scale factor 300.

So I will run the test again with scale factor 300 and this time i am
planning to run 2 cases.
1. when data fits in shared buffer
2. when data doesn't fit in shared buffer.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sun, Jan 31, 2016 at 11:44 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> By looking at the results with scale factor 1000 and 100 i don't see any
> reason why it will regress with scale factor 300.
>
> So I will run the test again with scale factor 300 and this time i am
> planning to run 2 cases.
> 1. when data fits in shared buffer
> 2. when data doesn't fit in shared buffer.
>

I have run the test again with 300 S.F and found no regression, in fact
there is improvement with the patch like we saw with 1000 scale factor.

Shared Buffer= 8GB
max_connections=150
Scale Factor=300

./pgbench -j$ -c$ -T300 -M prepared -S postgres

Client Base Patch
1 19744 19382
8 125923 126395
32 313931 333351
64 387339 496830
128 306412 350610

Shared Buffer= 512MB
max_connections=150
Scale Factor=300

./pgbench -j$ -c$ -T300 -M prepared -S postgres

Client Base Patch
1 17169 16454
8 108547 105559
32 241619 262818
64 206868 233606
128 137084 217013

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, Feb 1, 2016 at 7:05 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> On Sun, Jan 31, 2016 at 11:44 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
>
>> By looking at the results with scale factor 1000 and 100 i don't see any
>> reason why it will regress with scale factor 300.
>>
>> So I will run the test again with scale factor 300 and this time i am
>> planning to run 2 cases.
>> 1. when data fits in shared buffer
>> 2. when data doesn't fit in shared buffer.
>>
>
> I have run the test again with 300 S.F and found no regression, in fact
> there is improvement with the patch like we saw with 1000 scale factor.
>
> Shared Buffer= 8GB
> max_connections=150
> Scale Factor=300
>
> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>
> Client Base Patch
> 1 19744 19382
> 8 125923 126395
> 32 313931 333351
> 64 387339 496830
> 128 306412 350610
>
> Shared Buffer= 512MB
> max_connections=150
> Scale Factor=300
>
> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>
> Client Base Patch
> 1 17169 16454
> 8 108547 105559
> 32 241619 262818
> 64 206868 233606
> 128 137084 217013
>

Great, thanks!

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Feb 1, 2016 at 11:34 AM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Mon, Feb 1, 2016 at 7:05 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
>>
>> On Sun, Jan 31, 2016 at 11:44 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
>> wrote:
>>
>>> By looking at the results with scale factor 1000 and 100 i don't see any
>>> reason why it will regress with scale factor 300.
>>>
>>> So I will run the test again with scale factor 300 and this time i am
>>> planning to run 2 cases.
>>> 1. when data fits in shared buffer
>>> 2. when data doesn't fit in shared buffer.
>>>
>>
>> I have run the test again with 300 S.F and found no regression, in fact
>> there is improvement with the patch like we saw with 1000 scale factor.
>>
>> Shared Buffer= 8GB
>> max_connections=150
>> Scale Factor=300
>>
>> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>>
>> Client Base Patch
>> 1 19744 19382
>> 8 125923 126395
>> 32 313931 333351
>> 64 387339 496830
>> 128 306412 350610
>>
>> Shared Buffer= 512MB
>> max_connections=150
>> Scale Factor=300
>>
>> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>>
>> Client Base Patch
>> 1 17169 16454
>> 8 108547 105559
>> 32 241619 262818
>> 64 206868 233606
>> 128 137084 217013
>>
>
> Great, thanks!
>

Attached patch is rebased and have better comments.
Also, there is one comment which survive since original version by Andres.

/* Add exponential backoff? Should seldomly be contended tho. */

Andres, did you mean we should twice the delay with each unsuccessful try
to lock?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

>

Alexander Korotkov wrote:

> Attached patch is rebased and have better comments.
> Also, there is one comment which survive since original version by Andres.
>
> /* Add exponential backoff? Should seldomly be contended tho. */
>
>
> Andres, did you mean we should twice the delay with each unsuccessful try
> to lock?

This is probably a tough patch to review; trying to break it with low
number of shared buffers and high concurrency might be an interesting
exercise.

I know Andres is already pretty busy with the checkpoint flush patch and
I very much doubt he will be able to give this patch a lot of attention
in the short term. Moving to next CF.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-02-01 13:06:57 +0300, Alexander Korotkov wrote:
> On Mon, Feb 1, 2016 at 11:34 AM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> >> Client Base Patch
> >> 1 19744 19382
> >> 8 125923 126395
> >> 32 313931 333351
> >> 64 387339 496830
> >> 128 306412 350610
> >>
> >> Shared Buffer= 512MB
> >> max_connections=150
> >> Scale Factor=300
> >>
> >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
> >>
> >> Client Base Patch
> >> 1 17169 16454
> >> 8 108547 105559
> >> 32 241619 262818
> >> 64 206868 233606
> >> 128 137084 217013

So, there's a small regression on low client counts. That's worth
addressing.

> Attached patch is rebased and have better comments.
> Also, there is one comment which survive since original version by Andres.
>
> /* Add exponential backoff? Should seldomly be contended tho. */
>
>
> Andres, did you mean we should twice the delay with each unsuccessful try
> to lock?

Spinning on a lock as fast as possible leads to rapid cacheline bouncing
without anybody making progress. See s_lock() in s_lock.c.

Greetings,

Andres Freund

On 2016-02-01 22:35:06 +0100, Alvaro Herrera wrote:
> I know Andres is already pretty busy with the checkpoint flush patch and
> I very much doubt he will be able to give this patch a lot of attention
> in the short term. Moving to next CF.

Yea, there's no realistic chance I'll be able to take care of this in
the next couple days.

On Tue, Feb 2, 2016 at 12:43 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-02-01 13:06:57 +0300, Alexander Korotkov wrote:
> > On Mon, Feb 1, 2016 at 11:34 AM, Alexander Korotkov <
> > a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> > >> Client Base Patch
> > >> 1 19744 19382
> > >> 8 125923 126395
> > >> 32 313931 333351
> > >> 64 387339 496830
> > >> 128 306412 350610
> > >>
> > >> Shared Buffer= 512MB
> > >> max_connections=150
> > >> Scale Factor=300
> > >>
> > >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
> > >>
> > >> Client Base Patch
> > >> 1 17169 16454
> > >> 8 108547 105559
> > >> 32 241619 262818
> > >> 64 206868 233606
> > >> 128 137084 217013
>
> So, there's a small regression on low client counts. That's worth
> addressing.
>

Interesting. I'll try to reproduce it.

> > Attached patch is rebased and have better comments.
> > Also, there is one comment which survive since original version by
> Andres.
> >
> > /* Add exponential backoff? Should seldomly be contended tho. */
> >
> >
> > Andres, did you mean we should twice the delay with each unsuccessful try
> > to lock?
>
> Spinning on a lock as fast as possible leads to rapid cacheline bouncing
> without anybody making progress. See s_lock() in s_lock.c.
>

I didn't notice that s_lock() behaves so. Thank you.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2016-02-02 13:12:50 +0300, Alexander Korotkov wrote:
> On Tue, Feb 2, 2016 at 12:43 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > On 2016-02-01 13:06:57 +0300, Alexander Korotkov wrote:
> > > On Mon, Feb 1, 2016 at 11:34 AM, Alexander Korotkov <
> > > a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> > > >> Client Base Patch
> > > >> 1 19744 19382
> > > >> 8 125923 126395
> > > >> 32 313931 333351
> > > >> 64 387339 496830
> > > >> 128 306412 350610
> > > >>
> > > >> Shared Buffer= 512MB
> > > >> max_connections=150
> > > >> Scale Factor=300
> > > >>
> > > >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
> > > >>
> > > >> Client Base Patch
> > > >> 1 17169 16454
> > > >> 8 108547 105559
> > > >> 32 241619 262818
> > > >> 64 206868 233606
> > > >> 128 137084 217013
> >
> > So, there's a small regression on low client counts. That's worth
> > addressing.
> >
>
> Interesting. I'll try to reproduce it.

Any progress here?

Regards,

Andres

On Sat, Feb 27, 2016 at 5:14 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> > > > >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
> > > > >>
> > > > >> Client Base Patch
> > > > >> 1 17169 16454
> > > > >> 8 108547 105559
> > > > >> 32 241619 262818
> > > > >> 64 206868 233606
> > > > >> 128 137084 217013
> > >
> > > So, there's a small regression on low client counts. That's worth
> > > addressing.
> > >
> >
> > Interesting. I'll try to reproduce it.
>
> Any progress here?

In Multi socket machine with 8 sockets and 64 cores, I have seen more
regression compared to my previous run in power8 with 2 socket, currently I
tested Read only workload for 5 mins Run, When I get time, I will run for
longer time and confirm again.

Shared Buffer= 8GB
Scale Factor=300

./pgbench -j$ -c$ -T300 -M prepared -S postgres
client base patch
1 7057 5230
2 10043 9573
4 20140 18188

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sun, Feb 28, 2016 at 9:05 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
>
> On Sat, Feb 27, 2016 at 5:14 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>
>> > > > >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>> > > > >>
>> > > > >> Client Base Patch
>> > > > >> 1 17169 16454
>> > > > >> 8 108547 105559
>> > > > >> 32 241619 262818
>> > > > >> 64 206868 233606
>> > > > >> 128 137084 217013
>> > >
>> > > So, there's a small regression on low client counts. That's worth
>> > > addressing.
>> > >
>> >
>> > Interesting. I'll try to reproduce it.
>>
>> Any progress here?
>
>
> In Multi socket machine with 8 sockets and 64 cores, I have seen more
regression compared to my previous run in power8 with 2 socket, currently I
tested Read only workload for 5 mins Run, When I get time, I will run for
longer time and confirm again.
>

Have you tried by reverting the commits 6150a1b0 and ac1d794, which I think
effects read-only performance and sometimes create variation in TPS across
different runs, here second might have less impact, but first one could
impact performance? Is it possible for you to get perf data with and
without patch and share with others?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, Feb 29, 2016 at 8:26 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> Have you tried by reverting the commits 6150a1b0 and ac1d794, which I
> think effects read-only performance and sometimes create variation in TPS
> across different runs, here second might have less impact, but first one
> could impact performance? Is it possible for you to get perf data with and
> without patch and share with others?
>

I only reverted ac1d794 commit in my test, In my next run I will revert
6150a1b0 also and test.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sat, Feb 27, 2016 at 2:44 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-02-02 13:12:50 +0300, Alexander Korotkov wrote:
> > On Tue, Feb 2, 2016 at 12:43 AM, Andres Freund <andres(at)anarazel(dot)de>
> wrote:
> >
> > > On 2016-02-01 13:06:57 +0300, Alexander Korotkov wrote:
> > > > On Mon, Feb 1, 2016 at 11:34 AM, Alexander Korotkov <
> > > > a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> > > > >> Client Base Patch
> > > > >> 1 19744 19382
> > > > >> 8 125923 126395
> > > > >> 32 313931 333351
> > > > >> 64 387339 496830
> > > > >> 128 306412 350610
> > > > >>
> > > > >> Shared Buffer= 512MB
> > > > >> max_connections=150
> > > > >> Scale Factor=300
> > > > >>
> > > > >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
> > > > >>
> > > > >> Client Base Patch
> > > > >> 1 17169 16454
> > > > >> 8 108547 105559
> > > > >> 32 241619 262818
> > > > >> 64 206868 233606
> > > > >> 128 137084 217013
> > >
> > > So, there's a small regression on low client counts. That's worth
> > > addressing.
> > >
> >
> > Interesting. I'll try to reproduce it.
>
> Any progress here?
>

I didn't reproduce the regression. I had access to multicore machine but
didn't see either regression on low clients or improvements on high clients.
In the attached path spinlock delay was exposed in s_lock.h and used
in LockBufHdr().
Dilip, could you try this version of patch? Could you also run perf or
other profiler in the case of regression. It would be nice to compare
profiles with and without patch. We probably could find the cause of
regression.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Feb 29, 2016 at 5:18 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> I didn't reproduce the regression. I had access to multicore machine but
> didn't see either regression on low clients or improvements on high clients.
> In the attached path spinlock delay was exposed in s_lock.h and used
> in LockBufHdr().
> Dilip, could you try this version of patch? Could you also run perf or
> other profiler in the case of regression. It would be nice to compare
> profiles with and without patch. We probably could find the cause of
> regression.
>

OK, I will test it, sometime in this week.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Mar 1, 2016 at 10:19 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> OK, I will test it, sometime in this week.
>

I have tested this patch in my laptop, and there i did not see any
regression at 1 client

Shared buffer 10GB, 5 mins run with pgbench, read-only test

base patch
run1 22187 24334
run2 26288 27440
run3 26306 27411

May be in a day or 2 I will test it in the same machine where I reported
the regression, and if regression is there I will check the instruction
using Call grind.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On March 1, 2016 8:41:33 PM PST, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>On Tue, Mar 1, 2016 at 10:19 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
>wrote:
>
>>
>> OK, I will test it, sometime in this week.
>>
>
>I have tested this patch in my laptop, and there i did not see any
>regression at 1 client
>
>Shared buffer 10GB, 5 mins run with pgbench, read-only test
>
> base patch
>run1 22187 24334
>run2 26288 27440
>run3 26306 27411
>
>
>May be in a day or 2 I will test it in the same machine where I
>reported
>the regression, and if regression is there I will check the instruction
>using Call grind.

Sounds like a ppc vs. x86 issue. The regression was on the former, right?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Mar 2, 2016 at 10:39 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Sounds like a ppc vs. x86 issue. The regression was on the former, right?

Well, Regression what I reported last two time, out of that one was on X86
and other was on PPC.

Copied from older Threads
--------------------------------------
On PPC
>> > > > >> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>> > > > >>
>> > > > >> Client Base Patch
>> > > > >> 1 17169 16454
>> > > > >> 8 108547 105559
>> > > > >> 32 241619 262818
>> > > > >> 64 206868 233606
>> > > > >> 128 137084 217013

On X86
>> > > > >>Shared Buffer= 8GB
>> > > > >>Scale Factor=300

>> > > > >>./pgbench -j$ -c$ -T300 -M prepared -S postgres
>> > > > >>client base patch
>> > > > >>1 7057 5230
>> > > > >>2 10043 9573
>> > > > >>4 20140 18188

And this latest result (no regression) is on X86 but on my local machine.

I did not exactly saw what this new version of patch is doing different, so
I will test this version in other machines also and see the results.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 2, 2016 at 11:05 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> And this latest result (no regression) is on X86 but on my local machine.
>
> I did not exactly saw what this new version of patch is doing different,
> so I will test this version in other machines also and see the results.
>

I tested this on PPC again, This time in various order (sometime patch
first and then base first).
I tested with latest patch *pinunpin-cas-2.patch* on Power8.

Shared Buffer = 8GB
./pgbench -j$ -c$ -T300 -M prepared -S postgres

BASE
-----
Clients run1 run2 run3
1 21200 18754 20537
2 40331 39520 38746

Patch
-----
Clients run1 run2 run3
1 20225 19806 19778
2 39830 41898 36620

I think, here we can not see any regression, (If I take median then it may
looks low with patch so posting all 3 reading).

Note: reverted only ac1d794 commit in my test.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sat, Mar 5, 2016 at 7:22 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Wed, Mar 2, 2016 at 11:05 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>> And this latest result (no regression) is on X86 but on my local machine.
>>
>> I did not exactly saw what this new version of patch is doing different,
>> so I will test this version in other machines also and see the results.
>
>
> I tested this on PPC again, This time in various order (sometime patch first
> and then base first).
> I tested with latest patch pinunpin-cas-2.patch on Power8.
>
> Shared Buffer = 8GB
> ./pgbench -j$ -c$ -T300 -M prepared -S postgres
>
> BASE
> -----
> Clients run1 run2 run3
> 1 21200 18754 20537
> 2 40331 39520 38746
>
>
> Patch
> -----
> Clients run1 run2 run3
> 1 20225 19806 19778
> 2 39830 41898 36620
>
> I think, here we can not see any regression, (If I take median then it may
> looks low with patch so posting all 3 reading).

If the median looks low, how is that not a regression?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Mar 7, 2016 at 6:19 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sat, Mar 5, 2016 at 7:22 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > On Wed, Mar 2, 2016 at 11:05 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
> >> And this latest result (no regression) is on X86 but on my local
> machine.
> >>
> >> I did not exactly saw what this new version of patch is doing different,
> >> so I will test this version in other machines also and see the results.
> >
> >
> > I tested this on PPC again, This time in various order (sometime patch
> first
> > and then base first).
> > I tested with latest patch pinunpin-cas-2.patch on Power8.
> >
> > Shared Buffer = 8GB
> > ./pgbench -j$ -c$ -T300 -M prepared -S postgres
> >
> > BASE
> > -----
> > Clients run1 run2 run3
> > 1 21200 18754 20537
> > 2 40331 39520 38746
> >
> >
> > Patch
> > -----
> > Clients run1 run2 run3
> > 1 20225 19806 19778
> > 2 39830 41898 36620
> >
> > I think, here we can not see any regression, (If I take median then it
> may
> > looks low with patch so posting all 3 reading).
>
> If the median looks low, how is that not a regression?
>

I don't think we can rely on median that much if we have only 3 runs.
For 3 runs we can only apply Kornfeld method which claims that confidence
interval should be between lower and upper values.
Since confidence intervals for master and patched versions are overlapping
we can't conclude that expected TPS numbers are different.
Dilip, could you do more runs? 10, for example. Using such statistics we
would be able to conclude something.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Mar 10, 2016 at 8:26 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> I don't think we can rely on median that much if we have only 3 runs.
> For 3 runs we can only apply Kornfeld method which claims that confidence
> interval should be between lower and upper values.
> Since confidence intervals for master and patched versions are overlapping
> we can't conclude that expected TPS numbers are different.
> Dilip, could you do more runs? 10, for example. Using such statistics we
> would be able to conclude something.
>

Here is the reading for 10 runs....

Median Result
-----------------------

Client Base Patch
-------------------------------------------
1 19873 19739
2 38658 38276
4 68812 62075

Full Results of 10 runs...

Base
-------------
Runs 1 Client 2 Client 4 Client
-----------------------------------------------------
1 19442 34866 49023
2 19605 35139 51695
3 19726 37104 53899
4 19835 38439 55708
5 19866 38638 67473
6 19880 38679 70152
7 19973 38720 70920
8 20048 38737 71342
9 20057 38815 71403
10 20344 41423 77953
-----------------------------------------------------

Patch
-----------
Runs 1 Client 2 Client 4 Client
------------------------------------------------------
1 18881 30161 54928
2 19415 32031 59741
3 19564 35022 61060
4 19627 36712 61839
5 19670 37659 62011
6 19808 38894 62139
7 19857 39081 62983
8 19910 39923 75358
9 20169 39937 77481
10 20181 40003 78462
------------------------------------------------------

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Fri, Mar 11, 2016 at 7:08 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> On Thu, Mar 10, 2016 at 8:26 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> I don't think we can rely on median that much if we have only 3 runs.
>> For 3 runs we can only apply Kornfeld method which claims that confidence
>> interval should be between lower and upper values.
>> Since confidence intervals for master and patched versions are
>> overlapping we can't conclude that expected TPS numbers are different.
>> Dilip, could you do more runs? 10, for example. Using such statistics we
>> would be able to conclude something.
>>
>
> Here is the reading for 10 runs....
>
>
> Median Result
> -----------------------
>
> Client Base Patch
> -------------------------------------------
> 1 19873 19739
> 2 38658 38276
> 4 68812 62075
>
> Full Results of 10 runs...
>
> Base
> -------------
> Runs 1 Client 2 Client 4 Client
> -----------------------------------------------------
> 1 19442 34866 49023
> 2 19605 35139 51695
> 3 19726 37104 53899
> 4 19835 38439 55708
> 5 19866 38638 67473
> 6 19880 38679 70152
> 7 19973 38720 70920
> 8 20048 38737 71342
> 9 20057 38815 71403
> 10 20344 41423 77953
> -----------------------------------------------------
>
>
> Patch
> -----------
> Runs 1 Client 2 Client 4 Client
> ------------------------------------------------------
> 1 18881 30161 54928
> 2 19415 32031 59741
> 3 19564 35022 61060
> 4 19627 36712 61839
> 5 19670 37659 62011
> 6 19808 38894 62139
> 7 19857 39081 62983
> 8 19910 39923 75358
> 9 20169 39937 77481
> 10 20181 40003 78462
> ------------------------------------------------------
>

I've drawn graphs for these measurements. The variation doesn't look random
here. TPS is going higher from measurement to measurement. I bet you did
measurements sequentially.
I think we should do more measurements until TPS stop growing and beyond to
see accumulate average statistics. Could you please do the same tests but
50 runs.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Mar 14, 2016 at 3:09 AM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> I've drawn graphs for these measurements. The variation doesn't look
> random here. TPS is going higher from measurement to measurement. I bet
> you did measurements sequentially.
> I think we should do more measurements until TPS stop growing and beyond
> to see accumulate average statistics. Could you please do the same tests
> but 50 runs.
>

I have taken reading at different client count (1,2 and 4)

1. Avg: Is taken average after discarding lowest 5 and highest 5 readings.
2. With 4 client I have only 30 reading.

Summary:

I think if we check avg or median atleast at 1 or 2 client head always
looks winner, but same is not the case with 4 clients. And with 4 clients I
can see much more fluctuation in the reading.

Head(1 Client) Patch (1 Client)
Head (2 Client) Patch (2 Client)
Head (4 Client) Patch (4 Client)
Avg: 19628 19578
37180 36536
70044 70731
Median: 19663 19581
37967 37484
73003 75376
Below is all the reading. (Arranged in sorted order)

*Runs* *Head (1 Client)* *Patch (1 Client)*
*Head (2 Client)* *Patch (2 Client)*
*Head (4 Client)* *Patch (4 Client)*
1 18191 18153
29454 26128
49931 47210
2 18365 18768
31218 26956
53358 47287
3 19053 18769
31808 29286
53575 55458
4 19128 18915
31860 30558
54282 55794
5 19160 18948
32411 30945
56629 56253
6 19177 19055
32453 31316
57595 58147
7 19351 19232
32571 31703
59490 58543
8 19353 19239
33294 32029
63887 58990
9 19361 19255
33936 32217
64718 60233
10 19390 19297
34361 32767
65737 68210
11 19452 19368
34563 34487
65881 71409
12 19478 19387
36110 34907
67151 72108
13 19488 19388
36221 34936
70974 73977
14 19490 19395
36461 35068
72212 74891
15 19512 19406
36712 35298
73003 75376
16 19538 19450
37104 35492
74842 75468
17 19547 19487
37246 36648
75400 75515
18 19592 19521
37567 37263
75573 75626
19 19627 19536
37707 37430
75798 75745
20 19661 19556
37958 37461
75834 75868
21 19666 19607
37976 37507
76240 76161
22 19701 19624
38060 37557
76426 76162
23 19708 19643
38244 37717
76770 76333
24 19748 19684
38272 38285
77011 77009
25 19751 19694
38467 38294
77114 77168
26 19776 19695
38524 38469
77630 77318
27 19781 19709
38625 38642
77865 77550
28 19786 19765
38756 38643
77912 77904
29 19796 19823
38939 38649
78242 78736
30 19826 19847
39139 38659

31 19837 19899
39208 38713

32 19849 19909
39211 38837

33 19854 19932
39230 38876

34 19867 19949
39249 39088

35 19891 19990
39259 39148

36 20038 20085
39286 39453

37 20083 20128
39435 39563

38 20143 20166
39448 39959

39 20191 20198
39475 40495

40 20437 20455
40375 40664

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sat, Mar 19, 2016 at 3:22 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> On Mon, Mar 14, 2016 at 3:09 AM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> I've drawn graphs for these measurements. The variation doesn't look
>> random here. TPS is going higher from measurement to measurement. I bet
>> you did measurements sequentially.
>> I think we should do more measurements until TPS stop growing and beyond
>> to see accumulate average statistics. Could you please do the same tests
>> but 50 runs.
>>
>
> I have taken reading at different client count (1,2 and 4)
>
> 1. Avg: Is taken average after discarding lowest 5 and highest 5 readings.
> 2. With 4 client I have only 30 reading.
>
> Summary:
>
> I think if we check avg or median atleast at 1 or 2 client head always
> looks winner, but same is not the case with 4 clients. And with 4 clients I
> can see much more fluctuation in the reading.
>
>
> Head(1 Client) Patch (1 Client)
> Head (2 Client) Patch (2 Client)
> Head (4 Client) Patch (4 Client)
> Avg: 19628 19578
> 37180 36536
> 70044 70731
> Median: 19663 19581
> 37967 37484
> 73003 75376
> Below is all the reading. (Arranged in sorted order)
>
> *Runs* *Head (1 Client)* *Patch (1 Client)*
> *Head (2 Client)* *Patch (2 Client)*
> *Head (4 Client)* *Patch (4 Client)*
> 1 18191 18153
> 29454 26128
> 49931 47210
> 2 18365 18768
> 31218 26956
> 53358 47287
> 3 19053 18769
> 31808 29286
> 53575 55458
> 4 19128 18915
> 31860 30558
> 54282 55794
> 5 19160 18948
> 32411 30945
> 56629 56253
> 6 19177 19055
> 32453 31316
> 57595 58147
> 7 19351 19232
> 32571 31703
> 59490 58543
> 8 19353 19239
> 33294 32029
> 63887 58990
> 9 19361 19255
> 33936 32217
> 64718 60233
> 10 19390 19297
> 34361 32767
> 65737 68210
> 11 19452 19368
> 34563 34487
> 65881 71409
> 12 19478 19387
> 36110 34907
> 67151 72108
> 13 19488 19388
> 36221 34936
> 70974 73977
> 14 19490 19395
> 36461 35068
> 72212 74891
> 15 19512 19406
> 36712 35298
> 73003 75376
> 16 19538 19450
> 37104 35492
> 74842 75468
> 17 19547 19487
> 37246 36648
> 75400 75515
> 18 19592 19521
> 37567 37263
> 75573 75626
> 19 19627 19536
> 37707 37430
> 75798 75745
> 20 19661 19556
> 37958 37461
> 75834 75868
> 21 19666 19607
> 37976 37507
> 76240 76161
> 22 19701 19624
> 38060 37557
> 76426 76162
> 23 19708 19643
> 38244 37717
> 76770 76333
> 24 19748 19684
> 38272 38285
> 77011 77009
> 25 19751 19694
> 38467 38294
> 77114 77168
> 26 19776 19695
> 38524 38469
> 77630 77318
> 27 19781 19709
> 38625 38642
> 77865 77550
> 28 19786 19765
> 38756 38643
> 77912 77904
> 29 19796 19823
> 38939 38649
> 78242 78736
> 30 19826 19847
> 39139 38659
>
>
> 31 19837 19899
> 39208 38713
>
>
> 32 19849 19909
> 39211 38837
>
>
> 33 19854 19932
> 39230 38876
>
>
> 34 19867 19949
> 39249 39088
>
>
> 35 19891 19990
> 39259 39148
>
>
> 36 20038 20085
> 39286 39453
>
>
> 37 20083 20128
> 39435 39563
>
>
> 38 20143 20166
> 39448 39959
>
>
> 39 20191 20198
> 39475 40495
>
>
> 40 20437 20455
> 40375 40664
>
>
>
So, I think it's really some regression here on small number clients. I
have following hypothesis about that. In some cases we use more atomic
operations than before. For instace, in BufferAlloc we have following block
of code. Previous code deals with that without atomic operations relying
on spinlock. So, we have 2 extra atomic operations here, and similar
situation in other places.

pg_atomic_fetch_and_u32(&buf->state, ~(BM_VALID | BM_DIRTY |
BM_JUST_DIRTIED | BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
BUF_USAGECOUNT_MASK)); if (relpersistence == RELPERSISTENCE_PERMANENT)
pg_atomic_fetch_or_u32(&buf->state, BM_TAG_VALID | BM_PERMANENT |
BUF_USAGECOUNT_ONE); else pg_atomic_fetch_or_u32(&buf->state, BM_TAG_VALID
| BUF_USAGECOUNT_ONE);
Actually, we behave like old code and do such modifications without
increasing number of atomic operations. We can just calculate new value of
state (including unset of BM_LOCKED flag) and write it to the buffer. In
this case we don't require more atomic operations. However, it becomes not
safe to use atomic decrement in UnpinBuffer(). We can use there loop of
CAS which wait buffer header to be unlocked like PinBuffer() does. I'm not
sure if this affects multicore scalability, but I think it worth trying.

So, this idea is implemented in attached patch. Please, try it for both
regression on lower number of clients and scalability on large number of
clients.

Other changes in this patch:
1) PinBuffer() and UnpinBuffer() use exponential backoff in spindealy
like LockBufHdr() does.
2) Previous patch contained revert
of ac1d7945f866b1928c2554c0f80fd52d7f977772. This was not intentional.
No, it doesn't contains this revert.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sun, Mar 20, 2016 at 4:10 AM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> Actually, we behave like old code and do such modifications without
> increasing number of atomic operations. We can just calculate new value of
> state (including unset of BM_LOCKED flag) and write it to the buffer. In
> this case we don't require more atomic operations. However, it becomes not
> safe to use atomic decrement in UnpinBuffer(). We can use there loop of
> CAS which wait buffer header to be unlocked like PinBuffer() does. I'm not
> sure if this affects multicore scalability, but I think it worth trying.
>
> So, this idea is implemented in attached patch. Please, try it for both
> regression on lower number of clients and scalability on large number of
> clients.
>
>
Good, seems like we have reduced some instructions, I will test it soon.

> Other changes in this patch:
> 1) PinBuffer() and UnpinBuffer() use exponential backoff in spindealy
> like LockBufHdr() does.
> 2) Previous patch contained revert
> of ac1d7945f866b1928c2554c0f80fd52d7f977772. This was not intentional.
> No, it doesn't contains this revert.
>

Some other comments..

*************** ReadBuffer_common(SMgrRelation smgr, cha
*** 828,837 ****
*/
do
{
! LockBufHdr(bufHdr);
! Assert(bufHdr->flags & BM_VALID);
! bufHdr->flags &= ~BM_VALID;
! UnlockBufHdr(bufHdr);
} while (!StartBufferIO(bufHdr, true));
}
}
--- 826,834 ----
*/
do
{
! uint32 state = LockBufHdr(bufHdr);
! state &= ~(BM_VALID | BM_LOCKED);
! pg_atomic_write_u32(&bufHdr->state, state);
} while (!StartBufferIO(bufHdr, true));

Better Write some comment, about we clearing the BM_LOCKED from stage
directly and need not to call UnlockBufHdr explicitly.
otherwise its confusing.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Mar 22, 2016 at 12:31 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> ! pg_atomic_write_u32(&bufHdr->state, state);
> } while (!StartBufferIO(bufHdr, true));
>
> Better Write some comment, about we clearing the BM_LOCKED from stage
> directly and need not to call UnlockBufHdr explicitly.
> otherwise its confusing.
>

Few more comments..

*** 828,837 ****
*/
do
{
! LockBufHdr(bufHdr);
*! Assert(bufHdr->flags & BM_VALID);*
! bufHdr->flags &= ~BM_VALID;
! UnlockBufHdr(bufHdr);
} while (!StartBufferIO(bufHdr, true));
}
}
--- 826,834 ----
*/
do
{
! uint32 state = LockBufHdr(bufHdr);
! state &= ~(BM_VALID | BM_LOCKED);
! pg_atomic_write_u32(&bufHdr->state, state);
} while (!StartBufferIO(bufHdr, true));

1. Previously there was a Assert, any reason why we removed it ?
Assert(bufHdr->flags & BM_VALID);

2. And also if we don't need assert then can't we directly clear BM_VALID
flag from state variable (if not locked) like pin/unpin buffer does,
without taking buffer header lock?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Mar 22, 2016 at 7:57 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

>
> On Tue, Mar 22, 2016 at 12:31 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
>
>> ! pg_atomic_write_u32(&bufHdr->state, state);
>> } while (!StartBufferIO(bufHdr, true));
>>
>> Better Write some comment, about we clearing the BM_LOCKED from stage
>> directly and need not to call UnlockBufHdr explicitly.
>> otherwise its confusing.
>>
>
> Few more comments..
>
> *** 828,837 ****
> */
> do
> {
> ! LockBufHdr(bufHdr);
> *! Assert(bufHdr->flags & BM_VALID);*
> ! bufHdr->flags &= ~BM_VALID;
> ! UnlockBufHdr(bufHdr);
> } while (!StartBufferIO(bufHdr, true));
> }
> }
> --- 826,834 ----
> */
> do
> {
> ! uint32 state = LockBufHdr(bufHdr);
> ! state &= ~(BM_VALID | BM_LOCKED);
> ! pg_atomic_write_u32(&bufHdr->state, state);
> } while (!StartBufferIO(bufHdr, true));
>
> 1. Previously there was a Assert, any reason why we removed it ?
> Assert(bufHdr->flags & BM_VALID);
>

It was missed. In the attached patch I've put it back.

2. And also if we don't need assert then can't we directly clear BM_VALID
> flag from state variable (if not locked) like pin/unpin buffer does,
> without taking buffer header lock?
>

In this version of patch it could be also done as loop of CAS operation.
But I'm not intended to it so because it would significantly complicate
code. It's not yes clear that traffic in this place is high enough to make
such optimizations.
Since v4 patch implements slightly different approach. Could you please
test it? We need to check that this approach worth putting more efforts on
it. Or through it away otherwise.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Mar 22, 2016 at 1:08 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Tue, Mar 22, 2016 at 7:57 AM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> wrote:
>
>>
>> On Tue, Mar 22, 2016 at 12:31 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
>> wrote:
>>
>>> ! pg_atomic_write_u32(&bufHdr->state, state);
>>> } while (!StartBufferIO(bufHdr, true));
>>>
>>> Better Write some comment, about we clearing the BM_LOCKED from stage
>>> directly and need not to call UnlockBufHdr explicitly.
>>> otherwise its confusing.
>>>
>>
>> Few more comments..
>>
>> *** 828,837 ****
>> */
>> do
>> {
>> ! LockBufHdr(bufHdr);
>> *! Assert(bufHdr->flags & BM_VALID);*
>> ! bufHdr->flags &= ~BM_VALID;
>> ! UnlockBufHdr(bufHdr);
>> } while (!StartBufferIO(bufHdr, true));
>> }
>> }
>> --- 826,834 ----
>> */
>> do
>> {
>> ! uint32 state = LockBufHdr(bufHdr);
>> ! state &= ~(BM_VALID | BM_LOCKED);
>> ! pg_atomic_write_u32(&bufHdr->state, state);
>> } while (!StartBufferIO(bufHdr, true));
>>
>> 1. Previously there was a Assert, any reason why we removed it ?
>> Assert(bufHdr->flags & BM_VALID);
>>
>
> It was missed. In the attached patch I've put it back.
>
> 2. And also if we don't need assert then can't we directly clear BM_VALID
>> flag from state variable (if not locked) like pin/unpin buffer does,
>> without taking buffer header lock?
>>
>
> In this version of patch it could be also done as loop of CAS operation.
> But I'm not intended to it so because it would significantly complicate
> code. It's not yes clear that traffic in this place is high enough to make
> such optimizations.
> Since v4 patch implements slightly different approach. Could you please
> test it? We need to check that this approach worth putting more efforts on
> it. Or through it away otherwise.
>

Could anybody run benchmarks? Feature freeze is soon, but it would be
*very nice* to fit it into 9.6 release cycle, because it greatly improves
scalability on large machines. Without this patch PostgreSQL 9.6 will be
significantly behind competitors like MySQL 5.7.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Mar 25, 2016 at 8:09 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> Could anybody run benchmarks? Feature freeze is soon, but it would be
> *very nice* to fit it into 9.6 release cycle, because it greatly improves
> scalability on large machines. Without this patch PostgreSQL 9.6 will be
> significantly behind competitors like MySQL 5.7.

I have run the performance and here are the results.. With latest patch I
did not see any regression at lower client count (median of 3 reading).

scale factor 1000 shared buffer 8GB readonly
*Client Base patch*
1 12957 13068
2 24931 25816
4 46311 48767
32 300921 310062
64 387623 493843
128 249635 583513
scale factor 300 shared buffer 8GB readonly
*Client Base patch*
1 14537 14586 --> one thread number looks little less, generally I get
~18000 (will recheck).
2 34703 33929 --> may be run to run variance (once I get time, will
recheck)
4 67744 69069
32 312575 336012
64 213312 539056
128 190139 380122

*Summary:*

Actually with 64 client we have seen ~470,000 TPS with head also, by
revering commit 6150a1b0.
refer this thread: (
http://www.postgresql.org/message-id/CAA4eK1+ZeB8PMwwktf+3bRS0Pt4Ux6Rs6Aom0uip8c6shJWmyg@mail.gmail.com
)

I haven't tested this patch by reverting commit 6150a1b0, so not sure can
this patch give even better performance ?

It also points to the case, what Andres has mentioned in this thread.

http://www.postgresql.org/message-id/20160226191158.3vidtk3ktcmhimdu@alap3.anarazel.de

Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi, Dilip!

On Fri, Mar 25, 2016 at 8:32 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:

> On Fri, Mar 25, 2016 at 8:09 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> Could anybody run benchmarks? Feature freeze is soon, but it would be
>> *very nice* to fit it into 9.6 release cycle, because it greatly improves
>> scalability on large machines. Without this patch PostgreSQL 9.6 will be
>> significantly behind competitors like MySQL 5.7.
>
>
> I have run the performance and here are the results.. With latest patch I
> did not see any regression at lower client count (median of 3 reading).
>
> scale factor 1000 shared buffer 8GB readonly
> *Client Base patch*
> 1 12957 13068
> 2 24931 25816
> 4 46311 48767
> 32 300921 310062
> 64 387623 493843
> 128 249635 583513
> scale factor 300 shared buffer 8GB readonly
> *Client Base patch*
> 1 14537 14586 --> one thread number looks little less, generally I get
> ~18000 (will recheck).
> 2 34703 33929 --> may be run to run variance (once I get time, will
> recheck)
> 4 67744 69069
> 32 312575 336012
> 64 213312 539056
> 128 190139 380122
>
> *Summary:*
>
> Actually with 64 client we have seen ~470,000 TPS with head also, by
> revering commit 6150a1b0.
> refer this thread: (
> http://www.postgresql.org/message-id/CAA4eK1+ZeB8PMwwktf+3bRS0Pt4Ux6Rs6Aom0uip8c6shJWmyg@mail.gmail.com
> )
>
> I haven't tested this patch by reverting commit 6150a1b0, so not sure can
> this patch give even better performance ?
>
> It also points to the case, what Andres has mentioned in this thread.
>
>
> http://www.postgresql.org/message-id/20160226191158.3vidtk3ktcmhimdu@alap3.anarazel.de
>

Thank you very much for testing!
I also got access to 4 x 18 Intel server with 144 threads. I'm going to
post results of tests on this server in next Monday.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sat, Mar 26, 2016 at 1:26 AM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> Thank you very much for testing!
> I also got access to 4 x 18 Intel server with 144 threads. I'm going to
> post results of tests on this server in next Monday.
>

I've run pgbench tests on this machine: pgbench -s 1000 -c $clients -j 100
-M prepared -T 300.
See results in the table and chart.

clients master v3 v5
1 11671 12507 12679
2 24650 26005 25010
4 49631 48863 49811
8 96790 96441 99946
10 121275 119928 124100
20 243066 243365 246432
30 359616 342241 357310
40 431375 415310 441619
50 489991 489896 500590
60 538057 636473 554069
70 588659 714426 738535
80 405008 923039 902632
90 295443 1181247 1155918
100 258695 1323125 1325019
110 238842 1393767 1410274
120 226018 1432504 1474982
130 215102 1465459 1503241
140 206415 1470454 1505380
150 197850 1475479 1519908
160 190935 1420915 1484868
170 185835 1438965 1453128
180 182519 1416252 1453098

My conclusions are following:
1) We don't observe any regression in v5 in comparison to master.
2) v5 in most of cases slightly outperforms v3.

I'm going to do some code cleanup of v5 in Monday

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2016-03-25 23:02:11 +0530, Dilip Kumar wrote:
> On Fri, Mar 25, 2016 at 8:09 PM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
> > Could anybody run benchmarks? Feature freeze is soon, but it would be
> > *very nice* to fit it into 9.6 release cycle, because it greatly improves
> > scalability on large machines. Without this patch PostgreSQL 9.6 will be
> > significantly behind competitors like MySQL 5.7.
>
>
> I have run the performance and here are the results.. With latest patch I
> did not see any regression at lower client count (median of 3 reading).
>
> scale factor 1000 shared buffer 8GB readonly
> *Client Base patch*
> 1 12957 13068
> 2 24931 25816
> 4 46311 48767
> 32 300921 310062
> 64 387623 493843
> 128 249635 583513
> scale factor 300 shared buffer 8GB readonly
> *Client Base patch*
> 1 14537 14586 --> one thread number looks little less, generally I get
> ~18000 (will recheck).
> 2 34703 33929 --> may be run to run variance (once I get time, will
> recheck)
> 4 67744 69069
> 32 312575 336012
> 64 213312 539056
> 128 190139 380122
>
> *Summary:*
>
> Actually with 64 client we have seen ~470,000 TPS with head also, by
> revering commit 6150a1b0.
> refer this thread: (
> http://www.postgresql.org/message-id/CAA4eK1+ZeB8PMwwktf+3bRS0Pt4Ux6Rs6Aom0uip8c6shJWmyg@mail.gmail.com
> )
>
> I haven't tested this patch by reverting commit 6150a1b0, so not sure can
> this patch give even better performance ?
>
> It also points to the case, what Andres has mentioned in this thread.
>
> http://www.postgresql.org/message-id/20160226191158.3vidtk3ktcmhimdu@alap3.anarazel.de

On what hardware did you run these tests?

Thanks,

Andres

On 2016-03-27 12:38:25 +0300, Alexander Korotkov wrote:
> On Sat, Mar 26, 2016 at 1:26 AM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
> > Thank you very much for testing!
> > I also got access to 4 x 18 Intel server with 144 threads. I'm going to
> > post results of tests on this server in next Monday.
> >
>
> I've run pgbench tests on this machine: pgbench -s 1000 -c $clients -j 100
> -M prepared -T 300.
> See results in the table and chart.
>
> clients master v3 v5
> 1 11671 12507 12679
> 2 24650 26005 25010
> 4 49631 48863 49811
> 8 96790 96441 99946
> 10 121275 119928 124100
> 20 243066 243365 246432
> 30 359616 342241 357310
> 40 431375 415310 441619
> 50 489991 489896 500590
> 60 538057 636473 554069
> 70 588659 714426 738535
> 80 405008 923039 902632
> 90 295443 1181247 1155918
> 100 258695 1323125 1325019
> 110 238842 1393767 1410274
> 120 226018 1432504 1474982
> 130 215102 1465459 1503241
> 140 206415 1470454 1505380
> 150 197850 1475479 1519908
> 160 190935 1420915 1484868
> 170 185835 1438965 1453128
> 180 182519 1416252 1453098
>
> My conclusions are following:
> 1) We don't observe any regression in v5 in comparison to master.
> 2) v5 in most of cases slightly outperforms v3.

What commit did you base these tests on? I guess something recent, after
98a64d0bd?

> I'm going to do some code cleanup of v5 in Monday

Ok, I'll try to do a review and possibly commit after that.

Greetings,

Andres Freund

On Sun, Mar 27, 2016 at 5:37 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On what hardware did you run these tests?

IBM POWER 8 MACHINE.

Architecture: ppc64le
Byte Order: Little Endian
CPU(s): 192
Thread(s) per core: 8
Core(s) per socket: 1
Socket(s): 24
NUMA node(s): 4

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On 2016-03-27 17:45:52 +0530, Dilip Kumar wrote:
> On Sun, Mar 27, 2016 at 5:37 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > On what hardware did you run these tests?
>
>
> IBM POWER 8 MACHINE.
>
> Architecture: ppc64le
> Byte Order: Little Endian
> CPU(s): 192
> Thread(s) per core: 8
> Core(s) per socket: 1
> Socket(s): 24
> NUMA node(s): 4

What's sizeof(BufferDesc) after applying these patches? It should better
be <= 64...

Andres

On Sun, Mar 27, 2016 at 3:10 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-03-27 12:38:25 +0300, Alexander Korotkov wrote:
> > On Sat, Mar 26, 2016 at 1:26 AM, Alexander Korotkov <
> > a(dot)korotkov(at)postgrespro(dot)ru> wrote:
> >
> > > Thank you very much for testing!
> > > I also got access to 4 x 18 Intel server with 144 threads. I'm going to
> > > post results of tests on this server in next Monday.
> > >
> >
> > I've run pgbench tests on this machine: pgbench -s 1000 -c $clients -j
> 100
> > -M prepared -T 300.
> > See results in the table and chart.
> >
> > clients master v3 v5
> > 1 11671 12507 12679
> > 2 24650 26005 25010
> > 4 49631 48863 49811
> > 8 96790 96441 99946
> > 10 121275 119928 124100
> > 20 243066 243365 246432
> > 30 359616 342241 357310
> > 40 431375 415310 441619
> > 50 489991 489896 500590
> > 60 538057 636473 554069
> > 70 588659 714426 738535
> > 80 405008 923039 902632
> > 90 295443 1181247 1155918
> > 100 258695 1323125 1325019
> > 110 238842 1393767 1410274
> > 120 226018 1432504 1474982
> > 130 215102 1465459 1503241
> > 140 206415 1470454 1505380
> > 150 197850 1475479 1519908
> > 160 190935 1420915 1484868
> > 170 185835 1438965 1453128
> > 180 182519 1416252 1453098
> >
> > My conclusions are following:
> > 1) We don't observe any regression in v5 in comparison to master.
> > 2) v5 in most of cases slightly outperforms v3.
>
> What commit did you base these tests on? I guess something recent, after
> 98a64d0bd?
>

Yes, more recent than 98a64d0bd. It was based on 676265eb7b.

> > I'm going to do some code cleanup of v5 in Monday
>
> Ok, I'll try to do a review and possibly commit after that.
>

Sounds good.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sun, Mar 27, 2016 at 5:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>
> What's sizeof(BufferDesc) after applying these patches? It should better
> be <= 64...
>

It is 72.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sun, Mar 27, 2016 at 4:31 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Sun, Mar 27, 2016 at 3:10 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> On 2016-03-27 12:38:25 +0300, Alexander Korotkov wrote:
>> > On Sat, Mar 26, 2016 at 1:26 AM, Alexander Korotkov <
>> > a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>> >
>> > > Thank you very much for testing!
>> > > I also got access to 4 x 18 Intel server with 144 threads. I'm going
>> to
>> > > post results of tests on this server in next Monday.
>> > >
>> >
>> > I've run pgbench tests on this machine: pgbench -s 1000 -c $clients -j
>> 100
>> > -M prepared -T 300.
>> > See results in the table and chart.
>> >
>> > clients master v3 v5
>> > 1 11671 12507 12679
>> > 2 24650 26005 25010
>> > 4 49631 48863 49811
>> > 8 96790 96441 99946
>> > 10 121275 119928 124100
>> > 20 243066 243365 246432
>> > 30 359616 342241 357310
>> > 40 431375 415310 441619
>> > 50 489991 489896 500590
>> > 60 538057 636473 554069
>> > 70 588659 714426 738535
>> > 80 405008 923039 902632
>> > 90 295443 1181247 1155918
>> > 100 258695 1323125 1325019
>> > 110 238842 1393767 1410274
>> > 120 226018 1432504 1474982
>> > 130 215102 1465459 1503241
>> > 140 206415 1470454 1505380
>> > 150 197850 1475479 1519908
>> > 160 190935 1420915 1484868
>> > 170 185835 1438965 1453128
>> > 180 182519 1416252 1453098
>> >
>> > My conclusions are following:
>> > 1) We don't observe any regression in v5 in comparison to master.
>> > 2) v5 in most of cases slightly outperforms v3.
>>
>> What commit did you base these tests on? I guess something recent, after
>> 98a64d0bd?
>>
>
> Yes, more recent than 98a64d0bd. It was based on 676265eb7b.
>
>
>> > I'm going to do some code cleanup of v5 in Monday
>>
>> Ok, I'll try to do a review and possibly commit after that.
>>
>
> Sounds good.
>

There is next revision of patch. It contains mostly cosmetic changes.
Comment are adjusted to reflect changes of behaviour.
I also changed atomic AND/OR for local buffers to read/write pair which
would be cheaper I suppose. However, I don't insist on it, and it could be
reverted.
The patch is ready for your review. It's especially interesting what do
you think about the way I abstracted exponential back off of spinlock.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2016-03-28 11:48:46 +0530, Dilip Kumar wrote:
> On Sun, Mar 27, 2016 at 5:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> >
> > What's sizeof(BufferDesc) after applying these patches? It should better
> > be <= 64...
> >
>
> It is 72.

Ah yes, miscalculated the required alignment. Hm. So we got to get this
smaller. I see three approaches:

1) Reduce the spinlock size on ppc. That actually might just work by
replacing "unsigned int" by "unsigned char"
2) Replace the lwlock spinlock by a bit in LWLock->state. That'd avoid
embedding the spinlock, and actually might allow to avoid one atomic
op in a number of cases.
3) Shrink the size of BufferDesc by removing buf_id; that'd bring it to
64byte.

I'm a bit hesitant to go for 3), because it'd likely end up adding a bit
of arithmetic to a number of places in bufmgr.c. Robert, what do you
think?

Andres

On 2016-03-28 15:46:43 +0300, Alexander Korotkov wrote:
> diff --git a/src/backend/storage/buffer/bufmnew file mode 100644
> index 6dd7c6e..fe6fb9c
> --- a/src/backend/storage/buffer/bufmgr.c
> +++ b/src/backend/storage/buffer/bufmgr.c
> @@ -52,7 +52,6 @@
> #include "utils/resowner_private.h"
> #include "utils/timestamp.h"
>
> -
> /* Note: these two macros only work on shared buffers, not local ones! */
> #define BufHdrGetBlock(bufHdr) ((Block) (BufferBlocks + ((Size) (bufHdr)->buf_id) * BLCKSZ))
> #define BufferGetLSN(bufHdr) (PageGetLSN(BufHdrGetBlock(bufHdr)))

spurious

> @@ -848,7 +852,7 @@ ReadBuffer_common(SMgrRelation smgr, cha
> * it's not been recycled) but come right back here to try smgrextend
> * again.
> */
> - Assert(!(bufHdr->flags & BM_VALID)); /* spinlock not needed */
> + Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID)); /* header lock not needed */
>
> bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
>
> @@ -932,8 +936,13 @@ ReadBuffer_common(SMgrRelation smgr, cha
>
> if (isLocalBuf)
> {
> - /* Only need to adjust flags */
> - bufHdr->flags |= BM_VALID;
> + /*
> + * Only need to adjust flags. Since it's local buffer, there is no
> + * concurrency. We assume read/write pair to be cheaper than atomic OR.
> + */
> + uint32 state = pg_atomic_read_u32(&bufHdr->state);
> + state |= BM_VALID;
> + pg_atomic_write_u32(&bufHdr->state, state);

I'm not a fan of repeating that comment in multiple places. Hm.

> }
> else
> {
> @@ -987,10 +996,11 @@ BufferAlloc(SMgrRelation smgr, char relp
> BufferTag oldTag; /* previous identity of selected buffer */
> uint32 oldHash; /* hash value for oldTag */
> LWLock *oldPartitionLock; /* buffer partition lock for it */
> - BufFlags oldFlags;
> + uint32 oldFlags;
> int buf_id;
> BufferDesc *buf;
> bool valid;
> + uint32 state;
>
> /* create a tag so we can lookup the buffer */
> INIT_BUFFERTAG(newTag, smgr->smgr_rnode.node, forkNum, blockNum);
> @@ -1050,23 +1060,23 @@ BufferAlloc(SMgrRelation smgr, char relp
> for (;;)
> {
> /*
> - * Ensure, while the spinlock's not yet held, that there's a free
> + * Ensure, while the header lock isn't yet held, that there's a free
> * refcount entry.
> */
> ReservePrivateRefCountEntry();
>
> /*
> * Select a victim buffer. The buffer is returned with its header
> - * spinlock still held!
> + * lock still held!
> */
> - buf = StrategyGetBuffer(strategy);
> + buf = StrategyGetBuffer(strategy, &state);

The new thing really still is a spinlock, not sure if it's worth
changing the comments that way.

> @@ -1319,7 +1330,7 @@ BufferAlloc(SMgrRelation smgr, char relp
> * InvalidateBuffer -- mark a shared buffer invalid and return it to the
> * freelist.
> *
> - * The buffer header spinlock must be held at entry. We drop it before
> + * The buffer header lock must be held at entry. We drop it before
> * returning. (This is sane because the caller must have locked the
> * buffer in order to be sure it should be dropped.)
> *

Ok, by now I'm pretty sure that I don't want this to be changed
everywhere, just makes reviewing harder.

> @@ -1433,6 +1443,7 @@ void
> MarkBufferDirty(Buffer buffer)
> {
> BufferDesc *bufHdr;
> + uint32 state;
>
> if (!BufferIsValid(buffer))
> elog(ERROR, "bad buffer ID: %d", buffer);
> @@ -1449,14 +1460,14 @@ MarkBufferDirty(Buffer buffer)
> /* unfortunately we can't check if the lock is held exclusively */
> Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
>
> - LockBufHdr(bufHdr);
> + state = LockBufHdr(bufHdr);
>
> - Assert(bufHdr->refcount > 0);
> + Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
>
> /*
> * If the buffer was not dirty already, do vacuum accounting.
> */
> - if (!(bufHdr->flags & BM_DIRTY))
> + if (!(state & BM_DIRTY))
> {
> VacuumPageDirty++;
> pgBufferUsage.shared_blks_dirtied++;
> @@ -1464,9 +1475,9 @@ MarkBufferDirty(Buffer buffer)
> VacuumCostBalance += VacuumCostPageDirty;
> }
>
> - bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);
> -
> - UnlockBufHdr(bufHdr);
> + state |= BM_DIRTY | BM_JUST_DIRTIED;
> + state &= ~BM_LOCKED;
> + pg_atomic_write_u32(&bufHdr->state, state);
> }

Hm, this is a routine I think we should avoid taking the lock in; it's
often called quite frequently. Also doesn't look very hard.

> static bool
> PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
> @@ -1547,23 +1563,44 @@ PinBuffer(BufferDesc *buf, BufferAccessS
>
> if (ref == NULL)
> {
> + /* loop of CAS operations */
> + uint32 state;
> + uint32 oldstate;
> + SpinDelayStatus delayStatus;

> ReservePrivateRefCountEntry();
> ref = NewPrivateRefCountEntry(b);
>
> - LockBufHdr(buf);
> - buf->refcount++;
> - if (strategy == NULL)
> - {
> - if (buf->usage_count < BM_MAX_USAGE_COUNT)
> - buf->usage_count++;
> - }
> - else
> + state = pg_atomic_read_u32(&buf->state);
> + oldstate = state;
> +
> + init_spin_delay(&delayStatus, (Pointer)buf, __FILE__, __LINE__);

Hm. This way we're calling this on every iteration. That doesn't seem
like a good idea. How about making delayStatus a static, and
init_spin_delay a macro which returns a {struct, member, one, two} type
literal?

> + while (true)
> {
> - if (buf->usage_count == 0)
> - buf->usage_count = 1;
> + /* spin-wait till lock is free */
> + while (state & BM_LOCKED)
> + {
> + make_spin_delay(&delayStatus);
> + state = pg_atomic_read_u32(&buf->state);
> + oldstate = state;
> + }

Maybe we should abstract that to pg_atomic_wait_bit_unset_u32()? It
seems quite likely we need this in other places (e.g. lwlock.c itself).

> + /* increase refcount */
> + state += BUF_REFCOUNT_ONE;
> +
> + /* increase usagecount unless already max */
> + if (BUF_STATE_GET_USAGECOUNT(state) != BM_MAX_USAGE_COUNT)
> + state += BUF_USAGECOUNT_ONE;
> +
> + /* try to do CAS, exit on success */

Seems like a somewhat obvious comment?

> @@ -1603,15 +1640,22 @@ PinBuffer_Locked(BufferDesc *buf)
> {

> Assert(GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf), false) == NULL);
>
> - buf->refcount++;
> - UnlockBufHdr(buf);
> + /*
> + * Since we assume to held buffer header lock, we can update the buffer
> + * state in a single write operation.
> + */
> + state = pg_atomic_read_u32(&buf->state);
> + state += 1;
> + state &= ~BM_LOCKED;
> + pg_atomic_write_u32(&buf->state, state);

Te comment should probably mention that we're releasing the
spinlock. And the += 1 should be a BUF_REFCOUNT_ONE, otherwise it's hard
to understand.

> @@ -1646,30 +1690,66 @@ UnpinBuffer(BufferDesc *buf, bool fixOwn
> ref->refcount--;
> if (ref->refcount == 0)
> {

> +
> + /* Support LockBufferForCleanup() */
> + if (state & BM_PIN_COUNT_WAITER)
> + {
> + state = LockBufHdr(buf);
> +
> + if (state & BM_PIN_COUNT_WAITER && BUF_STATE_GET_REFCOUNT(state) == 1)
> + {
> + /* we just released the last pin other than the waiter's */
> + int wait_backend_pid = buf->wait_backend_pid;
>
> + state &= ~(BM_PIN_COUNT_WAITER | BM_LOCKED);
> + pg_atomic_write_u32(&buf->state, state);
> + ProcSendSignal(wait_backend_pid);
> + }
> + else
> + UnlockBufHdr(buf);
> + }

I think it's quite confusing to use UnlockBufHdr and direct bit
expressions in one branch.

Thinking about it I also don't think the pg_atomic_write_u32 variant is
correct without adding a write barrier; the other side might not see the
values yet.

I think we can just redefine UnlockBufHdr() to be a
pg_atomic_write_u32() and pg_write_memory_barrier()?

> * BM_PERMANENT can't be changed while we hold a pin on the buffer, so we
> - * need not bother with the buffer header spinlock. Even if someone else
> + * need not bother with the buffer header lock. Even if someone else
> * changes the buffer header flags while we're doing this, we assume that
> * changing an aligned 2-byte BufFlags value is atomic, so we'll read the
> * old value or the new value, but not random garbage.
> */

The rest of the comment is outdated, BufFlags isn't a 2 byte value
anymore.

> @@ -3078,7 +3168,7 @@ FlushRelationBuffers(Relation rel)
> localpage,
> false);
>
> - bufHdr->flags &= ~(BM_DIRTY | BM_JUST_DIRTIED);
> + pg_atomic_fetch_and_u32(&bufHdr->state, ~(BM_DIRTY | BM_JUST_DIRTIED));

Hm, in other places you replaced atomics on local buffers with plain
writes.

> lsn = XLogSaveBufferForHint(buffer, buffer_std);
> }
>
> - LockBufHdr(bufHdr);
> - Assert(bufHdr->refcount > 0);
> - if (!(bufHdr->flags & BM_DIRTY))
> + state = LockBufHdr(bufHdr);
> +
> + Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
> +
> + if (!(state & BM_DIRTY))
> {
> dirtied = true; /* Means "will be dirtied by this action" */

It's again worthwhile to try make this work without taking the lock.

> - buf->flags |= BM_IO_IN_PROGRESS;
> -
> - UnlockBufHdr(buf);
> + state |= BM_IO_IN_PROGRESS;
> + state &= ~BM_LOCKED;
> + pg_atomic_write_u32(&buf->state, state);

How about making UnlockBufHdr() take a new state parameter, and
internally unset BM_LOCKED?

> /*
> + * Lock buffer header - set BM_LOCKED in buffer state.
> + */
> +uint32
> +LockBufHdr(volatile BufferDesc *desc)
> +{
> + SpinDelayStatus delayStatus;
> + uint32 state;
> +
> + init_spin_delay(&delayStatus, (Pointer)desc, __FILE__, __LINE__);
> +
> + state = pg_atomic_read_u32(&desc->state);
> +
> + for (;;)
> + {
> + /* wait till lock is free */
> + while (state & BM_LOCKED)
> + {
> + make_spin_delay(&delayStatus);
> + state = pg_atomic_read_u32(&desc->state);
> + /* Add exponential backoff? Should seldomly be contended tho. */

Outdated comment.

> +/*
> + * Unlock buffer header - unset BM_LOCKED in buffer state.
> + */
> +void
> +UnlockBufHdr(volatile BufferDesc *desc)
> +{
> + Assert(pg_atomic_read_u32(&desc->state) & BM_LOCKED);
> +
> + pg_atomic_sub_fetch_u32(&desc->state, BM_LOCKED);
> +}

As suggested above, there's likely no need to use an actual atomic op
here.

Greetings,

Andres Freund

Andres Freund wrote:
> On 2016-03-28 15:46:43 +0300, Alexander Korotkov wrote:

> > @@ -932,8 +936,13 @@ ReadBuffer_common(SMgrRelation smgr, cha
> >
> > if (isLocalBuf)
> > {
> > - /* Only need to adjust flags */
> > - bufHdr->flags |= BM_VALID;
> > + /*
> > + * Only need to adjust flags. Since it's local buffer, there is no
> > + * concurrency. We assume read/write pair to be cheaper than atomic OR.
> > + */
> > + uint32 state = pg_atomic_read_u32(&bufHdr->state);
> > + state |= BM_VALID;
> > + pg_atomic_write_u32(&bufHdr->state, state);
>
> I'm not a fan of repeating that comment in multiple places. Hm.

Perhaps a set of macros should be offered that do "read, set some flag,
write". Then the comments can be attached to the macro instead of to
each callsite.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 28, 2016 at 9:09 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-03-28 11:48:46 +0530, Dilip Kumar wrote:
>> On Sun, Mar 27, 2016 at 5:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > What's sizeof(BufferDesc) after applying these patches? It should better
>> > be <= 64...
>> >
>>
>> It is 72.
>
> Ah yes, miscalculated the required alignment. Hm. So we got to get this
> smaller. I see three approaches:
>
> 1) Reduce the spinlock size on ppc. That actually might just work by
> replacing "unsigned int" by "unsigned char"
> 2) Replace the lwlock spinlock by a bit in LWLock->state. That'd avoid
> embedding the spinlock, and actually might allow to avoid one atomic
> op in a number of cases.
> 3) Shrink the size of BufferDesc by removing buf_id; that'd bring it to
> 64byte.
>
> I'm a bit hesitant to go for 3), because it'd likely end up adding a bit
> of arithmetic to a number of places in bufmgr.c. Robert, what do you
> think?

I don't have a clear idea what's going to be better here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-03-29 13:09:05 -0400, Robert Haas wrote:
> On Mon, Mar 28, 2016 at 9:09 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-03-28 11:48:46 +0530, Dilip Kumar wrote:
> >> On Sun, Mar 27, 2016 at 5:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> > What's sizeof(BufferDesc) after applying these patches? It should better
> >> > be <= 64...
> >> >
> >>
> >> It is 72.
> >
> > Ah yes, miscalculated the required alignment. Hm. So we got to get this
> > smaller. I see three approaches:
> >
> > 1) Reduce the spinlock size on ppc. That actually might just work by
> > replacing "unsigned int" by "unsigned char"
> > 2) Replace the lwlock spinlock by a bit in LWLock->state. That'd avoid
> > embedding the spinlock, and actually might allow to avoid one atomic
> > op in a number of cases.
> > 3) Shrink the size of BufferDesc by removing buf_id; that'd bring it to
> > 64byte.
> >
> > I'm a bit hesitant to go for 3), because it'd likely end up adding a bit
> > of arithmetic to a number of places in bufmgr.c. Robert, what do you
> > think?
>
> I don't have a clear idea what's going to be better here.

My gut feeling is that we should do both 1) and 2).

Dilip, could you test performance of reducing ppc's spinlock to 1 byte?
Cross-compiling suggest that doing so "just works". I.e. replace the
#if defined(__ppc__) typedef from an int to a char.

Regards,

Andres

Hi, Andres!

Please, find next revision of patch in attachment.

On Mon, Mar 28, 2016 at 4:59 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-03-28 15:46:43 +0300, Alexander Korotkov wrote:
> > diff --git a/src/backend/storage/buffer/bufmnew file mode 100644
> > index 6dd7c6e..fe6fb9c
> > --- a/src/backend/storage/buffer/bufmgr.c
> > +++ b/src/backend/storage/buffer/bufmgr.c
> > @@ -52,7 +52,6 @@
> > #include "utils/resowner_private.h"
> > #include "utils/timestamp.h"
> >
> > -
> > /* Note: these two macros only work on shared buffers, not local ones!
> */
> > #define BufHdrGetBlock(bufHdr) ((Block) (BufferBlocks + ((Size)
> (bufHdr)->buf_id) * BLCKSZ))
> > #define BufferGetLSN(bufHdr) (PageGetLSN(BufHdrGetBlock(bufHdr)))
>
> spurious
>

Fixed.

> > @@ -848,7 +852,7 @@ ReadBuffer_common(SMgrRelation smgr, cha
> > * it's not been recycled) but come right back here to try
> smgrextend
> > * again.
> > */
> > - Assert(!(bufHdr->flags & BM_VALID)); /* spinlock not
> needed */
> > + Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID)); /*
> header lock not needed */
> >
> > bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) :
> BufHdrGetBlock(bufHdr);
> >
> > @@ -932,8 +936,13 @@ ReadBuffer_common(SMgrRelation smgr, cha
> >
> > if (isLocalBuf)
> > {
> > - /* Only need to adjust flags */
> > - bufHdr->flags |= BM_VALID;
> > + /*
> > + * Only need to adjust flags. Since it's local buffer,
> there is no
> > + * concurrency. We assume read/write pair to be cheaper
> than atomic OR.
> > + */
> > + uint32 state = pg_atomic_read_u32(&bufHdr->state);
> > + state |= BM_VALID;
> > + pg_atomic_write_u32(&bufHdr->state, state);
>
> I'm not a fan of repeating that comment in multiple places. Hm.
>

Moved comments into single place where macros for CAS loop is defined (see
my comments below).

> > }
> > else
> > {
> > @@ -987,10 +996,11 @@ BufferAlloc(SMgrRelation smgr, char relp
> > BufferTag oldTag; /* previous identity of
> selected buffer */
> > uint32 oldHash; /* hash value for oldTag */
> > LWLock *oldPartitionLock; /* buffer partition lock
> for it */
> > - BufFlags oldFlags;
> > + uint32 oldFlags;
> > int buf_id;
> > BufferDesc *buf;
> > bool valid;
> > + uint32 state;
> >
> > /* create a tag so we can lookup the buffer */
> > INIT_BUFFERTAG(newTag, smgr->smgr_rnode.node, forkNum, blockNum);
> > @@ -1050,23 +1060,23 @@ BufferAlloc(SMgrRelation smgr, char relp
> > for (;;)
> > {
> > /*
> > - * Ensure, while the spinlock's not yet held, that there's
> a free
> > + * Ensure, while the header lock isn't yet held, that
> there's a free
> > * refcount entry.
> > */
> > ReservePrivateRefCountEntry();
> >
> > /*
> > * Select a victim buffer. The buffer is returned with
> its header
> > - * spinlock still held!
> > + * lock still held!
> > */
> > - buf = StrategyGetBuffer(strategy);
> > + buf = StrategyGetBuffer(strategy, &state);
>
> The new thing really still is a spinlock, not sure if it's worth
> changing the comments that way.
>
>
> > @@ -1319,7 +1330,7 @@ BufferAlloc(SMgrRelation smgr, char relp
> > * InvalidateBuffer -- mark a shared buffer invalid and return it to the
> > * freelist.
> > *
> > - * The buffer header spinlock must be held at entry. We drop it before
> > + * The buffer header lock must be held at entry. We drop it before
> > * returning. (This is sane because the caller must have locked the
> > * buffer in order to be sure it should be dropped.)
> > *
>
> Ok, by now I'm pretty sure that I don't want this to be changed
> everywhere, just makes reviewing harder.
>

Fixed.

> > @@ -1433,6 +1443,7 @@ void
> > MarkBufferDirty(Buffer buffer)
> > {
> > BufferDesc *bufHdr;
> > + uint32 state;
> >
> > if (!BufferIsValid(buffer))
> > elog(ERROR, "bad buffer ID: %d", buffer);
> > @@ -1449,14 +1460,14 @@ MarkBufferDirty(Buffer buffer)
> > /* unfortunately we can't check if the lock is held exclusively */
> > Assert(LWLockHeldByMe(BufferDescriptorGetContentLock(bufHdr)));
> >
> > - LockBufHdr(bufHdr);
> > + state = LockBufHdr(bufHdr);
> >
> > - Assert(bufHdr->refcount > 0);
> > + Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
> >
> > /*
> > * If the buffer was not dirty already, do vacuum accounting.
> > */
> > - if (!(bufHdr->flags & BM_DIRTY))
> > + if (!(state & BM_DIRTY))
> > {
> > VacuumPageDirty++;
> > pgBufferUsage.shared_blks_dirtied++;
> > @@ -1464,9 +1475,9 @@ MarkBufferDirty(Buffer buffer)
> > VacuumCostBalance += VacuumCostPageDirty;
> > }
> >
> > - bufHdr->flags |= (BM_DIRTY | BM_JUST_DIRTIED);
> > -
> > - UnlockBufHdr(bufHdr);
> > + state |= BM_DIRTY | BM_JUST_DIRTIED;
> > + state &= ~BM_LOCKED;
> > + pg_atomic_write_u32(&bufHdr->state, state);
> > }
>
> Hm, this is a routine I think we should avoid taking the lock in; it's
> often called quite frequently. Also doesn't look very hard.
>

Done.

> > static bool
> > PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
> > @@ -1547,23 +1563,44 @@ PinBuffer(BufferDesc *buf, BufferAccessS
> >
> > if (ref == NULL)
> > {
> > + /* loop of CAS operations */
> > + uint32 state;
> > + uint32 oldstate;
> > + SpinDelayStatus delayStatus;
>
>
>
> > ReservePrivateRefCountEntry();
> > ref = NewPrivateRefCountEntry(b);
> >
> > - LockBufHdr(buf);
> > - buf->refcount++;
> > - if (strategy == NULL)
> > - {
> > - if (buf->usage_count < BM_MAX_USAGE_COUNT)
> > - buf->usage_count++;
> > - }
> > - else
> > + state = pg_atomic_read_u32(&buf->state);
> > + oldstate = state;
> > +
> > + init_spin_delay(&delayStatus, (Pointer)buf, __FILE__,
> __LINE__);
>
> Hm. This way we're calling this on every iteration. That doesn't seem
> like a good idea. How about making delayStatus a static, and
> init_spin_delay a macro which returns a {struct, member, one, two} type
> literal?
>

Done.

> > + while (true)
> > {
> > - if (buf->usage_count == 0)
> > - buf->usage_count = 1;
> > + /* spin-wait till lock is free */
> > + while (state & BM_LOCKED)
> > + {
> > + make_spin_delay(&delayStatus);
> > + state = pg_atomic_read_u32(&buf->state);
> > + oldstate = state;
> > + }
>
> Maybe we should abstract that to pg_atomic_wait_bit_unset_u32()? It
> seems quite likely we need this in other places (e.g. lwlock.c itself).
>

I have some doubts about such function. At first, I can't find a place for
it in lwlock.c. It doesn't run explicit spin delays yet. Is it related to
some changes you're planning for lwlock.c.
At second, I doubt about it's signature. It would be logical if it would
be "uint32 pg_atomic_wait_bit_unset_u32(pg_atomic_u32 *var, uint32 mask)".
But in this case we have to do extra pg_atomic_read_u32 after unsuccessful
CAS. And it causes some small regression. Especially unpleasant that it's
also regression in comparison with master on low clients.

But there is code duplication which would be very nice to evade. I end up
with macros which encapsulates CAS loop.

BEGIN_BUFSTATE_CAS_LOOP(buf);
state |= BM_LOCKED;
END_BUFSTATE_CAS_LOOP(buf);

For me, it simplifies readability a lot.

> > + /* increase refcount */
> > + state += BUF_REFCOUNT_ONE;
> > +
> > + /* increase usagecount unless already max */
> > + if (BUF_STATE_GET_USAGECOUNT(state) !=
> BM_MAX_USAGE_COUNT)
> > + state += BUF_USAGECOUNT_ONE;
> > +
> > + /* try to do CAS, exit on success */
>
> Seems like a somewhat obvious comment?
>

Comment removed.

> > @@ -1603,15 +1640,22 @@ PinBuffer_Locked(BufferDesc *buf)
> > {
>
> > Assert(GetPrivateRefCountEntry(BufferDescriptorGetBuffer(buf),
> false) == NULL);
> >
> > - buf->refcount++;
> > - UnlockBufHdr(buf);
> > + /*
> > + * Since we assume to held buffer header lock, we can update the
> buffer
> > + * state in a single write operation.
> > + */
> > + state = pg_atomic_read_u32(&buf->state);
> > + state += 1;
> > + state &= ~BM_LOCKED;
> > + pg_atomic_write_u32(&buf->state, state);
>
> Te comment should probably mention that we're releasing the
> spinlock. And the += 1 should be a BUF_REFCOUNT_ONE, otherwise it's hard
> to understand.
>

Fixed.

> > @@ -1646,30 +1690,66 @@ UnpinBuffer(BufferDesc *buf, bool fixOwn
> > ref->refcount--;
> > if (ref->refcount == 0)
> > {
>
> > +
> > + /* Support LockBufferForCleanup() */
> > + if (state & BM_PIN_COUNT_WAITER)
> > + {
> > + state = LockBufHdr(buf);
> > +
> > + if (state & BM_PIN_COUNT_WAITER &&
> BUF_STATE_GET_REFCOUNT(state) == 1)
> > + {
> > + /* we just released the last pin other
> than the waiter's */
> > + int wait_backend_pid =
> buf->wait_backend_pid;
> >
> > + state &= ~(BM_PIN_COUNT_WAITER |
> BM_LOCKED);
> > + pg_atomic_write_u32(&buf->state, state);
> > + ProcSendSignal(wait_backend_pid);
> > + }
> > + else
> > + UnlockBufHdr(buf);
> > + }
>
> I think it's quite confusing to use UnlockBufHdr and direct bit
> expressions in one branch.
>
> Thinking about it I also don't think the pg_atomic_write_u32 variant is
> correct without adding a write barrier; the other side might not see the
> values yet.
>
> I think we can just redefine UnlockBufHdr() to be a
> pg_atomic_write_u32() and pg_write_memory_barrier()?
>

Done.

> > * BM_PERMANENT can't be changed while we hold a pin on the
> buffer, so we
> > - * need not bother with the buffer header spinlock. Even if
> someone else
> > + * need not bother with the buffer header lock. Even if someone
> else
> > * changes the buffer header flags while we're doing this, we
> assume that
> > * changing an aligned 2-byte BufFlags value is atomic, so we'll
> read the
> > * old value or the new value, but not random garbage.
> > */
>
> The rest of the comment is outdated, BufFlags isn't a 2 byte value
> anymore.
>

Fixed.

> > @@ -3078,7 +3168,7 @@ FlushRelationBuffers(Relation rel)
> > localpage,
> > false);
> >
> > - bufHdr->flags &= ~(BM_DIRTY |
> BM_JUST_DIRTIED);
> > + pg_atomic_fetch_and_u32(&bufHdr->state,
> ~(BM_DIRTY | BM_JUST_DIRTIED));
>
> Hm, in other places you replaced atomics on local buffers with plain
> writes.
>

Fixed.

> > lsn = XLogSaveBufferForHint(buffer, buffer_std);
> > }
> >
> > - LockBufHdr(bufHdr);
> > - Assert(bufHdr->refcount > 0);
> > - if (!(bufHdr->flags & BM_DIRTY))
> > + state = LockBufHdr(bufHdr);
> > +
> > + Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
> > +
> > + if (!(state & BM_DIRTY))
> > {
> > dirtied = true; /* Means "will be dirtied
> by this action" */
>
> It's again worthwhile to try make this work without taking the lock.
>

Comment there claims.

/*
* Set the page LSN if we wrote a backup block. We aren't supposed
* to set this when only holding a share lock but as long as we
* serialise it somehow we're OK. We choose to set LSN while
* holding the buffer header lock, which causes any reader of an
* LSN who holds only a share lock to also obtain a buffer header
* lock before using PageGetLSN(), which is enforced in
* BufferGetLSNAtomic().

Thus, buffer header lock is used for write serialization. I don't think it
would be correct to remove the lock here.

> > - buf->flags |= BM_IO_IN_PROGRESS;
> > -
> > - UnlockBufHdr(buf);
> > + state |= BM_IO_IN_PROGRESS;
> > + state &= ~BM_LOCKED;
> > + pg_atomic_write_u32(&buf->state, state);
>
> How about making UnlockBufHdr() take a new state parameter, and
> internally unset BM_LOCKED?
>

Done.

> > /*
> > + * Lock buffer header - set BM_LOCKED in buffer state.
> > + */
> > +uint32
> > +LockBufHdr(volatile BufferDesc *desc)
> > +{
> > + SpinDelayStatus delayStatus;
> > + uint32 state;
> > +
> > + init_spin_delay(&delayStatus, (Pointer)desc, __FILE__, __LINE__);
> > +
> > + state = pg_atomic_read_u32(&desc->state);
> > +
> > + for (;;)
> > + {
> > + /* wait till lock is free */
> > + while (state & BM_LOCKED)
> > + {
> > + make_spin_delay(&delayStatus);
> > + state = pg_atomic_read_u32(&desc->state);
> > + /* Add exponential backoff? Should seldomly be
> contended tho. */
>
> Outdated comment.
>

Fixed.

> > +/*
> > + * Unlock buffer header - unset BM_LOCKED in buffer state.
> > + */
> > +void
> > +UnlockBufHdr(volatile BufferDesc *desc)
> > +{
> > + Assert(pg_atomic_read_u32(&desc->state) & BM_LOCKED);
> > +
> > + pg_atomic_sub_fetch_u32(&desc->state, BM_LOCKED);
> > +}
>
> As suggested above, there's likely no need to use an actual atomic op
> here.
>

Fixed. Actually, this function is removed now.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Andres Freund <andres(at)anarazel(dot)de> writes:
> Dilip, could you test performance of reducing ppc's spinlock to 1 byte?
> Cross-compiling suggest that doing so "just works". I.e. replace the
> #if defined(__ppc__) typedef from an int to a char.

AFAICS, lwarx/stwcx are specifically *word* wide.

regards, tom lane

On 2016-03-29 13:24:40 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Dilip, could you test performance of reducing ppc's spinlock to 1 byte?
> > Cross-compiling suggest that doing so "just works". I.e. replace the
> > #if defined(__ppc__) typedef from an int to a char.
>
> AFAICS, lwarx/stwcx are specifically *word* wide.

Ah, x86 gcc is just too convenient, with it automatically adjusting
instruction types :(

There's actually lbarx/stbcx - but it's not present in all ISAs. So I
guess it's clear where to go.

Andres Freund

On 2016-03-29 20:22:00 +0300, Alexander Korotkov wrote:
> > > + while (true)
> > > {
> > > - if (buf->usage_count == 0)
> > > - buf->usage_count = 1;
> > > + /* spin-wait till lock is free */
> > > + while (state & BM_LOCKED)
> > > + {
> > > + make_spin_delay(&delayStatus);
> > > + state = pg_atomic_read_u32(&buf->state);
> > > + oldstate = state;
> > > + }
> >
> > Maybe we should abstract that to pg_atomic_wait_bit_unset_u32()? It
> > seems quite likely we need this in other places (e.g. lwlock.c itself).
> >
>
> I have some doubts about such function. At first, I can't find a place for
> it in lwlock.c. It doesn't run explicit spin delays yet. Is it related to
> some changes you're planning for lwlock.c.

Yes, see http://www.postgresql.org/message-id/20160328130904.4mhugvkf4f3wg4qb@awork2.anarazel.de

> > > lsn = XLogSaveBufferForHint(buffer, buffer_std);
> > > }
> > >
> > > - LockBufHdr(bufHdr);
> > > - Assert(bufHdr->refcount > 0);
> > > - if (!(bufHdr->flags & BM_DIRTY))
> > > + state = LockBufHdr(bufHdr);
> > > +
> > > + Assert(BUF_STATE_GET_REFCOUNT(state) > 0);
> > > +
> > > + if (!(state & BM_DIRTY))
> > > {
> > > dirtied = true; /* Means "will be dirtied
> > by this action" */
> >
> > It's again worthwhile to try make this work without taking the lock.
> >
>
> Comment there claims.
>
> /*
> * Set the page LSN if we wrote a backup block. We aren't supposed
> * to set this when only holding a share lock but as long as we
> * serialise it somehow we're OK. We choose to set LSN while
> * holding the buffer header lock, which causes any reader of an
> * LSN who holds only a share lock to also obtain a buffer header
> * lock before using PageGetLSN(), which is enforced in
> * BufferGetLSNAtomic().
>
> Thus, buffer header lock is used for write serialization. I don't think it
> would be correct to remove the lock here.

Gah, I forgot about this uglyness. Man, the checksumming commit sure
made a number of things really ugly.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-03-29 13:24:40 -0400, Tom Lane wrote:
>> AFAICS, lwarx/stwcx are specifically *word* wide.

> There's actually lbarx/stbcx - but it's not present in all ISAs. So I
> guess it's clear where to go.

Hm. We could certainly add a configure test to see if the local assembler
knows these instructions --- but it's not clear that we should depend on
compile-time environment to match run-time.

Googling suggests that these instructions came in with PPC ISA 2.06
which seems to date to 2010. So there's undoubtedly still production
hardware without them.

In the department of not-production hardware, I checked this on prairiedog
and got
/var/tmp/ccbQy9uG.s:1722:Invalid mnemonic 'lbarx'
/var/tmp/ccbQy9uG.s:1726:Invalid mnemonic 'stbcx.'

regards, tom lane

On 2016-03-29 14:09:42 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > There's actually lbarx/stbcx - but it's not present in all ISAs. So I
> > guess it's clear where to go.
>
> Hm. We could certainly add a configure test to see if the local assembler
> knows these instructions --- but it's not clear that we should depend on
> compile-time environment to match run-time.

I think it'd be easier to continue using lwarx/stwcx, but be careful
about only testing/setting the lowest byte, if we want to go there. But
that then likely would require hints about alignment to the compiler...

i've no experience writing ppc assembly, but it doesn't look too hard.

but i think it's easier to just remove the spinlock from struct lwlock
then - and it also improves the situation for other architectures with
wider spinlocks. i think that's beneficial for some future things
anyway.

> Googling suggests that these instructions came in with PPC ISA 2.06
> which seems to date to 2010. So there's undoubtedly still production
> hardware without them.
>
> In the department of not-production hardware, I checked this on prairiedog
> and got
> /var/tmp/ccbQy9uG.s:1722:Invalid mnemonic 'lbarx'
> /var/tmp/ccbQy9uG.s:1726:Invalid mnemonic 'stbcx.'

Heh. Thanks for testing.

Andres