Thread: Speed up Clog Access by increasing CLOG buffers

On 2015-09-01 10:19:19 +0530, Amit Kapila wrote:
> pgbench setup
> ------------------------
> scale factor - 300
> Data is on magnetic disk and WAL on ssd.
> pgbench -M prepared tpc-b
> 
> HEAD - commit 0e141c0f
> Patch-1 - increase_clog_bufs_v1
> 
> Client Count/Patch_ver 1 8 16 32 64 128 256 HEAD 911 5695 9886 18028 27851
> 28654 25714 Patch-1 954 5568 9898 18450 29313 31108 28213
> 
> 
> This data shows that there is an increase of ~5% at 64 client-count
> and 8~10% at more higher clients without degradation at lower client-
> count. In above data, there is some fluctuation seen at 8-client-count,
> but I attribute that to run-to-run variation, however if anybody has doubts
> I can again re-verify the data at lower client counts.

> Now if we try to further increase the number of CLOG buffers to 128,
> no improvement is seen.
> 
> I have also verified that this improvement can be seen only after the
> contention around ProcArrayLock is reduced.  Below is the data with
> Commit before the ProcArrayLock reduction patch.  Setup and test
> is same as mentioned for previous test.

The buffer replacement algorithm for clog is rather stupid - I do wonder
where the cutoff is that it hurts.

Could you perhaps try to create a testcase where xids are accessed that
are so far apart on average that they're unlikely to be in memory? And
then test that across a number of client counts?

There's two reasons that I'd like to see that: First I'd like to avoid
regression, second I'd like to avoid having to bump the maximum number
of buffers by small buffers after every hardware generation...

>  /*
>   * Number of shared CLOG buffers.
>   *
> - * Testing during the PostgreSQL 9.2 development cycle revealed that on a
> + * Testing during the PostgreSQL 9.6 development cycle revealed that on a
>   * large multi-processor system, it was possible to have more CLOG page
> - * requests in flight at one time than the number of CLOG buffers which existed
> - * at that time, which was hardcoded to 8.  Further testing revealed that
> - * performance dropped off with more than 32 CLOG buffers, possibly because
> - * the linear buffer search algorithm doesn't scale well.
> + * requests in flight at one time than the number of CLOG buffers which
> + * existed at that time, which was 32 assuming there are enough shared_buffers.
> + * Further testing revealed that either performance stayed same or dropped off
> + * with more than 64 CLOG buffers, possibly because the linear buffer search
> + * algorithm doesn't scale well or some other locking bottlenecks in the
> + * system mask the improvement.
>   *
> - * Unconditionally increasing the number of CLOG buffers to 32 did not seem
> + * Unconditionally increasing the number of CLOG buffers to 64 did not seem
>   * like a good idea, because it would increase the minimum amount of shared
>   * memory required to start, which could be a problem for people running very
>   * small configurations.  The following formula seems to represent a reasonable
>   * compromise: people with very low values for shared_buffers will get fewer
> - * CLOG buffers as well, and everyone else will get 32.
> + * CLOG buffers as well, and everyone else will get 64.
>   *
>   * It is likely that some further work will be needed here in future releases;
>   * for example, on a 64-core server, the maximum number of CLOG requests that
>   * can be simultaneously in flight will be even larger.  But that will
>   * apparently require more than just changing the formula, so for now we take
> - * the easy way out.
> + * the easy way out.  It could also happen that after removing other locking
> + * bottlenecks, further increase in CLOG buffers can help, but that's not the
> + * case now.
>   */

I think the comment should be more drastically rephrased to not
reference individual versions and numbers.

Greetings,

Andres Freund

Andres Freund wrote:

> The buffer replacement algorithm for clog is rather stupid - I do wonder
> where the cutoff is that it hurts.
> 
> Could you perhaps try to create a testcase where xids are accessed that
> are so far apart on average that they're unlikely to be in memory? And
> then test that across a number of client counts?
> 
> There's two reasons that I'd like to see that: First I'd like to avoid
> regression, second I'd like to avoid having to bump the maximum number
> of buffers by small buffers after every hardware generation...

I wonder if it would make sense to explore an idea that has been floated
for years now -- to have pg_clog pages be allocated as part of shared
buffers rather than have their own separate pool.  That way, no separate
hardcoded allocation limit is needed.  It's probably pretty tricky to
implement, though :-(

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

Hi,

On 2015-09-07 10:34:10 -0300, Alvaro Herrera wrote:
> I wonder if it would make sense to explore an idea that has been floated
> for years now -- to have pg_clog pages be allocated as part of shared
> buffers rather than have their own separate pool.  That way, no separate
> hardcoded allocation limit is needed.  It's probably pretty tricky to
> implement, though :-(

I still think that'd be a good plan, especially as it'd also let us use
a lot of related infrastructure. I doubt we could just use the standard
cache replacement mechanism though - it's not particularly efficient
either... I also have my doubts that a hash table lookup at every clog
lookup is going to be ok performancewise.

The biggest problem will probably be that the buffer manager is pretty
directly tied to relations and breaking up that bond won't be all that
easy. My guess is that the best bet here is that the easiest way to at
least explore this is to define pg_clog/... as their own tablespaces
(akin to pg_global) and treat the files therein as plain relations.

Greetings,

Andres Freund

Andres Freund wrote:

> On 2015-09-07 10:34:10 -0300, Alvaro Herrera wrote:
> > I wonder if it would make sense to explore an idea that has been floated
> > for years now -- to have pg_clog pages be allocated as part of shared
> > buffers rather than have their own separate pool.  That way, no separate
> > hardcoded allocation limit is needed.  It's probably pretty tricky to
> > implement, though :-(
> 
> I still think that'd be a good plan, especially as it'd also let us use
> a lot of related infrastructure. I doubt we could just use the standard
> cache replacement mechanism though - it's not particularly efficient
> either... I also have my doubts that a hash table lookup at every clog
> lookup is going to be ok performancewise.

Yeah.  I guess we'd have to mark buffers as unusable for regular pages
("somehow"), and have a separate lookup mechanism.  As I said, it is
likely to be tricky.

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

On Mon, Sep 7, 2015 at 9:34 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Andres Freund wrote:
>> The buffer replacement algorithm for clog is rather stupid - I do wonder
>> where the cutoff is that it hurts.
>>
>> Could you perhaps try to create a testcase where xids are accessed that
>> are so far apart on average that they're unlikely to be in memory? And
>> then test that across a number of client counts?
>>
>> There's two reasons that I'd like to see that: First I'd like to avoid
>> regression, second I'd like to avoid having to bump the maximum number
>> of buffers by small buffers after every hardware generation...
>
> I wonder if it would make sense to explore an idea that has been floated
> for years now -- to have pg_clog pages be allocated as part of shared
> buffers rather than have their own separate pool.  That way, no separate
> hardcoded allocation limit is needed.  It's probably pretty tricky to
> implement, though :-(

Yeah, I looked at that once and threw my hands up in despair pretty
quickly.  I also considered another idea that looked simpler: instead
of giving every SLRU its own pool of pages, have one pool of pages for
all of them, separate from shared buffers but common to all SLRUs.
That looked easier, but still not easy.

I've also considered trying to replace the entire SLRU system with new
code and throwing away what exists today.  The locking mode is just
really strange compared to what we do elsewhere.  That, too, does not
look all that easy.  :-(

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

On Fri, Sep 11, 2015 at 10:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Could you perhaps try to create a testcase where xids are accessed that
> > are so far apart on average that they're unlikely to be in memory? And
> > then test that across a number of client counts?
> >
>
> Now about the test, create a table with large number of rows (say 11617457,
> I have tried to create larger, but it was taking too much time (more than a day))
> and have each row with different transaction id.  Now each transaction should
> update rows that are at least 1048576 (number of transactions whose status can
> be held in 32 CLog buffers) distance apart, that way ideally for each update it will
> try to access Clog page that is not in-memory, however as the value to update
> is getting selected randomly and that leads to every 100th access as disk access.

What about just running a regular pgbench test, but hacking the
XID-assignment code so that we increment the XID counter by 100 each
time instead of 1?

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

On Fri, Sep 11, 2015 at 11:01 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> If I am not wrong we need 1048576 number of transactions difference
> for each record to make each CLOG access a disk access, so if we
> increment XID counter by 100, then probably every 10000th (or multiplier
> of 10000) transaction would go for disk access.
>
> The number 1048576 is derived by below calc:
> #define CLOG_XACTS_PER_BYTE 4
> #define CLOG_XACTS_PER_PAGE (BLCKSZ * CLOG_XACTS_PER_BYTE)
>
> Transaction difference required for each transaction to go for disk access:
> CLOG_XACTS_PER_PAGE * num_clog_buffers.
>
> I think reducing to every 100th access for transaction status as disk access
> is sufficient to prove that there is no regression with the patch for the
> screnario
> asked by Andres or do you think it is not?

I have no idea.  I was just suggesting that hacking the server somehow
might be an easier way of creating the scenario Andres was interested
in than the process you described.  But feel free to ignore me, I
haven't taken much time to think about this.

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

On 09/11/2015 10:31 AM, Amit Kapila wrote:
> Updated comments and the patch (increate_clog_bufs_v2.patch)
> containing the same is attached.
>

I have done various runs on an Intel Xeon 28C/56T w/ 256Gb mem and 2 x 
RAID10 SSD (data + xlog) with Min(64,).

Kept the shared_buffers=64GB and effective_cache_size=160GB settings 
across all runs, but did runs with both synchronous_commit on and off 
and different scale factors for pgbench.

The results are in flux for all client numbers within -2 to +2% 
depending on the latency average.

So no real conclusion from here other than the patch doesn't help/hurt 
performance on this setup, likely depends on further CLogControlLock 
related changes to see real benefit.

Best regards, Jesper

On Mon, Aug 31, 2015 at 9:49 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Increasing CLOG buffers to 64 helps in reducing the contention due to second
> reason.  Experiments revealed that increasing CLOG buffers only helps
> once the contention around ProcArrayLock is reduced.

There has been a lot of research on bitmap compression, more or less
for the benefit of bitmap index access methods.

Simple techniques like run length encoding are effective for some
things. If the need to map the bitmap into memory to access the status
of transactions is a concern, there has been work done on that, too.
Byte-aligned bitmap compression is a technique that might offer a good
trade-off between compression clog, and decompression overhead -- I
think that there basically is no decompression overhead, because set
operations can be performed on the "compressed" representation
directly. There are other techniques, too.

Something to consider. There could be multiple benefits to compressing
clog, even beyond simply avoiding managing clog buffers.

-- 
Peter Geoghegan

On 09/18/2015 11:11 PM, Amit Kapila wrote:
>> I have done various runs on an Intel Xeon 28C/56T w/ 256Gb mem and 2 x
>> RAID10 SSD (data + xlog) with Min(64,).
>>
>>
> The benefit with this patch could be seen at somewhat higher
> client-count as you can see in my initial mail, can you please
> once try with client count > 64?
>

Client count were from 1 to 80.

I did do one run with Min(128,) like you, but didn't see any difference 
in the result compared to Min(64,), so focused instead in the 
sync_commit on/off testing case.

Best regards, Jesper

On Thu, Nov 26, 2015 at 11:32 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Nov 17, 2015 at 6:30 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>
>> On 17 November 2015 at 11:48, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>
>>>
>>> I think in that case what we can do is if the total number of
>>> sub transactions is lesser than equal to 64 (we can find that by
>>> overflowed flag in PGXact) , then apply this optimisation, else use
>>> the existing flow to update the transaction status.  I think for that we
>>> don't even need to reserve any additional memory. Does that sound
>>> sensible to you?
>>
>>
>> I understand you to mean that the leader should look backwards through the
>> queue collecting xids while !(PGXACT->overflowed)
>>
>> No additional shmem is required
>>
>
> Okay, as discussed I have handled the case of sub-transactions without
> additional shmem in the attached patch.  Apart from that, I have tried
> to apply this optimization for Prepared transactions as well, but as
> the dummy proc used for such transactions doesn't have semaphore like
> backend proc's, so it is not possible to use such a proc in group status
> updation as each group member needs to wait on semaphore.  It is not tad
> difficult to add the support for that case if we are okay with creating
> additional
> semaphore for each such dummy proc which I was not sure, so I have left
> it for now.

Is this proposal instead of, or in addition to, the original thread
topic of increasing clog buffers to 64?

Thanks,

Jeff

On Fri, Nov 27, 2015 at 2:32 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Okay, as discussed I have handled the case of sub-transactions without
> additional shmem in the attached patch.  Apart from that, I have tried
> to apply this optimization for Prepared transactions as well, but as
> the dummy proc used for such transactions doesn't have semaphore like
> backend proc's, so it is not possible to use such a proc in group status
> updation as each group member needs to wait on semaphore.  It is not tad
> difficult to add the support for that case if we are okay with creating
> additional
> semaphore for each such dummy proc which I was not sure, so I have left
> it for now.

"updation" is not a word.  "acquirations" is not a word.  "penality"
is spelled wrong.

I think the approach this patch takes is pretty darned strange, and
almost certainly not what we want.  What you're doing here is putting
every outstanding CLOG-update request into a linked list, and then the
leader goes and does all of those CLOG updates.  But there's no
guarantee that the pages that need to be updated are even present in a
CLOG buffer.  If it turns out that all of the batched CLOG updates are
part of resident pages, then this is going to work great, just like
the similar ProcArrayLock optimization.  But if the pages are not
resident, then you will get WORSE concurrency and SLOWER performance
than the status quo.  The leader will sit there and read every page
that is needed, and to do that it will repeatedly release and
reacquire CLogControlLock (inside SimpleLruReadPage).  If you didn't
have a leader, the reads of all those pages could happen at the same
time, but with this design, they get serialized.  That's not good.

My idea for how this could possibly work is that you could have a list
of waiting backends for each SLRU buffer page.  Pages with waiting
backends can't be evicted without performing the updates for which
backends are waiting.  Updates to non-resident pages just work as they
do now.  When a backend acquires CLogControlLock to perform updates to
a given page, it also performs all other pending updates to that page
and releases those waiters.  When a backend acquires CLogControlLock
to evict a page, it must perform any pending updates and write the
page before completing the eviction.

I agree with Simon that it's probably a good idea for this
optimization to handle cases where a backend has a non-overflowed list
of subtransactions.  That seems doable.  Handling the case where the
subxid list has overflowed seems unimportant; it should happen rarely
and is therefore not performance-critical.  Also, handling the case
where the XIDs are spread over multiple pages seems far too
complicated to be worth the effort of trying to fit into a "fast
path".  Optimizing the case where there are 1+ XIDs that need to be
updated but all on the same page should cover well over 90% of commits
on real systems, very possibly over 99%.  That should be plenty good
enough to get whatever contention-reduction benefit is possible here.

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

On Thu, Dec 3, 2015 at 1:48 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I think the way to address is don't add backend to Group list if it is
> not intended to update the same page as Group leader.  For transactions
> to be on different pages, they have to be 32768 transactionid's far apart
> and I don't see much possibility of that happening for concurrent
> transactions that are going to be grouped.

That might work.

>> My idea for how this could possibly work is that you could have a list
>> of waiting backends for each SLRU buffer page.
>
> Won't this mean that first we need to ensure that page exists in one of
> the buffers and once we have page in SLRU buffer, we can form the
> list and ensure that before eviction, the list must be processed?
> If my understanding is right, then for this to work we need to probably
> acquire CLogControlLock in Shared mode in addition to acquiring it
> in Exclusive mode for updating the status on page and performing
> pending updates for other backends.

Hmm, that wouldn't be good.  You're right: this is a problem with my
idea.  We can try what you suggested above and see how that works.  We
could also have two or more slots for groups - if a backend doesn't
get the lock, it joins the existing group for the same page, or else
creates a new group if any slot is unused.  I think it might be
advantageous to have at least two groups because otherwise things
might slow down when some transactions are rolling over to a new page
while others are still in flight for the previous page.  Perhaps we
should try it both ways and benchmark.

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

On Sat, Dec 12, 2015 at 8:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

>>  I think it might be
>> advantageous to have at least two groups because otherwise things
>> might slow down when some transactions are rolling over to a new page
>> while others are still in flight for the previous page.  Perhaps we
>> should try it both ways and benchmark.
>>
>
> Sure, I can do the benchmarks with both the patches, but before that
> if you can once check whether group_slots_update_clog_v3.patch is inline
> with what you have in mind then it will be helpful.

Benchmarking sounds good.  This looks broadly like what I was thinking
about, although I'm not very sure you've got all the details right.

Some random comments:

- TransactionGroupUpdateXidStatus could do just as well without
add_proc_to_group.  You could just say if (group_no >= NUM_GROUPS)
break; instead.  Also, I think you could combine the two if statements
inside the loop.  if (nextidx != INVALID_PGPROCNO &&
ProcGlobal->allProcs[nextidx].clogPage == proc->clogPage) break; or
something like that.

- memberXid and memberXidstatus are terrible names.  Member of what?
That's going to be clear as mud to the next person looking at the
definitiono f PGPROC.  And the capitalization of memberXidstatus isn't
even consistent.  Nor is nextupdateXidStatusElem.  Please do give some
thought to the names you pick for variables and structure members.

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

On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> 1. At scale factor 300, there is gain of 11% at 128-client count and
> 27% at 256 client count with Patch-1. At 4 clients, the performance with
> Patch is 0.6% less (which might be a run-to-run variation or there could
> be a small regression, but I think it is too less to be bothered about)
>
> 2. At scale factor 1000, there is no visible difference and there is some
> at lower client count there is a <1% regression which could be due to
> I/O bound nature of test.
>
> 3. On these runs, Patch-2 is mostly always worse than Patch-1, but
> the difference between them is not significant.

Hmm, that's interesting.  So the slots don't help.  I was concerned
that with only a single slot, you might have things moving quickly
until you hit the point where you switch over to the next clog
segment, and then you get a bad stall.  It sounds like that either
doesn't happen in practice, or more likely it does happen but the
extra slot doesn't eliminate the stall because there's I/O at that
point.  Either way, it sounds like we can forget the slots idea for
now.

>> Some random comments:
>>
>> - TransactionGroupUpdateXidStatus could do just as well without
>> add_proc_to_group.  You could just say if (group_no >= NUM_GROUPS)
>> break; instead.  Also, I think you could combine the two if statements
>> inside the loop.  if (nextidx != INVALID_PGPROCNO &&
>> ProcGlobal->allProcs[nextidx].clogPage == proc->clogPage) break; or
>> something like that.
>>
>> - memberXid and memberXidstatus are terrible names.  Member of what?
>
> How about changing them to clogGroupMemberXid and
> clogGroupMemberXidStatus?

What we've currently got for group XID clearing for the ProcArray is
clearXid, nextClearXidElem, and backendLatestXid.  We should try to
make these things consistent.  Maybe rename those to
procArrayGroupMember, procArrayGroupNext, procArrayGroupXid and then
start all of these identifiers with clogGroup as you propose.

>> That's going to be clear as mud to the next person looking at the
>> definitiono f PGPROC.
>
> I understand that you don't like the naming convention, but using
> such harsh language could sometimes hurt others.

Sorry.  If I am slightly frustrated here I think it is because this
same point has been raised about three times now, by me and also by
Andres, just with respect to this particular technique, and also on
other patches.  But you are right - that is no excuse for being rude.

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

On Mon, Dec 21, 2015 at 1:27 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>>
>> >> Some random comments:
>> >>
>> >> - TransactionGroupUpdateXidStatus could do just as well without
>> >> add_proc_to_group.  You could just say if (group_no >= NUM_GROUPS)
>> >> break; instead.  Also, I think you could combine the two if statements
>> >> inside the loop.  if (nextidx != INVALID_PGPROCNO &&
>> >> ProcGlobal->allProcs[nextidx].clogPage == proc->clogPage) break; or
>> >> something like that.
>> >>
>
> Changed as per suggestion.
>
>> >> - memberXid and memberXidstatus are terrible names.  Member of what?
>> >
>> > How about changing them to clogGroupMemberXid and
>> > clogGroupMemberXidStatus?
>>
>> What we've currently got for group XID clearing for the ProcArray is
>> clearXid, nextClearXidElem, and backendLatestXid.  We should try to
>> make these things consistent.  Maybe rename those to
>> procArrayGroupMember, procArrayGroupNext, procArrayGroupXid
>>
>
> Here procArrayGroupXid sounds like Xid at group level, how about
> procArrayGroupMemberXid?
> Find the patch with renamed variables for PGProc
> (rename_pgproc_variables_v1.patch) attached with mail.

I sort of hate to make these member names any longer, but I wonder if
we should make it procArrayGroupClearXid etc.  Otherwise it might be
confused with some other time of grouping of PGPROCs.

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

On Wed, Dec 23, 2015 at 6:16 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> blah.

autovacuum log: Moved to next CF as thread is really active.
-- 
Michael

On Wed, Dec 23, 2015 at 1:16 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Dec 22, 2015 at 10:43 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Mon, Dec 21, 2015 at 1:27 AM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> > On Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <robertmhaas@gmail.com>
>> > wrote:
>> >>
>> >> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit.kapila16@gmail.com>
>> >> wrote:
>> >>
>> >> >> Some random comments:
>> >> >>
>> >> >> - TransactionGroupUpdateXidStatus could do just as well without
>> >> >> add_proc_to_group.  You could just say if (group_no >= NUM_GROUPS)
>> >> >> break; instead.  Also, I think you could combine the two if
>> >> >> statements
>> >> >> inside the loop.  if (nextidx != INVALID_PGPROCNO &&
>> >> >> ProcGlobal->allProcs[nextidx].clogPage == proc->clogPage) break; or
>> >> >> something like that.
>> >> >>
>> >
>> > Changed as per suggestion.
>> >
>> >> >> - memberXid and memberXidstatus are terrible names.  Member of what?
>> >> >
>> >> > How about changing them to clogGroupMemberXid and
>> >> > clogGroupMemberXidStatus?
>> >>
>> >> What we've currently got for group XID clearing for the ProcArray is
>> >> clearXid, nextClearXidElem, and backendLatestXid.  We should try to
>> >> make these things consistent.  Maybe rename those to
>> >> procArrayGroupMember, procArrayGroupNext, procArrayGroupXid
>> >>
>> >
>> > Here procArrayGroupXid sounds like Xid at group level, how about
>> > procArrayGroupMemberXid?
>> > Find the patch with renamed variables for PGProc
>> > (rename_pgproc_variables_v1.patch) attached with mail.
>>
>> I sort of hate to make these member names any longer, but I wonder if
>> we should make it procArrayGroupClearXid etc.
>
> If we go by this suggestion, then the name will look like:
> PGProc
> {
> ..
> bool procArrayGroupClearXid, pg_atomic_uint32 procArrayGroupNextClearXid,
> TransactionId procArrayGroupLatestXid;
> ..
>
> PROC_HDR
> {
> ..
> pg_atomic_uint32 procArrayGroupFirstClearXid;
> ..
> }
>
> I think whatever I sent in last patch were better.  It seems to me it is
> better to add some comments before variable names, so that anybody
> referring them can understand better and I have added comments in
> attached patch rename_pgproc_variables_v2.patch to explain the same.

Well, I don't know.  Anybody else have an opinion?

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

On 7 January 2016 at 05:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Dec 25, 2015 at 6:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Wed, Dec 23, 2015 at 1:16 AM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>> >> >
>> >> > Here procArrayGroupXid sounds like Xid at group level, how about
>> >> > procArrayGroupMemberXid?
>> >> > Find the patch with renamed variables for PGProc
>> >> > (rename_pgproc_variables_v1.patch) attached with mail.
>> >>
>> >> I sort of hate to make these member names any longer, but I wonder if
>> >> we should make it procArrayGroupClearXid etc.
>> >
>> > If we go by this suggestion, then the name will look like:
>> > PGProc
>> > {
>> > ..
>> > bool procArrayGroupClearXid, pg_atomic_uint32
>> > procArrayGroupNextClearXid,
>> > TransactionId procArrayGroupLatestXid;
>> > ..
>> >
>> > PROC_HDR
>> > {
>> > ..
>> > pg_atomic_uint32 procArrayGroupFirstClearXid;
>> > ..
>> > }
>> >
>> > I think whatever I sent in last patch were better.  It seems to me it is
>> > better to add some comments before variable names, so that anybody
>> > referring them can understand better and I have added comments in
>> > attached patch rename_pgproc_variables_v2.patch to explain the same.
>>
>> Well, I don't know.  Anybody else have an opinion?
>>
>
> It seems that either people don't have any opinion on this matter or they
> are okay with either of the naming conventions being discussed.  I think
> specifying Member after procArrayGroup can help distinguishing which
> variables are specific to the whole group and which are specific to a
> particular member.  I think that will be helpful for other places as well
> if we use this technique to improve performance.  Let me know what
> you think about the same.
>
> I have verified that previous patches can be applied cleanly and passes
> make check-world.  To avoid confusion, I am attaching the latest
> patches with this mail.

Patches still apply 1 month later.

I don't really have an opinion on the variable naming.  I guess they
only need making longer if there's going to be some confusion about
what they're for, but I'm guessing it's not a blocker here.

Thom

On Tue, Feb 9, 2016 at 11:14 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Patches still apply 1 month later.
>
> Thanks for verification!
>
>>
>> I don't really have an opinion on the variable naming.  I guess they
>> only need making longer if there's going to be some confusion about
>> what they're for,
>
> makes sense, that is the reason why I have added few comments
> as well, but not sure if you are suggesting something else.
>
>> but I'm guessing it's not a blocker here.
>>
>
> I also think so, but not sure what else is required here.  The basic
> idea of this rename_pgproc_variables_v2.patch is to rename
> few variables in existing similar code, so that the main patch
> group_update_clog can adapt those naming convention if required,
> other than that I have handled all review comments raised in this
> thread (mainly by Simon and Robert).
>
> Is there anything, I can do to move this forward?

Well, looking at this again, I think I'm OK to go with your names.
That doesn't seem like the thing to hold up the patch for.  So I'll go
ahead and push the renaming patch now.

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

On Thu, Feb 11, 2016 at 9:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Is there anything, I can do to move this forward?
>
> Well, looking at this again, I think I'm OK to go with your names.
> That doesn't seem like the thing to hold up the patch for.  So I'll go
> ahead and push the renaming patch now.

On the substantive part of the patch, this doesn't look safe:

+    /*
+     * Add ourselves to the list of processes needing a group XID status
+     * update.
+     */
+    proc->clogGroupMember = true;
+    proc->clogGroupMemberXid = xid;
+    proc->clogGroupMemberXidStatus = status;
+    proc->clogGroupMemberPage = pageno;
+    proc->clogGroupMemberLsn = lsn;
+    while (true)
+    {
+        nextidx = pg_atomic_read_u32(&procglobal->clogGroupFirst);
+
+        /*
+         * Add the proc to list if the clog page where we need to update the
+         * current transaction status is same as group leader's clog page.
+         */
+        if (nextidx != INVALID_PGPROCNO &&
+            ProcGlobal->allProcs[nextidx].clogGroupMemberPage !=
proc->clogGroupMemberPage)
+            return false;

DANGER HERE!

+        pg_atomic_write_u32(&proc->clogGroupNext, nextidx);
+
+        if (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
+                                           &nextidx,
+                                           (uint32) proc->pgprocno))
+            break;
+    }

There is a potential ABA problem here.  Suppose that this code
executes in one process as far as the line that says DANGER HERE.
Then, the group leader wakes up, performs all of the CLOG
modifications, performs another write transaction, and again becomes
the group leader, but for a different member page.  Then, the original
process that went to sleep at DANGER HERE wakes up.  At this point,
the pg_atomic_compare_exchange_u32 will succeed and we'll have
processes with different pages in the list, contrary to the intention
of the code.

This kind of thing can be really subtle and difficult to fix.  The
problem might not happen even with a very large amount of testing, and
then might happen in the real world on some other hardware or on
really rare occasions.  In general, compare-and-swap loops need to be
really really simple with minimal dependencies on other data, ideally
none.  It's very hard to make anything else work.

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

On Fri, Feb 12, 2016 at 12:55 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Very Good Catch.  I think if we want to address this we can detect
> the non-group leader transactions that tries to update the different
> CLOG page (different from group-leader) after acquiring
> CLogControlLock and then mark these transactions such that
> after waking they need to perform CLOG update via normal path.
> Now this can decrease the latency of such transactions, but I

I think you mean "increase".

> think there will be only very few transactions if at-all there which
> can face this condition, because most of the concurrent transactions
> should be on same page, otherwise the idea of multiple-slots we
> have tried upthread would have shown benefits.
> Another idea could be that we update the comments indicating the
> possibility of multiple Clog-page updates in same group on the basis
> that such cases will be less and even if it happens, it won't effect the
> transaction status update.

I think either approach of those approaches could work, as long as the
logic is correct and the comments are clear.  The important thing is
that the code had better do something safe if this situation ever
occurs, and the comments had better be clear that this is a possible
situation so that someone modifying the code in the future doesn't
think it's impossible, rely on it not happening, and consequently
introduce a very-low-probability bug.

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

On Fri, Feb 26, 2016 at 11:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>> Here, we can see that there is a gain of ~15% to ~38% at higher client
>> count.
>>
>> The attached document (perf_write_clogcontrollock_data_v6.ods) contains
>> data, mainly focussing on single client performance.  The data is for
>> multiple runs on different machines, so I thought it is better to present in
>> form of document rather than dumping everything in e-mail.  Do let me know
>> if there is any confusion in understanding/interpreting the data.
>
> Forgot to mention that all these tests have been done by reverting
> commit-ac1d794.

OK, that seems better.  But I have a question: if we don't really need
to make this optimization apply only when everything is on the same
page, then why even try?  If we didn't try, we wouldn't need the
all_trans_same_page flag, which would reduce the amount of code
change.  Would that hurt anything? Taking it even further, we could
remove the check from TransactionGroupUpdateXidStatus too.  I'd be
curious to know whether that set of changes would improve performance
or regress it.  Or maybe it does nothing, in which case perhaps
simpler is better.

All things being equal, it's probably better if the cases where
transactions from different pages get into the list together is
something that is more or less expected rather than a
once-in-a-blue-moon scenario - that way, if any bugs exist, we'll find
them.  The downside of that is that we could increase latency for the
leader that way - doing other work on the same page shouldn't hurt
much but different pages is a bigger hit.  But that hit might be
trivial enough not to be worth worrying about.

+       /*
+        * Now that we've released the lock, go back and wake everybody up.  We
+        * don't do this under the lock so as to keep lock hold times to a
+        * minimum.  The system calls we need to perform to wake other processes
+        * up are probably much slower than the simple memory writes
we did while
+        * holding the lock.
+        */

This comment was true in the place that you cut-and-pasted it from,
but it's not true here, since we potentially need to read from disk.

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

On 2/26/16 11:37 PM, Amit Kapila wrote:

> On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit.kapila16@gmail.com
>
>     Here, we can see that there is a gain of ~15% to ~38% at higher
>     client count.
>
>     The attached document (perf_write_clogcontrollock_data_v6.ods)
>     contains data, mainly focussing on single client performance.  The
>     data is for multiple runs on different machines, so I thought it is
>     better to present in form of document rather than dumping everything
>     in e-mail.  Do let me know if there is any confusion in
>     understanding/interpreting the data.
>
> Forgot to mention that all these tests have been done by
> reverting commit-ac1d794.

This patch no longer applies cleanly:

$ git apply ../other/group_update_clog_v6.patch
error: patch failed: src/backend/storage/lmgr/proc.c:404
error: src/backend/storage/lmgr/proc.c: patch does not apply
error: patch failed: src/include/storage/proc.h:152
error: src/include/storage/proc.h: patch does not apply

It's not clear to me whether Robert has completed a review of this code 
or it still needs to be reviewed more comprehensively.

Other than a comment that needs to be fixed it seems that all questions 
have been answered by Amit.

Is this "ready for committer" or still in need of further review?

-- 
-David
david@pgmasters.net

On 3/15/16 1:17 AM, Amit Kapila wrote:

> On Tue, Mar 15, 2016 at 12:00 AM, David Steele <david@pgmasters.net
>
>> This patch no longer applies cleanly:
>>
>> $ git apply ../other/group_update_clog_v6.patch
>> error: patch failed: src/backend/storage/lmgr/proc.c:404
>> error: src/backend/storage/lmgr/proc.c: patch does not apply
>> error: patch failed: src/include/storage/proc.h:152
>> error: src/include/storage/proc.h: patch does not apply
> 
> For me, with patch -p1 < <path_of_patch> it works, but any how I have
> updated the patch based on recent commit.  Can you please check the
> latest patch and see if it applies cleanly for you now.

Yes, it now applies cleanly (101fd93).

-- 
-David
david@pgmasters.net

David Steele wrote:

> This patch no longer applies cleanly:
> 
> $ git apply ../other/group_update_clog_v6.patch

Normally "git apply -3" gives good results in these cases -- it applies
the 3-way merge algorithm just as if you had applied the patch to the
revision it was built on and later git-merged with the latest head.

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

On 03/15/2016 01:17 AM, Amit Kapila wrote:
> I have updated the comments and changed the name of one of a variable from
> "all_trans_same_page" to "all_xact_same_page" as pointed out offlist by
> Alvaro.
>
>

I have done a run, and don't see any regressions.

Intel Xeon 28C/56T @ 2GHz w/ 256GB + 2 x RAID10 (data + xlog) SSD.

I can provide perf / flamegraph profiles if needed.

Thanks for working on this !

Best regards,
  Jesper

Hi,

On 2016-03-15 10:47:12 +0530, Amit Kapila wrote:
> @@ -248,12 +256,67 @@ set_status_by_pages(int nsubxids, TransactionId *subxids,
>   * Record the final state of transaction entries in the commit log for
>   * all entries on a single page.  Atomic only on this page.
>   *
> + * Group the status update for transactions. This improves the efficiency
> + * of the transaction status update by reducing the number of lock
> + * acquisitions required for it.  To achieve the group transaction status
> + * update, we need to populate the transaction status related information
> + * in shared memory and doing it for overflowed sub-transactions would need
> + * a big chunk of shared memory, so we are not doing this optimization for
> + * such cases. This optimization is only applicable if the transaction and
> + * all child sub-transactions belong to same page which we presume to be the
> + * most common case, we might be able to apply this when they are not on same
> + * page, but that needs us to map sub-transactions in proc's XidCache based
> + * on pageno for which each time a group leader needs to set the transaction
> + * status and that can lead to some performance penalty as well because it
> + * needs to be done after acquiring CLogControlLock, so let's leave that
> + * case for now.  We don't do this optimization for prepared transactions
> + * as the dummy proc associated with such transactions doesn't have a
> + * semaphore associated with it and the same is required for group status
> + * update.  We choose not to create a semaphore for dummy procs for this
> + * purpose as the advantage of using this optimization for prepared transactions
> + * is not clear.
> + *

I think you should try to break up some of the sentences, one of them
spans 7 lines.

I'm actually rather unconvinced that it's all that common that all
subtransactions are on one page. If you have concurrency - otherwise
there'd be not much point in this patch - they'll usually be heavily
interleaved, no?  You can argue that you don't care about subxacts,
because they're more often used in less concurrent scenarios, but if
that's the argument, it should actually be made.


>   * Otherwise API is same as TransactionIdSetTreeStatus()
>   */
>  static void
>  TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
>                             TransactionId *subxids, XidStatus status,
> -                           XLogRecPtr lsn, int pageno)
> +                           XLogRecPtr lsn, int pageno,
> +                           bool all_xact_same_page)
> +{
> +    /*
> +     * If we can immediately acquire CLogControlLock, we update the status
> +     * of our own XID and release the lock.  If not, use group XID status
> +     * update to improve efficiency and if still not able to update, then
> +     * acquire CLogControlLock and update it.
> +     */
> +    if (LWLockConditionalAcquire(CLogControlLock, LW_EXCLUSIVE))
> +    {
> +        TransactionIdSetPageStatusInternal(xid, nsubxids, subxids, status, lsn, pageno);
> +        LWLockRelease(CLogControlLock);
> +    }
> +    else if (!all_xact_same_page ||
> +             nsubxids > PGPROC_MAX_CACHED_SUBXIDS ||
> +             IsGXactActive() ||
> +             !TransactionGroupUpdateXidStatus(xid, status, lsn, pageno))
> +    {
> +        LWLockAcquire(CLogControlLock, LW_EXCLUSIVE);
> +
> +        TransactionIdSetPageStatusInternal(xid, nsubxids, subxids, status, lsn, pageno);
> +
> +        LWLockRelease(CLogControlLock);
> +    }
> +}
>

This code is a bit arcane. I think it should be restructured to
a) Directly go for LWLockAcquire if !all_xact_same_page || nsubxids >  PGPROC_MAX_CACHED_SUBXIDS || IsGXactActive().
Goingfor a conditional  lock acquire first can be rather expensive.
 
b) I'd rather see an explicit fallback for the  !TransactionGroupUpdateXidStatus case, this way it's too hard to
understand.It's also harder to add probes to detect whether that
 


> +
> +/*
> + * When we cannot immediately acquire CLogControlLock in exclusive mode at
> + * commit time, add ourselves to a list of processes that need their XIDs
> + * status update.

At this point my "ABA Problem" alarm goes off. If it's not an actual
danger, can you please document close by, why not?


> The first process to add itself to the list will acquire
> + * CLogControlLock in exclusive mode and perform TransactionIdSetPageStatusInternal
> + * on behalf of all group members.  This avoids a great deal of contention
> + * around CLogControlLock when many processes are trying to commit at once,
> + * since the lock need not be repeatedly handed off from one committing
> + * process to the next.
> + *
> + * Returns true, if transaction status is updated in clog page, else return
> + * false.
> + */
> +static bool
> +TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
> +                                XLogRecPtr lsn, int pageno)
> +{
> +    volatile PROC_HDR *procglobal = ProcGlobal;
> +    PGPROC       *proc = MyProc;
> +    uint32        nextidx;
> +    uint32        wakeidx;
> +    int            extraWaits = -1;
> +
> +    /* We should definitely have an XID whose status needs to be updated. */
> +    Assert(TransactionIdIsValid(xid));
> +
> +    /*
> +     * Add ourselves to the list of processes needing a group XID status
> +     * update.
> +     */
> +    proc->clogGroupMember = true;
> +    proc->clogGroupMemberXid = xid;
> +    proc->clogGroupMemberXidStatus = status;
> +    proc->clogGroupMemberPage = pageno;
> +    proc->clogGroupMemberLsn = lsn;
> +    while (true)
> +    {
> +        nextidx = pg_atomic_read_u32(&procglobal->clogGroupFirst);
> +
> +        /*
> +         * Add the proc to list, if the clog page where we need to update the
> +         * current transaction status is same as group leader's clog page.
> +         * There is a race condition here such that after doing the below
> +         * check and before adding this proc's clog update to a group, if the
> +         * group leader already finishes the group update for this page and
> +         * becomes group leader of another group which updates different clog
> +         * page, then it will lead to a situation where a single group can
> +         * have different clog page updates.  Now the chances of such a race
> +         * condition are less and even if it happens, the only downside is
> +         * that it could lead to serial access of clog pages from disk if
> +         * those pages are not in memory.  Tests doesn't indicate any
> +         * performance hit due to different clog page updates in same group,
> +         * however in future, if we want to improve the situation, then we can
> +         * detect the non-group leader transactions that tries to update the
> +         * different CLOG page after acquiring CLogControlLock and then mark
> +         * these transactions such that after waking they need to perform CLOG
> +         * update via normal path.
> +         */

Needs a good portion of polishing.


> +        if (nextidx != INVALID_PGPROCNO &&
> +            ProcGlobal->allProcs[nextidx].clogGroupMemberPage != proc->clogGroupMemberPage)
> +            return false;

I think we're returning with clogGroupMember = true - that doesn't look
right.


> +        pg_atomic_write_u32(&proc->clogGroupNext, nextidx);
> +
> +        if (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
> +                                           &nextidx,
> +                                           (uint32) proc->pgprocno))
> +            break;
> +    }

So this indeed has ABA type problems. And you appear to be arguing above
that that's ok. Need to ponder that for a bit.

So, we enqueue ourselves as the *head* of the wait list, if there's
other waiters. Seems like it could lead to the first element after the
leader to be delayed longer than the others.


FWIW, You can move the nextidx = part of out the loop,
pgatomic_compare_exchange will update the nextidx value from memory; no
need for another load afterwards.


> +    /*
> +     * If the list was not empty, the leader will update the status of our
> +     * XID. It is impossible to have followers without a leader because the
> +     * first process that has added itself to the list will always have
> +     * nextidx as INVALID_PGPROCNO.
> +     */
> +    if (nextidx != INVALID_PGPROCNO)
> +    {
> +        /* Sleep until the leader updates our XID status. */
> +        for (;;)
> +        {
> +            /* acts as a read barrier */
> +            PGSemaphoreLock(&proc->sem);
> +            if (!proc->clogGroupMember)
> +                break;
> +            extraWaits++;
> +        }
> +
> +        Assert(pg_atomic_read_u32(&proc->clogGroupNext) == INVALID_PGPROCNO);
> +
> +        /* Fix semaphore count for any absorbed wakeups */
> +        while (extraWaits-- > 0)
> +            PGSemaphoreUnlock(&proc->sem);
> +        return true;
> +    }
> +
> +    /* We are the leader.  Acquire the lock on behalf of everyone. */
> +    LWLockAcquire(CLogControlLock, LW_EXCLUSIVE);
> +
> +    /*
> +     * Now that we've got the lock, clear the list of processes waiting for
> +     * group XID status update, saving a pointer to the head of the list.
> +     * Trying to pop elements one at a time could lead to an ABA problem.
> +     */
> +    while (true)
> +    {
> +        nextidx = pg_atomic_read_u32(&procglobal->clogGroupFirst);
> +        if (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
> +                                           &nextidx,
> +                                           INVALID_PGPROCNO))
> +            break;
> +    }

Hm. It seems like you should should simply use pg_atomic_exchange_u32(),
rather than compare_exchange?


> diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
> index c4fd9ef..120b9c0 100644
> --- a/src/backend/access/transam/twophase.c
> +++ b/src/backend/access/transam/twophase.c
> @@ -177,7 +177,7 @@ static TwoPhaseStateData *TwoPhaseState;
>  /*
>   * Global transaction entry currently locked by us, if any.
>   */
> -static GlobalTransaction MyLockedGxact = NULL;
> +GlobalTransaction MyLockedGxact = NULL;

Hm, I'm doubtful it's worthwhile to expose this, just so we can use an
inline function, but whatever.


> +#include "access/clog.h"
>  #include "access/xlogdefs.h"
>  #include "lib/ilist.h"
>  #include "storage/latch.h"
> @@ -154,6 +155,17 @@ struct PGPROC
>  
>      uint32          wait_event_info;        /* proc's wait information */
>  
> +    /* Support for group transaction status update. */
> +    bool        clogGroupMember;    /* true, if member of clog group */
> +    pg_atomic_uint32 clogGroupNext;        /* next clog group member */
> +    TransactionId clogGroupMemberXid;    /* transaction id of clog group member */
> +    XidStatus    clogGroupMemberXidStatus;        /* transaction status of clog
> +                                                 * group member */
> +    int            clogGroupMemberPage;    /* clog page corresponding to
> +                                         * transaction id of clog group member */
> +    XLogRecPtr    clogGroupMemberLsn;        /* WAL location of commit record for
> +                                         * clog group member */
> +

Man, we're surely bloating PGPROC at a prodigious rate.


That's my first pass over the code itself.


Hm. Details aside, what concerns me most is that the whole group
mechanism, as implemented, only works als long as transactions only span
a short and regular amount of time. As soon as there's some variance in
transaction duration, the likelihood of building a group, where all xids
are on one page, diminishes. That likely works well in benchmarking, but
I'm afraid it's much less the case in the real world, where there's
network latency involved, and where applications actually contain
computations themselves.

If I understand correctly, without having followed the thread, the
reason you came up with this batching on a per-page level is to bound
the amount of effort spent by the leader; and thus bound the latency?

I think it's worthwhile to create a benchmark that does something like
BEGIN;SELECT ... FOR UPDATE; SELECT pg_sleep(random_time);
INSERT;COMMIT; you'd find that if random is a bit larger (say 20-200ms,
completely realistic values for network RTT + application computation),
the success rate of group updates shrinks noticeably.

Greetings,

Andres Freund

On 2016-03-22 18:19:48 +0530, Amit Kapila wrote:
> > I'm actually rather unconvinced that it's all that common that all
> > subtransactions are on one page. If you have concurrency - otherwise
> > there'd be not much point in this patch - they'll usually be heavily
> > interleaved, no?  You can argue that you don't care about subxacts,
> > because they're more often used in less concurrent scenarios, but if
> > that's the argument, it should actually be made.
> >
> 
> Note, that we are doing it only when a transaction has less than equal to
> 64 sub transactions.

So?

> > This code is a bit arcane. I think it should be restructured to
> > a) Directly go for LWLockAcquire if !all_xact_same_page || nsubxids >
> >    PGPROC_MAX_CACHED_SUBXIDS || IsGXactActive(). Going for a conditional
> >    lock acquire first can be rather expensive.
> 
> The previous version (v5 - [1]) has code that way, but that adds few extra
> instructions for single client case and I was seeing minor performance
> regression for single client case due to which it has been changed as per
> current code.

I don't believe that changing conditions here is likely to cause a
measurable regression.


> > So, we enqueue ourselves as the *head* of the wait list, if there's
> > other waiters. Seems like it could lead to the first element after the
> > leader to be delayed longer than the others.
> >
> 
> It will not matter because we are waking the queued process only once we
> are done with xid status update.

If there's only N cores, process N+1 won't be run immediately. But yea,
it's probably not large.


> > FWIW, You can move the nextidx = part of out the loop,
> > pgatomic_compare_exchange will update the nextidx value from memory; no
> > need for another load afterwards.
> >
> 
> Not sure, if I understood which statement you are referring here (are you
> referring to atomic read operation) and how can we save the load operation?

Yes, to the atomic read. And we can save it in the loop, because
compare_exchange returns the current value if it fails.


> > > +      * Now that we've got the lock, clear the list of processes
> waiting for
> > > +      * group XID status update, saving a pointer to the head of the
> list.
> > > +      * Trying to pop elements one at a time could lead to an ABA
> problem.
> > > +      */
> > > +     while (true)
> > > +     {
> > > +             nextidx = pg_atomic_read_u32(&procglobal->clogGroupFirst);
> > > +             if
> (pg_atomic_compare_exchange_u32(&procglobal->clogGroupFirst,
> > > +
>          &nextidx,
> > > +
>          INVALID_PGPROCNO))
> > > +                     break;
> > > +     }
> >
> > Hm. It seems like you should should simply use pg_atomic_exchange_u32(),
> > rather than compare_exchange?
> >
> 
> We need to remember the head of list to wake up the processes due to which
> I think above loop is required.

exchange returns the old value? There's no need for a compare here.


> > I think it's worthwhile to create a benchmark that does something like
> > BEGIN;SELECT ... FOR UPDATE; SELECT pg_sleep(random_time);
> > INSERT;COMMIT; you'd find that if random is a bit larger (say 20-200ms,
> > completely realistic values for network RTT + application computation),
> > the success rate of group updates shrinks noticeably.
> >
> 
> I think it will happen that way, but what do we want to see with that
> benchmark? I think the results will be that for such a workload either
> there is no benefit or will be very less as compare to short transactions.

Because we want our performance improvements to matter in reality, not
just in unrealistic benchmarks where the benchmarking tool is running on
the same machine as the the database, and uses unix sockets. That not
actually an all that realistic workload.


Andres

On Tue, Mar 22, 2016 at 6:52 AM, Andres Freund <andres@anarazel.de> wrote:
> I'm actually rather unconvinced that it's all that common that all
> subtransactions are on one page. If you have concurrency - otherwise
> there'd be not much point in this patch - they'll usually be heavily
> interleaved, no?  You can argue that you don't care about subxacts,
> because they're more often used in less concurrent scenarios, but if
> that's the argument, it should actually be made.

But a single clog page holds a lot of transactions - I think it's
~32k.  If you have 100 backends running, and each one allocates an XID
in turn, and then each allocates a sub-XID in turn, and then they all
commit, and then you repeat this pattern, >99% of transactions will be
on a single CLOG page.  And that is a pretty pathological case.

It's true that if you have many short-running transactions interleaved
with occasional long-running transactions, and the latter use
subxacts, the optimization might fail to apply to the long-running
subxacts fairly often.  But who cares?  Those are, by definition, a
small percentage of the overall transaction stream.

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

On 2016-03-22 10:40:28 -0400, Robert Haas wrote:
> On Tue, Mar 22, 2016 at 6:52 AM, Andres Freund <andres@anarazel.de> wrote:
> > I'm actually rather unconvinced that it's all that common that all
> > subtransactions are on one page. If you have concurrency - otherwise
> > there'd be not much point in this patch - they'll usually be heavily
> > interleaved, no?  You can argue that you don't care about subxacts,
> > because they're more often used in less concurrent scenarios, but if
> > that's the argument, it should actually be made.
> 
> But a single clog page holds a lot of transactions - I think it's
> ~32k.

At 30-40k TPS/sec that's not actually all that much.


> If you have 100 backends running, and each one allocates an XID
> in turn, and then each allocates a sub-XID in turn, and then they all
> commit, and then you repeat this pattern, >99% of transactions will be
> on a single CLOG page.  And that is a pretty pathological case.

I think it's much more likely that some backends will immediately
allocate and others won't for a short while.


> It's true that if you have many short-running transactions interleaved
> with occasional long-running transactions, and the latter use
> subxacts, the optimization might fail to apply to the long-running
> subxacts fairly often.  But who cares?  Those are, by definition, a
> small percentage of the overall transaction stream.

Leaving subtransactions aside, I think the problem is that if you're
having slightly longer running transactions on a regular basis (and I'm
thinking 100-200ms, very common on OLTP systems due to network and
client processing), the effectiveness of the batching will be greatly
reduced.

I'll play around with the updated patch Amit promised, and see how high
the batching rate is over time, depending on the type of transaction
processed.


Andres

On 3/22/16 9:36 AM, Amit Kapila wrote:
>  > > Note, that we are doing it only when a transaction has less than
> equal to
>  > > 64 sub transactions.
>  >
>  > So?
>  >
>
> They should fall on one page, unless they are heavily interleaved as
> pointed by you.  I think either subtransactions are present or not, this
> patch won't help for bigger transactions.

FWIW, the use case that comes to mind here is the "upsert" example in 
the docs. AFAIK that's going to create a subtransaction every time it's 
called, regardless if whether it performs actual DML. I've used that in 
places that would probably have moderately high concurrency, and I 
suspect I'm not alone in that.

That said, it wouldn't surprise me if plpgsql overhead swamps an effect 
this patch has, so perhaps it's a moot point.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 2016-03-23 12:33:22 +0530, Amit Kapila wrote:
> On Wed, Mar 23, 2016 at 12:26 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Tue, Mar 22, 2016 at 4:22 PM, Andres Freund <andres@anarazel.de> wrote:
> > >
> > >
> > > I think it's worthwhile to create a benchmark that does something like
> > > BEGIN;SELECT ... FOR UPDATE; SELECT pg_sleep(random_time);
> > > INSERT;COMMIT; you'd find that if random is a bit larger (say 20-200ms,
> > > completely realistic values for network RTT + application computation),
> > > the success rate of group updates shrinks noticeably.
> > >
> >
> > Will do some tests based on above test and share results.
> >
> 
> Forgot to mention that the effect of patch is better visible with unlogged
> tables, so will do the test with those and request you to use same if you
> yourself is also planning to perform some tests.

I'm playing around with SELECT txid_current(); right now - that should
be about the most specific load for setting clog bits.

Andres

On 2016-03-23 21:43:41 +0100, Andres Freund wrote:
> I'm playing around with SELECT txid_current(); right now - that should
> be about the most specific load for setting clog bits.

Or so I thought.

In my testing that showed just about zero performance difference between
the patch and master. And more surprisingly, profiling showed very
little contention on the control lock. Hacking
TransactionIdSetPageStatus() to return without doing anything, actually
only showed minor performance benefits.

[there's also the fact that txid_current() indirectly acquires two
lwlock twice, which showed up more prominently than control lock, but
that I could easily hack around by adding a xid_current().]

Similar with an INSERT only workload. And a small scale pgbench.


Looking through the thread showed that the positive results you'd posted
all were with relativey big scale factors. Which made me think. Running
a bigger pgbench showed that most the interesting (i.e. long) lock waits
were both via TransactionIdSetPageStatus *and* TransactionIdGetStatus().


So I think what happens is that once you have a big enough table, the
UPDATEs standard pgbench does start to often hit *old* xids (in unhinted
rows). Thus old pages have to be read in, potentially displacing slru
content needed very shortly after.


Have you, in your evaluation of the performance of this patch, done
profiles over time? I.e. whether the performance benefits are the
immediately, or only after a significant amount of test time? Comparing
TPS over time, for both patched/unpatched looks relevant.


Even after changing to scale 500, the performance benefits on this,
older 2 socket, machine were minor; even though contention on the
ClogControlLock was the second most severe (after ProcArrayLock).

Afaics that squares with Jesper's result, which basically also didn't
show a difference either way?


I'm afraid that this patch might be putting bandaid on some of the
absolutely worst cases, without actually addressing the core
problem. Simon's patch in [1] seems to come closer addressing that
(which I don't believe it's safe without going doing every status
manipulation atomically, as individual status bits are smaller than 4
bytes).  Now it's possibly to argue that the bandaid might slow the
bleeding to a survivable level, but I have to admit I'm doubtful.

Here's the stats for a -s 500 run btw:Performance counter stats for 'system wide':           18,747
probe_postgres:TransactionIdSetTreeStatus          68,884      probe_postgres:TransactionIdGetStatus            9,718
  probe_postgres:PGSemaphoreLock
 
(the PGSemaphoreLock is over 50% ProcArrayLock, followed by ~15%
SimpleLruReadPage_ReadOnly)


My suspicion is that a better approach for now would be to take Simon's
patch, but add a (per-page?) 'ClogModificationLock'; to avoid the need
of doing something fancier in TransactionIdSetStatusBit().

Andres

[1]:
http://archives.postgresql.org/message-id/CANP8%2Bj%2BimQfHxkChFyfnXDyi6k-arAzRV%2BZG-V_OFxEtJjOL2Q%40mail.gmail.com

Hi,

On 2016-03-24 01:10:55 +0100, Andres Freund wrote:
> I'm afraid that this patch might be putting bandaid on some of the
> absolutely worst cases, without actually addressing the core
> problem. Simon's patch in [1] seems to come closer addressing that
> (which I don't believe it's safe without going doing every status
> manipulation atomically, as individual status bits are smaller than 4
> bytes).  Now it's possibly to argue that the bandaid might slow the
> bleeding to a survivable level, but I have to admit I'm doubtful.
> 
> Here's the stats for a -s 500 run btw:
>  Performance counter stats for 'system wide':
>             18,747      probe_postgres:TransactionIdSetTreeStatus
>             68,884      probe_postgres:TransactionIdGetStatus
>              9,718      probe_postgres:PGSemaphoreLock
> (the PGSemaphoreLock is over 50% ProcArrayLock, followed by ~15%
> SimpleLruReadPage_ReadOnly)
> 
> 
> My suspicion is that a better approach for now would be to take Simon's
> patch, but add a (per-page?) 'ClogModificationLock'; to avoid the need
> of doing something fancier in TransactionIdSetStatusBit().
> 
> Andres
> 
> [1]:
http://archives.postgresql.org/message-id/CANP8%2Bj%2BimQfHxkChFyfnXDyi6k-arAzRV%2BZG-V_OFxEtJjOL2Q%40mail.gmail.com

Simon, would you mind if I took your patch for a spin like roughly
suggested above?


Andres

On 2016-03-28 22:50:49 +0530, Amit Kapila wrote:
> On Fri, Sep 11, 2015 at 8:01 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Thu, Sep 3, 2015 at 5:11 PM, Andres Freund <andres@anarazel.de> wrote:
> > >
> >
> > Updated comments and the patch (increate_clog_bufs_v2.patch)
> > containing the same is attached.
> >
>
> Andres mentioned to me in off-list discussion, that he thinks we should
> first try to fix the clog buffers problem as he sees in his tests that clog
> buffer replacement is one of the bottlenecks. He also suggested me a test
> to see if the increase in buffers could lead to regression.  The basic idea
> of test was to ensure every access on clog access to be a disk one.  Based
> on his suggestion, I have written a SQL statement which will allow every
> access of CLOG to be a disk access and the query used for same is as below:
> With ins AS (INSERT INTO test_clog_access values(default) RETURNING c1)
> Select * from test_clog_access where c1 = (Select c1 from ins) - 32768 *
> :client_id;
>
> Test Results
> ---------------------
> HEAD - commit d12e5bb7 Clog Buffers - 32
> Patch-1 - Clog Buffers - 64
> Patch-2 - Clog Buffers - 128
>
>
> Patch_Ver/Client_Count 1 64
> HEAD 12677 57470
> Patch-1 12305 58079
> Patch-2 12761 58637
>
> Above data is a median of 3 10-min runs.  Above data indicates that there
> is no substantial dip in increasing clog buffers.
>
> Test scripts used in testing are attached with this mail.  In
> perf_clog_access.sh, you need to change data_directory path as per your
> m/c, also you might want to change the binary name, if you want to create
> postgres binaries with different names.
>
> Andres, Is this test inline with what you have in mind?

Yes. That looks good. My testing shows that increasing the number of
buffers can increase both throughput and reduce latency variance. The
former is a smaller effect with one of the discussed patches applied,
the latter seems to actually increase in scale (with increased
throughput).


I've attached patches to:
0001: Increase the max number of clog buffers
0002: Implement 64bit atomics fallback and optimize read/write
0003: Edited version of Simon's clog scalability patch

WRT 0003 - still clearly WIP - I've:
- made group_lsn pg_atomic_u64*, to allow for tear-free reads
- split content from IO lock
- made SimpleLruReadPage_optShared always return with only share lock
  held
- Implement a different, experimental, concurrency model for
  SetStatusBit using cmpxchg. A define USE_CONTENT_LOCK controls which
  bit is used.

I've tested this and saw this outperform Amit's approach. Especially so
when using a read/write mix, rather then only reads. I saw over 30%
increase on a large EC2 instance with -btpcb-like@1 -bselect-only@3. But
that's in a virtualized environment, not very good for reproducability.

Amit, could you run benchmarks on your bigger hardware? Both with
USE_CONTENT_LOCK commented out and in?

I think we should go for 1) and 2) unconditionally. And then evaluate
whether to go with your, or 3) from above. If the latter, we've to do
some cleanup :)

Greetings,

Andres Freund

On 2016-03-31 15:07:22 +0530, Amit Kapila wrote:
> On Thu, Mar 31, 2016 at 4:39 AM, Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2016-03-28 22:50:49 +0530, Amit Kapila wrote:
> > > On Fri, Sep 11, 2015 at 8:01 PM, Amit Kapila <amit.kapila16@gmail.com>
> > > wrote:
> > > >
> >
> > Amit, could you run benchmarks on your bigger hardware? Both with
> > USE_CONTENT_LOCK commented out and in?
> >
> 
> Yes.

Cool.


> > I think we should go for 1) and 2) unconditionally.

> Yes, that makes sense.  On 20 min read-write pgbench --unlogged-tables
> benchmark, I see that with HEAD Tps is 36241 and with increase the clog
> buffers patch, Tps is 69340 at 128 client count (very good performance
> boost) which indicates that we should go ahead with 1) and 2) patches.

Especially considering the line count... I do wonder about going crazy
and increasing to 256 immediately. It otherwise seems likely that we'll
have the the same issue in a year.  Could you perhaps run your test
against that as well?


> I think we should change comments on top of this function.

Yes, definitely.


> 0001-Improve-64bit-atomics-support
> 
> +#if 0
> +#ifndef PG_HAVE_ATOMIC_READ_U64
> +#define PG_HAVE_ATOMIC_READ_U64
> +static inline uint64
> 
> What the purpose of above #if 0?  Other than that patch looks good to me.

I think I was investigating something. Other than that obviously there's
no point. Sorry for that.

Greetings,

Andres Freund

On 2016-03-31 17:52:12 +0530, Amit Kapila wrote:
> On Thu, Mar 31, 2016 at 3:48 PM, Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2016-03-31 15:07:22 +0530, Amit Kapila wrote:
> > > On Thu, Mar 31, 2016 at 4:39 AM, Andres Freund <andres@anarazel.de>
> wrote:
> > > >
> > > > On 2016-03-28 22:50:49 +0530, Amit Kapila wrote:
> > > > > On Fri, Sep 11, 2015 at 8:01 PM, Amit Kapila <
> amit.kapila16@gmail.com>
> > > > > wrote:
> > > > > >
> > > >
> > > > Amit, could you run benchmarks on your bigger hardware? Both with
> > > > USE_CONTENT_LOCK commented out and in?
> > > >
> > >
> > > Yes.
> >
> > Cool.
> >
> >
> > > > I think we should go for 1) and 2) unconditionally.
> >
> > > Yes, that makes sense.  On 20 min read-write pgbench --unlogged-tables
> > > benchmark, I see that with HEAD Tps is 36241 and with increase the clog
> > > buffers patch, Tps is 69340 at 128 client count (very good performance
> > > boost) which indicates that we should go ahead with 1) and 2) patches.
> >
> > Especially considering the line count... I do wonder about going crazy
> > and increasing to 256 immediately. It otherwise seems likely that we'll
> > have the the same issue in a year.  Could you perhaps run your test
> > against that as well?
> >
> 
> Unfortunately, it dipped to 65005 with 256 clog bufs.  So I think 128 is
> appropriate number.

Ah, interesting. Then let's go with that.

Hi,

On 03/30/2016 07:09 PM, Andres Freund wrote:
> Yes. That looks good. My testing shows that increasing the number of
> buffers can increase both throughput and reduce latency variance. The
> former is a smaller effect with one of the discussed patches applied,
> the latter seems to actually increase in scale (with increased
> throughput).
>
>
> I've attached patches to:
> 0001: Increase the max number of clog buffers
> 0002: Implement 64bit atomics fallback and optimize read/write
> 0003: Edited version of Simon's clog scalability patch
>
> WRT 0003 - still clearly WIP - I've:
> - made group_lsn pg_atomic_u64*, to allow for tear-free reads
> - split content from IO lock
> - made SimpleLruReadPage_optShared always return with only share lock
>    held
> - Implement a different, experimental, concurrency model for
>    SetStatusBit using cmpxchg. A define USE_CONTENT_LOCK controls which
>    bit is used.
>
> I've tested this and saw this outperform Amit's approach. Especially so
> when using a read/write mix, rather then only reads. I saw over 30%
> increase on a large EC2 instance with -btpcb-like@1 -bselect-only@3. But
> that's in a virtualized environment, not very good for reproducability.
>
> Amit, could you run benchmarks on your bigger hardware? Both with
> USE_CONTENT_LOCK commented out and in?
>
> I think we should go for 1) and 2) unconditionally. And then evaluate
> whether to go with your, or 3) from above. If the latter, we've to do
> some cleanup :)
>

I have been testing Amit's patch in various setups and work loads, with 
up to 400 connections on a 2 x Xeon E5-2683 (28C/56T @ 2 GHz), not 
seeing an improvement, but no regression either.

Testing with 0001 and 0002 do show up to a 5% improvement when using a 
HDD for data + wal - about 1% when using 2 x RAID10 SSD - unlogged.

I can do a USE_CONTENT_LOCK run on 0003 if it is something for 9.6.

Thanks for your work on this !

Best regards, Jesper

On March 31, 2016 11:13:46 PM GMT+02:00, Jesper Pedersen <jesper.pedersen@redhat.com> wrote:

>I can do a USE_CONTENT_LOCK run on 0003 if it is something for 9.6.

Yes please. I think the lock variant is realistic, the lockless did isn't.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On 03/31/2016 06:21 PM, Andres Freund wrote:
> On March 31, 2016 11:13:46 PM GMT+02:00, Jesper Pedersen <jesper.pedersen@redhat.com> wrote:
>
>> I can do a USE_CONTENT_LOCK run on 0003 if it is something for 9.6.
>
> Yes please. I think the lock variant is realistic, the lockless did isn't.
>

I have done a run with -M prepared on unlogged running 10min per data 
point, up to 300 connections. Using data + wal on HDD.

I'm not seeing a difference between with and without USE_CONTENT_LOCK -- 
all points are within +/- 0.5%.

Let me know if there are other tests I can perform.

Best regards, Jesper

On April 1, 2016 10:25:51 PM GMT+02:00, Jesper Pedersen <jesper.pedersen@redhat.com> wrote:
>Hi,
>
>On 03/31/2016 06:21 PM, Andres Freund wrote:
>> On March 31, 2016 11:13:46 PM GMT+02:00, Jesper Pedersen
><jesper.pedersen@redhat.com> wrote:
>>
>>> I can do a USE_CONTENT_LOCK run on 0003 if it is something for 9.6.
>>
>> Yes please. I think the lock variant is realistic, the lockless did
>isn't.
>>
>
>I have done a run with -M prepared on unlogged running 10min per data 
>point, up to 300 connections. Using data + wal on HDD.
>
>I'm not seeing a difference between with and without USE_CONTENT_LOCK
>-- 
>all points are within +/- 0.5%.
>
>Let me know if there are other tests I can perform

How do either compare to just 0002 applied?

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

On 04/01/2016 04:39 PM, Andres Freund wrote:
> On April 1, 2016 10:25:51 PM GMT+02:00, Jesper Pedersen <jesper.pedersen@redhat.com> wrote:
>> Hi,
>>
>> On 03/31/2016 06:21 PM, Andres Freund wrote:
>>> On March 31, 2016 11:13:46 PM GMT+02:00, Jesper Pedersen
>> <jesper.pedersen@redhat.com> wrote:
>>>
>>>> I can do a USE_CONTENT_LOCK run on 0003 if it is something for 9.6.
>>>
>>> Yes please. I think the lock variant is realistic, the lockless did
>> isn't.
>>>
>>
>> I have done a run with -M prepared on unlogged running 10min per data
>> point, up to 300 connections. Using data + wal on HDD.
>>
>> I'm not seeing a difference between with and without USE_CONTENT_LOCK
>> --
>> all points are within +/- 0.5%.
>>
>> Let me know if there are other tests I can perform
>
> How do either compare to just 0002 applied?
>

0001 + 0002 compared to 0001 + 0002 + 0003 (either way) were pretty much 
the same +/- 0.5% on the HDD run.

Best regards, Jesper

Hi,

On 2016-04-07 09:14:00 +0530, Amit Kapila wrote:
> On Sat, Apr 2, 2016 at 5:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I have ran exactly same test on intel x86 m/c and the results are as below:

Thanks for running these tests!

> Client Count/Patch_ver (tps) 2 128 256
> HEAD – Commit 2143f5e1 2832 35001 26756
> clog_buf_128 2909 50685 40998
> clog_buf_128 +group_update_clog_v8 2981 53043 50779
> clog_buf_128 +content_lock 2843 56261 54059
> clog_buf_128 +nocontent_lock 2630 56554 54429

Interesting.

could you perhaps also run a test with -btpcb-like@1 -bselect-only@3?
That much represents real world loads, and it's where I saw simon's
approach outshining yours considerably...

Greetings,

Andres Freund

On 2016-04-07 18:40:14 +0530, Amit Kapila wrote:
> On Thu, Apr 7, 2016 at 10:16 AM, Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > On 2016-04-07 09:14:00 +0530, Amit Kapila wrote:
> > > On Sat, Apr 2, 2016 at 5:25 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > I have ran exactly same test on intel x86 m/c and the results are as
> below:
> >
> > Thanks for running these tests!
> >
> > > Client Count/Patch_ver (tps) 2 128 256
> > > HEAD – Commit 2143f5e1 2832 35001 26756
> > > clog_buf_128 2909 50685 40998
> > > clog_buf_128 +group_update_clog_v8 2981 53043 50779
> > > clog_buf_128 +content_lock 2843 56261 54059
> > > clog_buf_128 +nocontent_lock 2630 56554 54429
> >
> > Interesting.
> >
> > could you perhaps also run a test with -btpcb-like@1 -bselect-only@3?

> This is the data with -b tpcb-like@1 with 20-min run for each version and I
> could see almost similar results as the data posted in previous e-mail.
> 
> Client Count/Patch_ver (tps) 256
> clog_buf_128 40617
> clog_buf_128 +group_clog_v8 51137
> clog_buf_128 +content_lock 54188
> 
> For -b select-only@3,  I have done quicktest for each version and number is
> same 62K~63K for all version, why do you think this will improve
> select-only workload?

What I was looking for was pgbench with both -btpcb-like@1
-bselect-only@3 specified; i.e. a mixed read/write test. In my
measurement that's where Simon's approach shines (not surprising if you
look at the way it works), and it's of immense practical importance -
most workloads are mixed.

Regards,

Andres

On 2016-03-31 15:07:22 +0530, Amit Kapila wrote:
> I think we should change comments on top of this function.  I have changed
> the comments as per my previous patch and attached the modified patch with
> this mail, see if that makes sense.

I've applied this patch.

Regards,

Andres

On 2016-04-08 13:07:05 +0530, Amit Kapila wrote:
> I think by now, we have done many tests with both approaches and we find
> that in some cases, it is slightly better and in most cases it is neutral
> and in some cases it is worse than group clog approach.  I feel we should
> go with group clog approach now as that has been tested and reviewed
> multiple times and in future if we find that other approach is giving
> substantial gain, then we can anyway change it.

I think that's a discussion for the 9.7 cycle unfortunately. I've now
pushed the #clog-buffers patch; that's going to help the worst cases.

Greetings,

Andres Freund

Hi,

This thread started a year ago, different people contributed various
patches, some of which already got committed. Can someone please post a
summary of this thread, so that it's a bit more clear what needs
review/testing, what are the main open questions and so on?

I'm interested in doing some tests on the hardware I have available, but
I'm not willing spending my time untangling the discussion.

thanks

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

On Mon, Sep 5, 2016 at 3:18 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Hi,
>
> This thread started a year ago, different people contributed various
> patches, some of which already got committed. Can someone please post a
> summary of this thread, so that it's a bit more clear what needs
> review/testing, what are the main open questions and so on?
>

Okay, let me try to summarize this thread.  This thread started off to
ameliorate the CLOGControlLock contention with a patch to increase the
clog buffers to 128 (which got committed in 9.6).   Then the second
patch was developed to use Group mode to further reduce the
CLOGControlLock contention, latest version of which is upthread [1] (I
have checked that version still gets applied).  Then Andres suggested
to compare the Group lock mode approach with an alternative (more
granular) locking model approach for which he has posted patches
upthread [2].  There are three patches on that link, the patches of
interest are 0001-Improve-64bit-atomics-support and
0003-Use-a-much-more-granular-locking-model-for-the-clog-.  I have
checked that second one of those doesn't get applied, so I have
rebased it and attached it with this mail.  In the more granular
locking approach, actually, you can comment USE_CONTENT_LOCK to make
it use atomic operations (I could not compile it by disabling
USE_CONTENT_LOCK on my windows box, you can try by commenting that as
well, if it works for you).  So, in short we have to compare three
approaches here.

1) Group mode to reduce CLOGControlLock contention
2) Use granular locking model
3) Use atomic operations

For approach-1, you can use patch [1].  For approach-2, you can use
0001-Improve-64bit-atomics-support patch[2] and the patch attached
with this mail.  For approach-3, you can use
0001-Improve-64bit-atomics-support patch[2] and the patch attached
with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
work for you then for now we can compare approach-1 and approach-2.

I have done some testing of these patches for read-write pgbench
workload and doesn't find big difference.  Now the interesting test
case could be to use few sub-transactions (may be 4-8) for each
transaction as with that we can see more contention for
CLOGControlLock.

Few points to note for performance testing, one should use --unlogged
tables, else the WAL writing and WALWriteLock contention masks the
impact of this patch.  The impact of this patch is visible at
higher-client counts (say at 64~128).

> I'm interested in doing some tests on the hardware I have available, but
> I'm not willing spending my time untangling the discussion.
>

Thanks for showing the interest and let me know if something is still
un-clear or you need more information to proceed.


[1] - https://www.postgresql.org/message-id/CAA4eK1%2B8gQTyGSZLe1Rb7jeM1Beh4FqA4VNjtpZcmvwizDQ0hw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/20160330230914.GH13305%40awork2.anarazel.de

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

On Mon, Sep 5, 2016 at 2:00 PM, Pavan Deolasee <pavan.deolasee@gmail.com> wrote:
>
>
> On Mon, Sep 5, 2016 at 3:18 AM, Tomas Vondra <tomas.vondra@2ndquadrant.com>
> wrote:
>>
>> Hi,
>>
>> This thread started a year ago, different people contributed various
>> patches, some of which already got committed. Can someone please post a
>> summary of this thread, so that it's a bit more clear what needs
>> review/testing, what are the main open questions and so on?
>>
>> I'm interested in doing some tests on the hardware I have available, but
>> I'm not willing spending my time untangling the discussion.
>>
>
> I signed up for reviewing this patch. But as Amit explained later, there are
> two different and independent implementations to solve the problem. Since
> Tomas has volunteered to do some benchmarking, I guess I should wait for the
> results because that might influence which approach we choose.
>
> Does that sound correct?
>

Sounds correct to me.

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

On 09/05/2016 06:03 AM, Amit Kapila wrote:
> On Mon, Sep 5, 2016 at 3:18 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> Hi,
>>
>> This thread started a year ago, different people contributed various
>> patches, some of which already got committed. Can someone please post a
>> summary of this thread, so that it's a bit more clear what needs
>> review/testing, what are the main open questions and so on?
>>
> 
> Okay, let me try to summarize this thread.  This thread started off to
> ameliorate the CLOGControlLock contention with a patch to increase the
> clog buffers to 128 (which got committed in 9.6).   Then the second
> patch was developed to use Group mode to further reduce the
> CLOGControlLock contention, latest version of which is upthread [1] (I
> have checked that version still gets applied).  Then Andres suggested
> to compare the Group lock mode approach with an alternative (more
> granular) locking model approach for which he has posted patches
> upthread [2].  There are three patches on that link, the patches of
> interest are 0001-Improve-64bit-atomics-support and
> 0003-Use-a-much-more-granular-locking-model-for-the-clog-.  I have
> checked that second one of those doesn't get applied, so I have
> rebased it and attached it with this mail.  In the more granular
> locking approach, actually, you can comment USE_CONTENT_LOCK to make
> it use atomic operations (I could not compile it by disabling
> USE_CONTENT_LOCK on my windows box, you can try by commenting that as
> well, if it works for you).  So, in short we have to compare three
> approaches here.
> 
> 1) Group mode to reduce CLOGControlLock contention
> 2) Use granular locking model
> 3) Use atomic operations
> 
> For approach-1, you can use patch [1].  For approach-2, you can use
> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
> with this mail.  For approach-3, you can use
> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
> with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
> work for you then for now we can compare approach-1 and approach-2.
> 

OK, I can compile all three cases - but onl with gcc 4.7 or newer. Sadly
the 4-socket 64-core machine runs Debian Jessie with just gcc 4.6 and my
attempts to update to a newer version were unsuccessful so far.

> I have done some testing of these patches for read-write pgbench
> workload and doesn't find big difference.  Now the interesting test
> case could be to use few sub-transactions (may be 4-8) for each
> transaction as with that we can see more contention for
> CLOGControlLock.

Understood. So a bunch of inserts/updates interleaved by savepoints?

I presume you started looking into this based on a real-world
performance issue, right? Would that be a good test case?

> 
> Few points to note for performance testing, one should use --unlogged
> tables, else the WAL writing and WALWriteLock contention masks the
> impact of this patch.  The impact of this patch is visible at
> higher-client counts (say at 64~128).
> 

Even on good hardware (say, PCIe SSD storage that can do thousands of
fsyncs per second)? Does it then make sense to try optimizing this if
the effect can only be observed without the WAL overhead (so almost
never in practice)?

regards

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

On Mon, Sep 5, 2016 at 11:34 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
>
> On 09/05/2016 06:03 AM, Amit Kapila wrote:
>>  So, in short we have to compare three
>> approaches here.
>>
>> 1) Group mode to reduce CLOGControlLock contention
>> 2) Use granular locking model
>> 3) Use atomic operations
>>
>> For approach-1, you can use patch [1].  For approach-2, you can use
>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>> with this mail.  For approach-3, you can use
>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>> with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
>> work for you then for now we can compare approach-1 and approach-2.
>>
>
> OK, I can compile all three cases - but onl with gcc 4.7 or newer. Sadly
> the 4-socket 64-core machine runs Debian Jessie with just gcc 4.6 and my
> attempts to update to a newer version were unsuccessful so far.
>

So which all patches your are able to compile on 4-socket m/c?  I
think it is better to measure the performance on bigger machine.

>> I have done some testing of these patches for read-write pgbench
>> workload and doesn't find big difference.  Now the interesting test
>> case could be to use few sub-transactions (may be 4-8) for each
>> transaction as with that we can see more contention for
>> CLOGControlLock.
>
> Understood. So a bunch of inserts/updates interleaved by savepoints?
>

Yes.

> I presume you started looking into this based on a real-world
> performance issue, right? Would that be a good test case?
>

I had started looking into it based on LWLOCK_STATS data for
read-write workload (pgbench tpc-b).  I think it will depict many of
the real-world read-write workloads.

>>
>> Few points to note for performance testing, one should use --unlogged
>> tables, else the WAL writing and WALWriteLock contention masks the
>> impact of this patch.  The impact of this patch is visible at
>> higher-client counts (say at 64~128).
>>
>
> Even on good hardware (say, PCIe SSD storage that can do thousands of
> fsyncs per second)?

Not sure, because it could be masked by WALWriteLock contention.

> Does it then make sense to try optimizing this if
> the effect can only be observed without the WAL overhead (so almost
> never in practice)?
>

It is not that there is no improvement with WAL overhead (like one can
observe that via LWLOCK_STATS apart from TPS), but it is clearly
visible with unlogged tables.  The situation is not that simple,
because let us say we don't do anything for the remaining contention
for CLOGControlLock, then when we try to reduce the contention around
other locks like WALWriteLock or may be ProcArrayLock, there is a
chance that contention will shift to CLOGControlLock.  So, the basic
idea is to get the big benefits, we need to eliminate contention
around each of the locks.


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

On 09/06/2016 04:49 AM, Amit Kapila wrote:
> On Mon, Sep 5, 2016 at 11:34 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>>
>>
>> On 09/05/2016 06:03 AM, Amit Kapila wrote:
>>>  So, in short we have to compare three
>>> approaches here.
>>>
>>> 1) Group mode to reduce CLOGControlLock contention
>>> 2) Use granular locking model
>>> 3) Use atomic operations
>>>
>>> For approach-1, you can use patch [1].  For approach-2, you can use
>>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>>> with this mail.  For approach-3, you can use
>>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>>> with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
>>> work for you then for now we can compare approach-1 and approach-2.
>>>
>>
>> OK, I can compile all three cases - but onl with gcc 4.7 or newer. Sadly
>> the 4-socket 64-core machine runs Debian Jessie with just gcc 4.6 and my
>> attempts to update to a newer version were unsuccessful so far.
>>
> 
> So which all patches your are able to compile on 4-socket m/c?  I
> think it is better to measure the performance on bigger machine.

Oh, sorry - I forgot to mention that only the last test (with
USE_CONTENT_LOCK commented out) fails to compile, because the functions
for atomics were added in gcc-4.7.


regards

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

On Wed, Sep 7, 2016 at 1:08 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/06/2016 04:49 AM, Amit Kapila wrote:
>> On Mon, Sep 5, 2016 at 11:34 PM, Tomas Vondra
>> <tomas.vondra@2ndquadrant.com> wrote:
>>>
>>>
>>> On 09/05/2016 06:03 AM, Amit Kapila wrote:
>>>>  So, in short we have to compare three
>>>> approaches here.
>>>>
>>>> 1) Group mode to reduce CLOGControlLock contention
>>>> 2) Use granular locking model
>>>> 3) Use atomic operations
>>>>
>>>> For approach-1, you can use patch [1].  For approach-2, you can use
>>>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>>>> with this mail.  For approach-3, you can use
>>>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>>>> with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
>>>> work for you then for now we can compare approach-1 and approach-2.
>>>>
>>>
>>> OK, I can compile all three cases - but onl with gcc 4.7 or newer. Sadly
>>> the 4-socket 64-core machine runs Debian Jessie with just gcc 4.6 and my
>>> attempts to update to a newer version were unsuccessful so far.
>>>
>>
>> So which all patches your are able to compile on 4-socket m/c?  I
>> think it is better to measure the performance on bigger machine.
>
> Oh, sorry - I forgot to mention that only the last test (with
> USE_CONTENT_LOCK commented out) fails to compile, because the functions
> for atomics were added in gcc-4.7.
>

No issues, in that case we can leave the last test for now and do it later.

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

On 09/07/2016 01:13 PM, Amit Kapila wrote:
> On Wed, Sep 7, 2016 at 1:08 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> On 09/06/2016 04:49 AM, Amit Kapila wrote:
>>> On Mon, Sep 5, 2016 at 11:34 PM, Tomas Vondra
>>> <tomas.vondra@2ndquadrant.com> wrote:
>>>>
>>>>
>>>> On 09/05/2016 06:03 AM, Amit Kapila wrote:
>>>>>  So, in short we have to compare three
>>>>> approaches here.
>>>>>
>>>>> 1) Group mode to reduce CLOGControlLock contention
>>>>> 2) Use granular locking model
>>>>> 3) Use atomic operations
>>>>>
>>>>> For approach-1, you can use patch [1].  For approach-2, you can use
>>>>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>>>>> with this mail.  For approach-3, you can use
>>>>> 0001-Improve-64bit-atomics-support patch[2] and the patch attached
>>>>> with this mail by commenting USE_CONTENT_LOCK.  If the third doesn't
>>>>> work for you then for now we can compare approach-1 and approach-2.
>>>>>
>>>>
>>>> OK, I can compile all three cases - but onl with gcc 4.7 or newer. Sadly
>>>> the 4-socket 64-core machine runs Debian Jessie with just gcc 4.6 and my
>>>> attempts to update to a newer version were unsuccessful so far.
>>>>
>>>
>>> So which all patches your are able to compile on 4-socket m/c?  I
>>> think it is better to measure the performance on bigger machine.
>>
>> Oh, sorry - I forgot to mention that only the last test (with
>> USE_CONTENT_LOCK commented out) fails to compile, because the functions
>> for atomics were added in gcc-4.7.
>>
> 
> No issues, in that case we can leave the last test for now and do it later.
> 

FWIW I've managed to compile a new GCC on the system (all I had to do
was to actually read the damn manual), so I'm ready to do the test once
I get a bit of time.

regards

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

On Mon, Sep 5, 2016 at 9:33 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> USE_CONTENT_LOCK on my windows box, you can try by commenting that as
> well, if it works for you).  So, in short we have to compare three
> approaches here.
>
> 1) Group mode to reduce CLOGControlLock contention
> 2) Use granular locking model
> 3) Use atomic operations

I have tested performance with approach 1 and approach 2.

1. Transaction (script.sql): I have used below transaction to run my
bench mark, We can argue that this may not be an ideal workload, but I
tested this to put more load on ClogControlLock during commit
transaction.

-----------
\set aid random (1,30000000)
\set tid random (1,3000)

BEGIN;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
SAVEPOINT s1;
SELECT tbalance FROM pgbench_tellers WHERE tid = :tid for UPDATE;
SAVEPOINT s2;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
END;
-----------

2. Results
./pgbench -c $threads -j $threads -T 10 -M prepared postgres -f script.sql
scale factor: 300
Clients   head(tps)        grouplock(tps)          granular(tps)
-------      ---------               ----------                   -------
128        29367                 39326                    37421
180        29777                 37810                    36469
256        28523                 37418                    35882


grouplock --> 1) Group mode to reduce CLOGControlLock contention
granular  --> 2) Use granular locking model

I will test with 3rd approach also, whenever I get time.

3. Summary:
1. I can see on head we are gaining almost ~30 % performance at higher
client count (128 and beyond).
2. group lock is ~5% better compared to granular lock.

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

On Wed, Sep 14, 2016 at 10:25 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> I have tested performance with approach 1 and approach 2.
>
> 1. Transaction (script.sql): I have used below transaction to run my
> bench mark, We can argue that this may not be an ideal workload, but I
> tested this to put more load on ClogControlLock during commit
> transaction.
>
> -----------
> \set aid random (1,30000000)
> \set tid random (1,3000)
>
> BEGIN;
> SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
> SAVEPOINT s1;
> SELECT tbalance FROM pgbench_tellers WHERE tid = :tid for UPDATE;
> SAVEPOINT s2;
> SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
> END;
> -----------
>
> 2. Results
> ./pgbench -c $threads -j $threads -T 10 -M prepared postgres -f script.sql
> scale factor: 300
> Clients   head(tps)        grouplock(tps)          granular(tps)
> -------      ---------               ----------                   -------
> 128        29367                 39326                    37421
> 180        29777                 37810                    36469
> 256        28523                 37418                    35882
>
>
> grouplock --> 1) Group mode to reduce CLOGControlLock contention
> granular  --> 2) Use granular locking model
>
> I will test with 3rd approach also, whenever I get time.
>
> 3. Summary:
> 1. I can see on head we are gaining almost ~30 % performance at higher
> client count (128 and beyond).
> 2. group lock is ~5% better compared to granular lock.

Forgot to mention that, this test is on unlogged tables.


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

On Wed, Sep 14, 2016 at 12:55 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> 2. Results
> ./pgbench -c $threads -j $threads -T 10 -M prepared postgres -f script.sql
> scale factor: 300
> Clients   head(tps)        grouplock(tps)          granular(tps)
> -------      ---------               ----------                   -------
> 128        29367                 39326                    37421
> 180        29777                 37810                    36469
> 256        28523                 37418                    35882
>
>
> grouplock --> 1) Group mode to reduce CLOGControlLock contention
> granular  --> 2) Use granular locking model
>
> I will test with 3rd approach also, whenever I get time.
>
> 3. Summary:
> 1. I can see on head we are gaining almost ~30 % performance at higher
> client count (128 and beyond).
> 2. group lock is ~5% better compared to granular lock.

Sure, but you're testing at *really* high client counts here.  Almost
nobody is going to benefit from a 5% improvement at 256 clients.  You
need to test 64 clients and 32 clients and 16 clients and 8 clients
and see what happens there.  Those cases are a lot more likely than
these stratospheric client counts.

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

On Wed, Sep 14, 2016 at 8:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> Sure, but you're testing at *really* high client counts here.  Almost
> nobody is going to benefit from a 5% improvement at 256 clients.

I agree with your point, but here we need to consider one more thing,
that on head we are gaining ~30% with both the approaches.

So for comparing these two patches we can consider..

A.  Other workloads (one can be as below)  -> Load on CLogControlLock at commit (exclusive mode) + Load on
CLogControlLock at Transaction status (shared mode).  I think we can mix (savepoint + updates)

B. Simplicity of the patch (if both are performing almost equal in all
practical scenarios).

C. Bases on algorithm whichever seems winner.

I will try to test these patches with other workloads...

>  You
> need to test 64 clients and 32 clients and 16 clients and 8 clients
> and see what happens there.  Those cases are a lot more likely than
> these stratospheric client counts.

I tested with 64 clients as well..
1. On head we are gaining ~15% with both the patches.
2. But group lock vs granular lock is almost same.

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

On 09/14/2016 06:04 PM, Dilip Kumar wrote:
> On Wed, Sep 14, 2016 at 8:59 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Sure, but you're testing at *really* high client counts here.  Almost
>> nobody is going to benefit from a 5% improvement at 256 clients.
>
> I agree with your point, but here we need to consider one more thing,
> that on head we are gaining ~30% with both the approaches.
>
> So for comparing these two patches we can consider..
>
> A.  Other workloads (one can be as below)
>    -> Load on CLogControlLock at commit (exclusive mode) + Load on
> CLogControlLock at Transaction status (shared mode).
>    I think we can mix (savepoint + updates)
>
> B. Simplicity of the patch (if both are performing almost equal in all
> practical scenarios).
>
> C. Bases on algorithm whichever seems winner.
>
> I will try to test these patches with other workloads...
>
>>  You
>> need to test 64 clients and 32 clients and 16 clients and 8 clients
>> and see what happens there.  Those cases are a lot more likely than
>> these stratospheric client counts.
>
> I tested with 64 clients as well..
> 1. On head we are gaining ~15% with both the patches.
> 2. But group lock vs granular lock is almost same.
>

I've been doing some testing too, but I haven't managed to measure any
significant difference between master and any of the patches. Not sure
why, I've repeated the test from scratch to make sure I haven't done
anything stupid, but I got the same results (which is one of the main
reasons why the testing took me so long).

Attached is an archive with a script running the benchmark (including
SQL scripts generating the data and custom transaction for pgbench), and
results in a CSV format.

The benchmark is fairly simple - for each case (master + 3 different
patches) we do 10 runs, 5 minutes each, for 32, 64, 128 and 192 clients
(the machine has 32 physical cores).

The transaction is using a single unlogged table initialized like this:

     create unlogged table t(id int, val int);
     insert into t select i, i from generate_series(1,100000) s(i);
     vacuum t;
     create index on t(id);

(I've also ran it with 100M rows, called "large" in the results), and
pgbench is running this transaction:

     \set id random(1, 100000)

     BEGIN;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s1;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s2;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s3;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s4;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s5;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s6;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s7;
     UPDATE t SET val = val + 1 WHERE id = :id;
     SAVEPOINT s8;
     COMMIT;

So 8 simple UPDATEs interleaved by savepoints. The benchmark was running
on a machine with 256GB of RAM, 32 cores (4x E5-4620) and a fairly large
SSD array. I'd done some basic tuning on the system, most importantly:

     effective_io_concurrency = 32
     work_mem = 512MB
     maintenance_work_mem = 512MB
     max_connections = 300
     checkpoint_completion_target = 0.9
     checkpoint_timeout = 3600
     max_wal_size = 128GB
     min_wal_size = 16GB
     shared_buffers = 16GB

Although most of the changes probably does not matter much for unlogged
tables (I planned to see how this affects regular tables, but as I see
no difference for unlogged ones, I haven't done that yet).

So the question is why Dilip sees +30% improvement, while my results are
almost exactly the same. Looking at Dilip's benchmark, I see he only ran
the test for 10 seconds, and I'm not sure how many runs he did, warmup
etc. Dilip, can you provide additional info?

I'll ask someone else to redo the benchmark after the weekend to make
sure it's not actually some stupid mistake of mine.

regards

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

On Sat, Sep 17, 2016 at 6:54 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/14/2016 06:04 PM, Dilip Kumar wrote:
>>
>> On Wed, Sep 14, 2016 at 8:59 PM, Robert Haas <robertmhaas@gmail.com>
>> wrote:
>>>
>>> Sure, but you're testing at *really* high client counts here.  Almost
>>> nobody is going to benefit from a 5% improvement at 256 clients.
>>
>>
>> I agree with your point, but here we need to consider one more thing,
>> that on head we are gaining ~30% with both the approaches.
>>
>> So for comparing these two patches we can consider..
>>
>> A.  Other workloads (one can be as below)
>>    -> Load on CLogControlLock at commit (exclusive mode) + Load on
>> CLogControlLock at Transaction status (shared mode).
>>    I think we can mix (savepoint + updates)
>>
>> B. Simplicity of the patch (if both are performing almost equal in all
>> practical scenarios).
>>
>> C. Bases on algorithm whichever seems winner.
>>
>> I will try to test these patches with other workloads...
>>
>>>  You
>>> need to test 64 clients and 32 clients and 16 clients and 8 clients
>>> and see what happens there.  Those cases are a lot more likely than
>>> these stratospheric client counts.
>>
>>
>> I tested with 64 clients as well..
>> 1. On head we are gaining ~15% with both the patches.
>> 2. But group lock vs granular lock is almost same.
>>
>
>
> The transaction is using a single unlogged table initialized like this:
>
>     create unlogged table t(id int, val int);
>     insert into t select i, i from generate_series(1,100000) s(i);
>     vacuum t;
>     create index on t(id);
>
> (I've also ran it with 100M rows, called "large" in the results), and
> pgbench is running this transaction:
>
>     \set id random(1, 100000)
>
>     BEGIN;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s1;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s2;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s3;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s4;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s5;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s6;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s7;
>     UPDATE t SET val = val + 1 WHERE id = :id;
>     SAVEPOINT s8;
>     COMMIT;
>
> So 8 simple UPDATEs interleaved by savepoints.
>

The difference between these and tests performed by Dilip is that he
has lesser savepoints.  I think if you want to try it again, then can
you once do it with either no savepoint or 1~2 savepoints.  The other
thing you could try out is the same test as Dilip has done (with and
without 2 savepoints).

> The benchmark was running on
> a machine with 256GB of RAM, 32 cores (4x E5-4620) and a fairly large SSD
> array. I'd done some basic tuning on the system, most importantly:
>
>     effective_io_concurrency = 32
>     work_mem = 512MB
>     maintenance_work_mem = 512MB
>     max_connections = 300
>     checkpoint_completion_target = 0.9
>     checkpoint_timeout = 3600
>     max_wal_size = 128GB
>     min_wal_size = 16GB
>     shared_buffers = 16GB
>
> Although most of the changes probably does not matter much for unlogged
> tables (I planned to see how this affects regular tables, but as I see no
> difference for unlogged ones, I haven't done that yet).
>

You are right.  Unless, we don't see the benefit with unlogged tables,
there is no point in doing it for regular tables.

> So the question is why Dilip sees +30% improvement, while my results are
> almost exactly the same. Looking at Dilip's benchmark, I see he only ran the
> test for 10 seconds, and I'm not sure how many runs he did, warmup etc.
> Dilip, can you provide additional info?
>
> I'll ask someone else to redo the benchmark after the weekend to make sure
> it's not actually some stupid mistake of mine.
>

I think there is not much point in repeating the tests you have done,
rather it is better if we can try again the tests done by Dilip in
your environment to see the results.

Thanks for doing the tests.

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

On 09/17/2016 05:23 AM, Amit Kapila wrote:
> On Sat, Sep 17, 2016 at 6:54 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> On 09/14/2016 06:04 PM, Dilip Kumar wrote:
>>>
...
>>
>> (I've also ran it with 100M rows, called "large" in the results), and
>> pgbench is running this transaction:
>>
>>     \set id random(1, 100000)
>>
>>     BEGIN;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s1;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s2;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s3;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s4;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s5;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s6;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s7;
>>     UPDATE t SET val = val + 1 WHERE id = :id;
>>     SAVEPOINT s8;
>>     COMMIT;
>>
>> So 8 simple UPDATEs interleaved by savepoints.
>>
>
> The difference between these and tests performed by Dilip is that he
> has lesser savepoints.  I think if you want to try it again, then can
> you once do it with either no savepoint or 1~2 savepoints.  The other
> thing you could try out is the same test as Dilip has done (with and
> without 2 savepoints).
>

I don't follow. My understanding is the patches should make savepoints 
cheaper - so why would using fewer savepoints increase the effect of the 
patches?

FWIW I've already done a quick test with 2 savepoints, no difference. I 
can do a full test of course.

>> The benchmark was running on
>> a machine with 256GB of RAM, 32 cores (4x E5-4620) and a fairly large SSD
>> array. I'd done some basic tuning on the system, most importantly:
>>
>>     effective_io_concurrency = 32
>>     work_mem = 512MB
>>     maintenance_work_mem = 512MB
>>     max_connections = 300
>>     checkpoint_completion_target = 0.9
>>     checkpoint_timeout = 3600
>>     max_wal_size = 128GB
>>     min_wal_size = 16GB
>>     shared_buffers = 16GB
>>
>> Although most of the changes probably does not matter much for unlogged
>> tables (I planned to see how this affects regular tables, but as I see no
>> difference for unlogged ones, I haven't done that yet).
>>
>
> You are right.  Unless, we don't see the benefit with unlogged tables,
> there is no point in doing it for regular tables.
>
>> So the question is why Dilip sees +30% improvement, while my results are
>> almost exactly the same. Looking at Dilip's benchmark, I see he only ran the
>> test for 10 seconds, and I'm not sure how many runs he did, warmup etc.
>> Dilip, can you provide additional info?
>>
>> I'll ask someone else to redo the benchmark after the weekend to make sure
>> it's not actually some stupid mistake of mine.
>>
>
> I think there is not much point in repeating the tests you have
> done, rather it is better if we can try again the tests done by Dilip
> in your environment to see the results.
>

I'm OK with running Dilip's tests, but I'm not sure why there's not much 
point in running the tests I've done. Or perhaps I'd like to understand 
why "my tests" show no improvement whatsoever first - after all, they're 
not that different from Dilip's.

regards

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

On 09/14/2016 05:29 PM, Robert Haas wrote:
> On Wed, Sep 14, 2016 at 12:55 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> 2. Results
>> ./pgbench -c $threads -j $threads -T 10 -M prepared postgres -f script.sql
>> scale factor: 300
>> Clients   head(tps)        grouplock(tps)          granular(tps)
>> -------      ---------               ----------                   -------
>> 128        29367                 39326                    37421
>> 180        29777                 37810                    36469
>> 256        28523                 37418                    35882
>>
>>
>> grouplock --> 1) Group mode to reduce CLOGControlLock contention
>> granular  --> 2) Use granular locking model
>>
>> I will test with 3rd approach also, whenever I get time.
>>
>> 3. Summary:
>> 1. I can see on head we are gaining almost ~30 % performance at higher
>> client count (128 and beyond).
>> 2. group lock is ~5% better compared to granular lock.
>
> Sure, but you're testing at *really* high client counts here.  Almost
> nobody is going to benefit from a 5% improvement at 256 clients.  You
> need to test 64 clients and 32 clients and 16 clients and 8 clients
> and see what happens there.  Those cases are a lot more likely than
> these stratospheric client counts.
>

Right. My impression from the discussion so far is that the patches only 
improve performance with very many concurrent clients - but as Robert 
points out, almost no one is running with 256 active clients, unless 
they have 128 cores or so. At least not if they value latency more than 
throughput.

So while it's nice to improve throughput in those cases, it's a bit like 
a tree falling in the forest without anyone around.

regards

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

On Sat, Sep 17, 2016 at 9:12 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/17/2016 05:23 AM, Amit Kapila wrote:
>>
>> The difference between these and tests performed by Dilip is that he
>> has lesser savepoints.  I think if you want to try it again, then can
>> you once do it with either no savepoint or 1~2 savepoints.  The other
>> thing you could try out is the same test as Dilip has done (with and
>> without 2 savepoints).
>>
>
> I don't follow. My understanding is the patches should make savepoints
> cheaper - so why would using fewer savepoints increase the effect of the
> patches?
>

Oh, no the purpose of the patch is not to make savepoints cheaper (I
know I have earlier suggested to check by having few savepoints, but
that was not intended to mean that this patch makes savepoint cheaper,
rather it might show the difference between different approaches,
sorry if that was not clearly stated earlier).  The purpose of this
patch('es) is to make commits cheaper and in particular updating the
status in CLOG.  Let me try to explain in brief about the CLOG
contention and what these patches try to accomplish.  As of head, when
we try to update the status in CLOG (TransactionIdSetPageStatus), we
take CLOGControlLock in EXCLUSIVE mode for reading the appropriate
CLOG page (most of the time, it will be in memory, so it is cheap) and
then updating the transaction status in the same.  We take
CLOGControlLock in SHARED mode (if we the required clog page is in
memory, otherwise the lock is upgraded to Exclusive) while reading the
transaction status which happen when we access the tuple where hint
bit is not set.

So, we have two different type of contention around CLOGControlLock,
(a) all the transactions that try to commit at same time, each of them
have to do it almost serially (b) readers of transaction status
contend with writers.

Now with the patch that went in 9.6 (increasing the clog buffers), the
second type of contention is mostly reduced as most of the required
pages are in-memory and we are hoping that this patch will help in
reducing first type (a) of contention as well.

>>
>
> I'm OK with running Dilip's tests, but I'm not sure why there's not much
> point in running the tests I've done. Or perhaps I'd like to understand why
> "my tests" show no improvement whatsoever first - after all, they're not
> that different from Dilip's.
>

The test which Dilip is doing "Select ... For Update" is mainly aiming
at first type (a) of contention as it doesn't change the hint bits, so
mostly it should not go for reading the transaction status when
accessing the tuple.  Whereas, the tests you are doing is mainly
focussed on second type (b) of contention.

I think one point we have to keep in mind here is that this contention
is visible in bigger socket m/c, last time Jesper also tried these
patches, but didn't find much difference in his environment and on
further analyzing (IIRC) we found that the reason was that contention
was not visible in his environment.


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

On Sat, Sep 17, 2016 at 9:17 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/14/2016 05:29 PM, Robert Haas wrote:
>>
>> On Wed, Sep 14, 2016 at 12:55 AM, Dilip Kumar <dilipbalaut@gmail.com>
>> wrote:
>>>
>>> 2. Results
>>> ./pgbench -c $threads -j $threads -T 10 -M prepared postgres -f
>>> script.sql
>>> scale factor: 300
>>> Clients   head(tps)        grouplock(tps)          granular(tps)
>>> -------      ---------               ----------                   -------
>>> 128        29367                 39326                    37421
>>> 180        29777                 37810                    36469
>>> 256        28523                 37418                    35882
>>>
>>>
>>> grouplock --> 1) Group mode to reduce CLOGControlLock contention
>>> granular  --> 2) Use granular locking model
>>>
>>> I will test with 3rd approach also, whenever I get time.
>>>
>>> 3. Summary:
>>> 1. I can see on head we are gaining almost ~30 % performance at higher
>>> client count (128 and beyond).
>>> 2. group lock is ~5% better compared to granular lock.
>>
>>
>> Sure, but you're testing at *really* high client counts here.  Almost
>> nobody is going to benefit from a 5% improvement at 256 clients.  You
>> need to test 64 clients and 32 clients and 16 clients and 8 clients
>> and see what happens there.  Those cases are a lot more likely than
>> these stratospheric client counts.
>>
>
> Right. My impression from the discussion so far is that the patches only
> improve performance with very many concurrent clients - but as Robert points
> out, almost no one is running with 256 active clients, unless they have 128
> cores or so. At least not if they value latency more than throughput.
>

See, I am also not in favor of going with any of these patches, if
they doesn't help in reduction of contention.  However, I think it is
important to understand, under what kind of workload and which
environment it can show the benefit or regression whichever is
applicable.  Just FYI, couple of days back one of EDB's partner who
was doing the performance tests by using HammerDB (which is again OLTP
focussed workload) on 9.5 based code has found that CLogControlLock
has the significantly high contention.  They were using
synchronous_commit=off in their settings.  Now, it is quite possible
that with improvements done in 9.6, the contention they are seeing
will be eliminated, but we have yet to figure that out.  I just shared
this information to you with the intention that this seems to be a
real problem and we should try to work on it unless we are able to
convince ourselves that this is not a problem.

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

On Sat, Sep 17, 2016 at 6:54 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Although most of the changes probably does not matter much for unlogged
> tables (I planned to see how this affects regular tables, but as I see no
> difference for unlogged ones, I haven't done that yet).
>
> So the question is why Dilip sees +30% improvement, while my results are
> almost exactly the same. Looking at Dilip's benchmark, I see he only ran the
> test for 10 seconds, and I'm not sure how many runs he did, warmup etc.
> Dilip, can you provide additional info?

Actually I ran test for 10 minutes.

Sorry for the confusion (I copy paste my script and manually replaced
the variable and made mistake )

My script is like this

scale_factor=300
shared_bufs=8GB
time_for_reading=600

./postgres -c shared_buffers=8GB -c checkpoint_timeout=40min -c
max_wal_size=20GB -c max_connections=300 -c maintenance_work_mem=1GB&
./pgbench -i -s $scale_factor --unlogged-tables postgres
./pgbench -c $threads -j $threads -T $time_for_reading -M prepared
postgres -f ../../script.sql >> test_results.txt

I am taking median of three readings..

with below script, I can repeat my results every time (64 client 15%
gain on head and 128+ client 30% gain on head).

I will repeat my test with 8,16 and 32 client and post the results soon.

> \set aid random (1,30000000)
> \set tid random (1,3000)
>
> BEGIN;
> SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
> SAVEPOINT s1;
> SELECT tbalance FROM pgbench_tellers WHERE tid = :tid for UPDATE;
> SAVEPOINT s2;
> SELECT abalance FROM pgbench_accounts WHERE aid = :aid for UPDATE;
> END;
> -----------


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

On 09/17/2016 07:05 AM, Amit Kapila wrote:
> On Sat, Sep 17, 2016 at 9:17 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> On 09/14/2016 05:29 PM, Robert Haas wrote:
...
>>> Sure, but you're testing at *really* high client counts here.
>>> Almost nobody is going to benefit from a 5% improvement at 256
>>> clients. You need to test 64 clients and 32 clients and 16
>>> clients and 8 clients and see what happens there. Those cases are
>>> a lot more likely than these stratospheric client counts.
>>>
>>
>> Right. My impression from the discussion so far is that the patches
>> only improve performance with very many concurrent clients - but as
>> Robert points out, almost no one is running with 256 active
>> clients, unless they have 128 cores or so. At least not if they
>> value latency more than throughput.
>>
>
> See, I am also not in favor of going with any of these patches, if
> they doesn't help in reduction of contention. However, I think it is
> important to understand, under what kind of workload and which
> environment it can show the benefit or regression whichever is
> applicable.

Sure. Which is why I initially asked what type of workload should I be 
testing, and then done the testing with multiple savepoints as that's 
what you suggested. But apparently that's not a workload that could 
benefit from this patch, so I'm a bit confused.

> Just FYI, couple of days back one of EDB's partner who was doing the
> performance tests by using HammerDB (which is again OLTP focussed
> workload) on 9.5 based code has found that CLogControlLock has the
> significantly high contention. They were using synchronous_commit=off
> in their settings. Now, it is quite possible that with improvements
> done in 9.6, the contention they are seeing will be eliminated, but
> we have yet to figure that out. I just shared this information to you
> with the intention that this seems to be a real problem and we should
> try to work on it unless we are able to convince ourselves that this
> is not a problem.
>

So, can we approach the problem from this direction instead? That is, 
instead of looking for workloads that might benefit from the patches, 
look at real-world examples of CLOG lock contention and then evaluate 
the impact on those?

Extracting the workload from benchmarks probably is not ideal, but it's 
still better than constructing the workload on our own to fit the patch.

FWIW I'll do a simple pgbench test - first with synchronous_commit=on 
and then with synchronous_commit=off. Probably the workloads we should 
have started with anyway, I guess.

regards

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

On Sat, Sep 17, 2016 at 11:25 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/17/2016 07:05 AM, Amit Kapila wrote:
>>
>> On Sat, Sep 17, 2016 at 9:17 AM, Tomas Vondra
>> <tomas.vondra@2ndquadrant.com> wrote:
>>>
>>> On 09/14/2016 05:29 PM, Robert Haas wrote:
>
> ...
>>>>
>>>> Sure, but you're testing at *really* high client counts here.
>>>> Almost nobody is going to benefit from a 5% improvement at 256
>>>> clients. You need to test 64 clients and 32 clients and 16
>>>> clients and 8 clients and see what happens there. Those cases are
>>>> a lot more likely than these stratospheric client counts.
>>>>
>>>
>>> Right. My impression from the discussion so far is that the patches
>>> only improve performance with very many concurrent clients - but as
>>> Robert points out, almost no one is running with 256 active
>>> clients, unless they have 128 cores or so. At least not if they
>>> value latency more than throughput.
>>>
>>
>> See, I am also not in favor of going with any of these patches, if
>> they doesn't help in reduction of contention. However, I think it is
>> important to understand, under what kind of workload and which
>> environment it can show the benefit or regression whichever is
>> applicable.
>
>
> Sure. Which is why I initially asked what type of workload should I be
> testing, and then done the testing with multiple savepoints as that's what
> you suggested. But apparently that's not a workload that could benefit from
> this patch, so I'm a bit confused.
>
>> Just FYI, couple of days back one of EDB's partner who was doing the
>> performance tests by using HammerDB (which is again OLTP focussed
>> workload) on 9.5 based code has found that CLogControlLock has the
>> significantly high contention. They were using synchronous_commit=off
>> in their settings. Now, it is quite possible that with improvements
>> done in 9.6, the contention they are seeing will be eliminated, but
>> we have yet to figure that out. I just shared this information to you
>> with the intention that this seems to be a real problem and we should
>> try to work on it unless we are able to convince ourselves that this
>> is not a problem.
>>
>
> So, can we approach the problem from this direction instead? That is,
> instead of looking for workloads that might benefit from the patches, look
> at real-world examples of CLOG lock contention and then evaluate the impact
> on those?
>

Sure, we can go that way as well, but I thought instead of testing
with a new benchmark kit (HammerDB), it is better to first get with
some simple statements.

> Extracting the workload from benchmarks probably is not ideal, but it's
> still better than constructing the workload on our own to fit the patch.
>
> FWIW I'll do a simple pgbench test - first with synchronous_commit=on and
> then with synchronous_commit=off. Probably the workloads we should have
> started with anyway, I guess.
>

Here, synchronous_commit = off case could be interesting.  Do you see
any problem with first trying a workload where Dilip is seeing
benefit?  I am not suggesting we should not do any other testing, but
just first lets try to reproduce the performance gain which is seen in
Dilip's tests.


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

On 09/18/2016 06:08 AM, Amit Kapila wrote:
> On Sat, Sep 17, 2016 at 11:25 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> On 09/17/2016 07:05 AM, Amit Kapila wrote:
>>>
>>> On Sat, Sep 17, 2016 at 9:17 AM, Tomas Vondra
>>> <tomas.vondra@2ndquadrant.com> wrote:
>>>>
>>>> On 09/14/2016 05:29 PM, Robert Haas wrote:
>>
>> ...
>>>>>
>>>>> Sure, but you're testing at *really* high client counts here.
>>>>> Almost nobody is going to benefit from a 5% improvement at 256
>>>>> clients. You need to test 64 clients and 32 clients and 16
>>>>> clients and 8 clients and see what happens there. Those cases are
>>>>> a lot more likely than these stratospheric client counts.
>>>>>
>>>>
>>>> Right. My impression from the discussion so far is that the patches
>>>> only improve performance with very many concurrent clients - but as
>>>> Robert points out, almost no one is running with 256 active
>>>> clients, unless they have 128 cores or so. At least not if they
>>>> value latency more than throughput.
>>>>
>>>
>>> See, I am also not in favor of going with any of these patches, if
>>> they doesn't help in reduction of contention. However, I think it is
>>> important to understand, under what kind of workload and which
>>> environment it can show the benefit or regression whichever is
>>> applicable.
>>
>>
>> Sure. Which is why I initially asked what type of workload should I be
>> testing, and then done the testing with multiple savepoints as that's what
>> you suggested. But apparently that's not a workload that could benefit from
>> this patch, so I'm a bit confused.
>>
>>> Just FYI, couple of days back one of EDB's partner who was doing the
>>> performance tests by using HammerDB (which is again OLTP focussed
>>> workload) on 9.5 based code has found that CLogControlLock has the
>>> significantly high contention. They were using synchronous_commit=off
>>> in their settings. Now, it is quite possible that with improvements
>>> done in 9.6, the contention they are seeing will be eliminated, but
>>> we have yet to figure that out. I just shared this information to you
>>> with the intention that this seems to be a real problem and we should
>>> try to work on it unless we are able to convince ourselves that this
>>> is not a problem.
>>>
>>
>> So, can we approach the problem from this direction instead? That is,
>> instead of looking for workloads that might benefit from the patches, look
>> at real-world examples of CLOG lock contention and then evaluate the impact
>> on those?
>>
>
> Sure, we can go that way as well, but I thought instead of testing
> with a new benchmark kit (HammerDB), it is better to first get with
> some simple statements.
>

IMHO in the ideal case the first message in this thread would provide a 
test case, demonstrating the effect of the patch. Then we wouldn't have 
the issue of looking for a good workload two years later.

But now that I look at the first post, I see it apparently used a plain 
tpc-b pgbench (with synchronous_commit=on) to show the benefits, which 
is the workload I'm running right now (results sometime tomorrow).

That workload clearly uses no savepoints at all, so I'm wondering why 
you suggested to use several of them - I know you said that it's to show 
differences between the approaches, but why should that matter to any of 
the patches (and if it matters, why I got almost no differences in the 
benchmarks)?

Pardon my ignorance, CLOG is not my area of expertise ...

>> Extracting the workload from benchmarks probably is not ideal, but
>> it's still better than constructing the workload on our own to fit
>> the patch.
>>
>> FWIW I'll do a simple pgbench test - first with
>> synchronous_commit=on and then with synchronous_commit=off.
>> Probably the workloads we should have started with anyway, I
>> guess.
>>
>
> Here, synchronous_commit = off case could be interesting. Do you see
> any problem with first trying a workload where Dilip is seeing
> benefit? I am not suggesting we should not do any other testing, but
> just first lets try to reproduce the performance gain which is seen
> in Dilip's tests.
>

I plan to run Dilip's workload once the current benchmarks complete.


regard

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

On Mon, Sep 19, 2016 at 2:41 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> But now that I look at the first post, I see it apparently used a plain
> tpc-b pgbench (with synchronous_commit=on) to show the benefits, which is
> the workload I'm running right now (results sometime tomorrow).

Good option, We can test plain TPC-B also..

I have some more results.. I have got the result for "Update with no
savepoint"....

below is my script...

\set aid random (1,30000000)
\set tid random (1,3000)
\set delta random(-5000, 5000)
BEGIN;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;


Results: (median of three, 10 minutes run).

Clients   Head     GroupLock
16          21452    21589
32          42422    42688
64          42460    52590           ~ 23%
128        22683    56825           ~150%
256       18748     54867

With this workload I observed that gain is bigger than my previous
workload (select for update with 2 SP)..

Just to confirm that the gain what we are seeing is because of Clog
Lock contention removal or it's
something else, I ran 128 client with perf for 5 minutes and below is my result.

I can see that after applying group lock patch, LWLockAcquire become
28% to just 4%, and all because
of Clog Lock.

On Head:
------------
-   28.45%     0.24%  postgres  postgres           [.] LWLockAcquire  - LWLockAcquire     + 53.49%
TransactionIdSetPageStatus    + 40.83% SimpleLruReadPage_ReadOnly     + 1.16% BufferAlloc     + 0.92% GetSnapshotData
 + 0.89% GetNewTransactionId     + 0.72% LockBuffer     + 0.70% ProcArrayGroupClearXid
 


After Group Lock Patch:
-------------------------------
-    4.47%     0.26%  postgres  postgres           [.] LWLockAcquire  - LWLockAcquire     + 27.11% GetSnapshotData
+21.57% GetNewTransactionId     + 11.44% SimpleLruReadPage_ReadOnly     + 10.13% BufferAlloc     + 7.24%
ProcArrayGroupClearXid    + 4.74% LockBuffer     + 4.08% LockAcquireExtended     + 2.91%
TransactionGroupUpdateXidStatus    + 2.71% LockReleaseAll     + 1.90% WALInsertLockAcquire     + 0.94% LockRelease
+0.91% VirtualXactLockTableInsert     + 0.90% VirtualXactLockTableCleanup     + 0.72% MultiXactIdSetOldestMember     +
0.66%LockRefindAndRelease
 

Next I will test, "update with 2 savepoints", "select for update with
no savepoints"....
I will also test the granular lock and atomic lock patch in next run..

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

On Sun, Sep 18, 2016 at 5:11 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> IMHO in the ideal case the first message in this thread would provide a test
> case, demonstrating the effect of the patch. Then we wouldn't have the issue
> of looking for a good workload two years later.
>
> But now that I look at the first post, I see it apparently used a plain
> tpc-b pgbench (with synchronous_commit=on) to show the benefits, which is
> the workload I'm running right now (results sometime tomorrow).
>
> That workload clearly uses no savepoints at all, so I'm wondering why you
> suggested to use several of them - I know you said that it's to show
> differences between the approaches, but why should that matter to any of the
> patches (and if it matters, why I got almost no differences in the
> benchmarks)?
>
> Pardon my ignorance, CLOG is not my area of expertise ...

It's possible that the effect of this patch depends on the number of
sockets.  EDB test machine cthulhu as 8 sockets, and power2 has 4
sockets.  I assume Dilip's tests were run on one of those two,
although he doesn't seem to have mentioned which one.  Your system is
probably 2 or 4 sockets, which might make a difference.  Results might
also depend on CPU architecture; power2 is, unsurprisingly, a POWER
system, whereas I assume you are testing x86.  Maybe somebody who has
access should test on hydra.pg.osuosl.org, which is a community POWER
resource.  (Send me a private email if you are a known community
member who wants access for benchmarking purposes.)

Personally, I find the results so far posted on this thread thoroughly
unimpressive.  I acknowledge that Dilip's results appear to show that
in a best-case scenario these patches produce a rather large gain.
However, that gain seems to happen in a completely contrived scenario:
astronomical client counts, unlogged tables, and a test script that
maximizes pressure on CLogControlLock.  If you have to work that hard
to find a big win, and tests under more reasonable conditions show no
benefit, it's not clear to me that it's really worth the time we're
all spending benchmarking and reviewing this, or the risk of bugs, or
the damage to the SLRU abstraction layer.  I think there's a very good
chance that we're better off moving on to projects that have a better
chance of helping in the real world.

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

On 2016-09-19 15:10:58 -0400, Robert Haas wrote:
> Personally, I find the results so far posted on this thread thoroughly
> unimpressive.  I acknowledge that Dilip's results appear to show that
> in a best-case scenario these patches produce a rather large gain.
> However, that gain seems to happen in a completely contrived scenario:
> astronomical client counts, unlogged tables, and a test script that
> maximizes pressure on CLogControlLock.  If you have to work that hard
> to find a big win, and tests under more reasonable conditions show no
> benefit, it's not clear to me that it's really worth the time we're
> all spending benchmarking and reviewing this, or the risk of bugs, or
> the damage to the SLRU abstraction layer.  I think there's a very good
> chance that we're better off moving on to projects that have a better
> chance of helping in the real world.

+1

On Tue, Sep 20, 2016 at 12:40 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sun, Sep 18, 2016 at 5:11 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> IMHO in the ideal case the first message in this thread would provide a test
>> case, demonstrating the effect of the patch. Then we wouldn't have the issue
>> of looking for a good workload two years later.
>>
>> But now that I look at the first post, I see it apparently used a plain
>> tpc-b pgbench (with synchronous_commit=on) to show the benefits, which is
>> the workload I'm running right now (results sometime tomorrow).
>>
>> That workload clearly uses no savepoints at all, so I'm wondering why you
>> suggested to use several of them - I know you said that it's to show
>> differences between the approaches, but why should that matter to any of the
>> patches (and if it matters, why I got almost no differences in the
>> benchmarks)?
>>
>> Pardon my ignorance, CLOG is not my area of expertise ...
>
> It's possible that the effect of this patch depends on the number of
> sockets.  EDB test machine cthulhu as 8 sockets, and power2 has 4
> sockets.  I assume Dilip's tests were run on one of those two,
>

I think it is former (8 socket machine).

> although he doesn't seem to have mentioned which one.  Your system is
> probably 2 or 4 sockets, which might make a difference.  Results might
> also depend on CPU architecture; power2 is, unsurprisingly, a POWER
> system, whereas I assume you are testing x86.  Maybe somebody who has
> access should test on hydra.pg.osuosl.org, which is a community POWER
> resource.  (Send me a private email if you are a known community
> member who wants access for benchmarking purposes.)
>
> Personally, I find the results so far posted on this thread thoroughly
> unimpressive.  I acknowledge that Dilip's results appear to show that
> in a best-case scenario these patches produce a rather large gain.
> However, that gain seems to happen in a completely contrived scenario:
> astronomical client counts, unlogged tables, and a test script that
> maximizes pressure on CLogControlLock.
>

You are right that the scenario is somewhat contrived, but I think he
hasn't posted the results for simple-update or tpc-b kind of scenarios
for pgbench, so we can't conclude that those won't show benefit.  I
think we can see benefits with synchronous_commit=off as well may not
be as good as with unlogged tables.  The other thing to keep in mind
is that reducing contention on one lock (assume in this case
CLOGControlLock) also gives benefits when we reduce contention on
other locks (like ProcArrayLock, WALWriteLock, ..).  Last time we have
verified this effect with Andres's patch (cache the snapshot) which
reduces the remaining contention on ProcArrayLock.  We have seen that
individually that patch gives some benefit, but by removing the
contention on CLOGControlLock with the patches (increase the clog
buffers and grouping stuff, each one helps) discussed in this thread,
it gives much bigger benefit.

You point related to high-client count is valid and I am sure it won't
give noticeable benefit at lower client-count as the the
CLOGControlLock contention starts impacting only at high-client count.
I am not sure if it is good idea to reject a patch which helps in
stabilising the performance (helps in falling off the cliff) when the
processes increases the number of cores (or hardware threads)

>  If you have to work that hard
> to find a big win, and tests under more reasonable conditions show no
> benefit, it's not clear to me that it's really worth the time we're
> all spending benchmarking and reviewing this, or the risk of bugs, or
> the damage to the SLRU abstraction layer.

I agree with you unless it shows benefit on somewhat more usual
scenario's, we should not accept it.  So shouldn't we wait for results
of other workloads like simple-update or tpc-b on bigger machines
before reaching to conclusion?


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

On Tue, Sep 20, 2016 at 8:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> I think it is former (8 socket machine).

I confirm this is 8 sockets machine(cthulhu)
>

>
> You point related to high-client count is valid and I am sure it won't
> give noticeable benefit at lower client-count as the the
> CLOGControlLock contention starts impacting only at high-client count.
> I am not sure if it is good idea to reject a patch which helps in
> stabilising the performance (helps in falling off the cliff) when the
> processes increases the number of cores (or hardware threads)
>
>>  If you have to work that hard
>> to find a big win, and tests under more reasonable conditions show no
>> benefit, it's not clear to me that it's really worth the time we're
>> all spending benchmarking and reviewing this, or the risk of bugs, or
>> the damage to the SLRU abstraction layer.
>
> I agree with you unless it shows benefit on somewhat more usual
> scenario's, we should not accept it.  So shouldn't we wait for results
> of other workloads like simple-update or tpc-b on bigger machines
> before reaching to conclusion?

+1

My test are under run, I will post it soon..


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

Hi,

On 09/19/2016 09:10 PM, Robert Haas wrote:
 >
> It's possible that the effect of this patch depends on the number of
> sockets. EDB test machine cthulhu as 8 sockets, and power2 has 4
> sockets. I assume Dilip's tests were run on one of those two,
> although he doesn't seem to have mentioned which one. Your system is
> probably 2 or 4 sockets, which might make a difference. Results
> might also depend on CPU architecture; power2 is, unsurprisingly, a
> POWER system, whereas I assume you are testing x86. Maybe somebody
> who has access should test on hydra.pg.osuosl.org, which is a
> community POWER resource. (Send me a private email if you are a known
> community member who wants access for benchmarking purposes.)
>

Yes, I'm using x86 machines:

1) large but slightly old
- 4 sockets, e5-4620 (so a bit old CPU, 32 cores in total)
- kernel 3.2.80

2) smaller but fresh
- 2 sockets, e5-2620 v4 (newest type of Xeons, 16 cores in total)
- kernel 4.8.0

> Personally, I find the results so far posted on this thread
> thoroughly unimpressive. I acknowledge that Dilip's results appear
> to show that in a best-case scenario these patches produce a rather
> large gain. However, that gain seems to happen in a completely
> contrived scenario: astronomical client counts, unlogged tables, and
> a test script that maximizes pressure on CLogControlLock. If you
> have to work that hard to find a big win, and tests under more
> reasonable conditions show no benefit, it's not clear to me that it's
> really worth the time we're all spending benchmarking and reviewing
> this, or the risk of bugs, or the damage to the SLRU abstraction
> layer. I think there's a very good chance that we're better off
> moving on to projects that have a better chance of helping in the
> real world.

I'm posting results from two types of workloads - traditional r/w
pgbench and Dilip's transaction. With synchronous_commit on/off.

Full results (including script driving the benchmark) are available
here, if needed:

     https://bitbucket.org/tvondra/group-clog-benchmark/src

It'd be good if someone could try reproduce this on a comparable
machine, to rule out my stupidity.


2 x e5-2620 v4 (16 cores, 32 with HT)
=====================================

On the "smaller" machine the results look like this - I have only tested
up to 64 clients, as higher values seem rather uninteresting on a
machine with only 16 physical cores.

These are averages of 5 runs, where the min/max for each group are
within ~5% in most cases (see the "spread" sheet). The "e5-2620" sheet
also shows the numbers as % compared to master.


  dilip / sync=off      1        4        8       16       32       64
----------------------------------------------------------------------
  master             4756    17672    35542    57303    74596    82138
  granular-locking   4745    17728    35078    56105    72983    77858
  no-content-lock    4646    17650    34887    55794    73273    79000
  group-update       4582    17757    35383    56974    74387    81794

  dilip / sync=on       1        4        8       16       32       64
----------------------------------------------------------------------
  master             4819    17583    35636    57437    74620    82036
  granular-locking   4568    17816    35122    56168    73192    78462
  no-content-lock    4540    17662    34747    55560    73508    79320
  group-update       4495    17612    35474    57095    74409    81874

  pgbench / sync=off    1        4        8       16       32       64
----------------------------------------------------------------------
  master             3791    14368    27806    43369    54472    62956
  granular-locking   3822    14462    27597    43173    56391    64669
  no-content-lock    3725    14212    27471    43041    55431    63589
  group-update       3895    14453    27574    43405    56783    62406

  pgbench / sync=on     1        4        8       16       32       64
----------------------------------------------------------------------
  master             3907    14289    27802    43717    56902    62916
  granular-locking   3770    14503    27636    44107    55205    63903
  no-content-lock    3772    14111    27388    43054    56424    64386
  group-update       3844    14334    27452    43621    55896    62498

There's pretty much no improvement at all - most of the results are
within 1-2% of master, in both directions. Hardly a win.

Actually, with 1 client there seems to be ~5% regression, but it might
also be noise and verifying it would require further testing.


4 x e5-4620 v1 (32 cores, 64 with HT)
=====================================

These are averages of 10 runs, and there are a few strange things here.

Firstly, for Dilip's workload the results get much (much) worse between
64 and 128 clients, for some reason. I suspect this might be due to
fairly old kernel (3.2.80), so I'll reboot the machine with 4.5.x kernel
and try again.

Secondly, the min/max differences get much larger than the ~5% on the
smaller machine - with 128 clients, the (max-min)/average is often
 >100%. See the "spread" or "spread2" sheets in the attached file.

But for some reason this only affects Dilip's workload, and apparently
the patches make it measurably worse (master is ~75%, patches ~120%). If
you look at tps for individual runs, there's usually 9 runs with almost
the same performance, and then one or two much faster runs. Again, the
pgbench seems not to have this issue.

I have no idea what's causing this - it might be related to the kernel,
but I'm not sure why it should affect the patches differently. Let's see
how the new kernel affects this.

  dilip / sync=off       16       32       64      128     192
--------------------------------------------------------------
  master              26198    37901    37211    14441    8315
  granular-locking    25829    38395    40626    14299    8160
  no-content-lock     25872    38994    41053    14058    8169
  group-update        26503    38911    42993    19474    8325

  dilip / sync=on        16       32       64      128     192
--------------------------------------------------------------
  master              26138    37790    38492    13653    8337
  granular-locking    25661    38586    40692    14535    8311
  no-content-lock     25653    39059    41169    14370    8373
  group-update        26472    39170    42126    18923    8366

  pgbench / sync=off     16       32       64      128     192
--------------------------------------------------------------
  master              23001    35762    41202    31789    8005
  granular-locking    23218    36130    42535    45850    8701
  no-content-lock     23322    36553    42772    47394    8204
  group-update        23129    36177    41788    46419    8163

  pgbench / sync=on      16       32       64      128     192
--------------------------------------------------------------
  master              22904    36077    41295    35574    8297
  granular-locking    23323    36254    42446    43909    8959
  no-content-lock     23304    36670    42606    48440    8813
  group-update        23127    36696    41859    46693    8345


So there is some improvement due to the patches for 128 clients (+30% in
some cases), but it's rather useless as 64 clients either give you
comparable performance (pgbench workload) or way better one (Dilip's
workload).

Also, pretty much no difference between synchronous_commit on/off,
probably thanks to running on unlogged tables.

I'll repeat the test on the 4-socket machine with a newer kernel, but
that's probably the last benchmark I'll do for this patch for now. I
agree with Robert that the cases the patch is supposed to improve are a
bit contrived because of the very high client counts.

IMHO to continue with the patch (or even with testing it), we really
need a credible / practical example of a real-world workload that
benefits from the patches. The closest we have to that is Amit's
suggestion someone hit the commit lock when running HammerDB, but we
have absolutely no idea what parameters they were using, except that
they were running with synchronous_commit=off. Pgbench shows no such
improvements (at least for me), at least with reasonable parameters.


regards

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

On Tue, Sep 20, 2016 at 9:15 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> +1
>
> My test are under run, I will post it soon..

I have some more results now....

8 socket machine
10 min run(median of 3 run)
synchronous_commit=off
scal factor = 300
share buffer= 8GB

test1: Simple update(pgbench)

Clients         Head          GroupLock32               45702           4540264               46974           51627
128              35056           55362


test2: TPCB (pgbench)

Clients         Head           GroupLock32               27969           2776564               33140           34786
128              21555           38848

Summary:
--------------
At 32 clients no gain, I think at this workload Clog Lock is not a problem.
At 64 Clients we can see ~10% gain with simple update and ~5% with TPCB.
At 128 Clients we can see > 50% gain.

Currently I have tested with synchronous commit=off, later I can try
with on. I can also test at 80 client, I think we will see some
significant gain at this client count also, but as of now I haven't
yet tested.

With above results, what we think ? should we continue our testing ?

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

On Wed, Sep 21, 2016 at 3:48 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> I have no idea what's causing this - it might be related to the kernel, but
> I'm not sure why it should affect the patches differently. Let's see how the
> new kernel affects this.
>
>  dilip / sync=off       16       32       64      128     192
> --------------------------------------------------------------
>  master              26198    37901    37211    14441    8315
>  granular-locking    25829    38395    40626    14299    8160
>  no-content-lock     25872    38994    41053    14058    8169
>  group-update        26503    38911    42993    19474    8325
>
>  dilip / sync=on        16       32       64      128     192
> --------------------------------------------------------------
>  master              26138    37790    38492    13653    8337
>  granular-locking    25661    38586    40692    14535    8311
>  no-content-lock     25653    39059    41169    14370    8373
>  group-update        26472    39170    42126    18923    8366
>
>  pgbench / sync=off     16       32       64      128     192
> --------------------------------------------------------------
>  master              23001    35762    41202    31789    8005
>  granular-locking    23218    36130    42535    45850    8701
>  no-content-lock     23322    36553    42772    47394    8204
>  group-update        23129    36177    41788    46419    8163
>
>  pgbench / sync=on      16       32       64      128     192
> --------------------------------------------------------------
>  master              22904    36077    41295    35574    8297
>  granular-locking    23323    36254    42446    43909    8959
>  no-content-lock     23304    36670    42606    48440    8813
>  group-update        23127    36696    41859    46693    8345
>
>
> So there is some improvement due to the patches for 128 clients (+30% in
> some cases), but it's rather useless as 64 clients either give you
> comparable performance (pgbench workload) or way better one (Dilip's
> workload).
>

I think these results are somewhat similar to what Dilip has reported.
Here, if you see in both cases, the performance improvement is seen
when the client count is greater than cores (including HT).  As far as
I know the m/c on which Dilip is running the tests also has 64 HT.
The point here is that the CLOGControlLock contention is noticeable
only at that client count, so it is not fault of patch that it is not
improving at lower client-count.  I guess that we will see performance
improvement between 64~128 client counts as well.


> Also, pretty much no difference between synchronous_commit on/off, probably
> thanks to running on unlogged tables.
>

Yeah.

> I'll repeat the test on the 4-socket machine with a newer kernel, but that's
> probably the last benchmark I'll do for this patch for now.
>

Okay, but I think it is better to see the results between 64~128
client count and may be greater than128 client counts, because it is
clear that patch won't improve performance below that.

> I agree with
> Robert that the cases the patch is supposed to improve are a bit contrived
> because of the very high client counts.
>

No issues, I have already explained why I think it is important to
reduce the remaining CLOGControlLock contention in yesterday's and
this mail.  If none of you is convinced, then I think we have no
choice but to drop this patch.


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

On 09/21/2016 08:04 AM, Amit Kapila wrote:
> On Wed, Sep 21, 2016 at 3:48 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
...
>
>> I'll repeat the test on the 4-socket machine with a newer kernel,
>> but that's probably the last benchmark I'll do for this patch for
>> now.
>>

Attached are results from benchmarks running on kernel 4.5 (instead of
the old 3.2.80). I've only done synchronous_commit=on, and I've added a
few client counts (mostly at the lower end). The data are pushed the
data to the git repository, see

     git push --set-upstream origin master

The summary looks like this (showing both the 3.2.80 and 4.5.5 results):

1) Dilip's workload

  3.2.80                             16     32     64    128    192
-------------------------------------------------------------------
  master                          26138  37790  38492  13653   8337
  granular-locking                25661  38586  40692  14535   8311
  no-content-lock                 25653  39059  41169  14370   8373
  group-update                    26472  39170  42126  18923   8366

  4.5.5                 1      8     16     32     64    128    192
-------------------------------------------------------------------
  granular-locking   4050  23048  27969  32076  34874  36555  37710
  no-content-lock    4025  23166  28430  33032  35214  37576  39191
  group-update       4002  23037  28008  32492  35161  36836  38850
  master             3968  22883  27437  32217  34823  36668  38073


2) pgbench

  3.2.80                             16     32     64    128    192
-------------------------------------------------------------------
  master                          22904  36077  41295  35574   8297
  granular-locking                23323  36254  42446  43909   8959
  no-content-lock                 23304  36670  42606  48440   8813
  group-update                    23127  36696  41859  46693   8345

  4.5.5                 1      8     16     32     64    128    192
-------------------------------------------------------------------
  granular-locking   3116  19235  27388  29150  31905  34105  36359
  no-content-lock    3206  19071  27492  29178  32009  34140  36321
  group-update       3195  19104  26888  29236  32140  33953  35901
  master             3136  18650  26249  28731  31515  33328  35243


The 4.5 kernel clearly changed the results significantly:

(a) Compared to the results from 3.2.80 kernel, some numbers improved,
some got worse. For example, on 3.2.80 pgbench did ~23k tps with 16
clients, on 4.5.5 it does 27k tps. With 64 clients the performance
dropped from 41k tps to ~34k (on master).

(b) The drop above 64 clients is gone - on 3.2.80 it dropped very
quickly to only ~8k with 192 clients. On 4.5 the tps actually continues
to increase, and we get ~35k with 192 clients.

(c) Although it's not visible in the results, 4.5.5 almost perfectly
eliminated the fluctuations in the results. For example when 3.2.80
produced this results (10 runs with the same parameters):

     12118 11610 27939 11771 18065
     12152 14375 10983 13614 11077

we get this on 4.5.5

     37354 37650 37371 37190 37233
     38498 37166 36862 37928 38509

Notice how much more even the 4.5.5 results are, compared to 3.2.80.

(d) There's no sign of any benefit from any of the patches (it was only
helpful >= 128 clients, but that's where the tps actually dropped on
3.2.80 - apparently 4.5.5 fixes that and the benefit is gone).

It's a bit annoying that after upgrading from 3.2.80 to 4.5.5, the
performance with 32 and 64 clients dropped quite noticeably (by more
than 10%). I believe that might be a kernel regression, but perhaps it's
a price for improved scalability for higher client counts.

It of course begs the question what kernel version is running on the
machine used by Dilip (i.e. cthulhu)? Although it's a Power machine, so
I'm not sure how much the kernel matters on it.

I'll ask someone else with access to this particular machine to repeat
the tests, as I have a nagging suspicion that I've missed something
important when compiling / running the benchmarks. I'll also retry the
benchmarks on 3.2.80 to see if I get the same numbers.

>
> Okay, but I think it is better to see the results between 64~128
> client count and may be greater than128 client counts, because it is
> clear that patch won't improve performance below that.
>

There are results for 64, 128 and 192 clients. Why should we care about
numbers in between? How likely (and useful) would it be to get
improvement with 96 clients, but no improvement for 64 or 128 clients?

 >>
>> I agree with Robert that the cases the patch is supposed to
>> improve are a bit contrived because of the very high client
>> counts.
>>
>
> No issues, I have already explained why I think it is important to
> reduce the remaining CLOGControlLock contention in yesterday's and
> this mail. If none of you is convinced, then I think we have no
> choice but to drop this patch.
>

I agree it's useful to reduce lock contention in general, but
considering the last set of benchmarks shows no benefit with recent
kernel, I think we really need a better understanding of what's going
on, what workloads / systems it's supposed to improve, etc.

I don't dare to suggest rejecting the patch, but I don't see how we
could commit any of the patches at this point. So perhaps "returned with
feedback" and resubmitting in the next CF (along with analysis of
improved workloads) would be appropriate.

regards

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

On Thu, Sep 22, 2016 at 7:44 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I don't dare to suggest rejecting the patch, but I don't see how we could
> commit any of the patches at this point. So perhaps "returned with feedback"
> and resubmitting in the next CF (along with analysis of improved workloads)
> would be appropriate.

I think it would be useful to have some kind of theoretical analysis
of how much time we're spending waiting for various locks.  So, for
example, suppose we one run of these tests with various client counts
- say, 1, 8, 16, 32, 64, 96, 128, 192, 256 - and we run "select
wait_event from pg_stat_activity" once per second throughout the test.
Then we see how many times we get each wait event, including NULL (no
wait event).  Now, from this, we can compute the approximate
percentage of time we're spending waiting on CLogControlLock and every
other lock, too, as well as the percentage of time we're not waiting
for lock.  That, it seems to me, would give us a pretty clear idea
what the maximum benefit we could hope for from reducing contention on
any given lock might be.

Now, we could also try that experiment with various patches.  If we
can show that some patch reduces CLogControlLock contention without
increasing TPS, they might still be worth committing for that reason.
Otherwise, you could have a chicken-and-egg problem.  If reducing
contention on A doesn't help TPS because of lock B and visca-versa,
then does that mean we can never commit any patch to reduce contention
on either lock?  Hopefully not.  But I agree with you that there's
certainly not enough evidence to commit any of these patches now.  To
my mind, these numbers aren't convincing.

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

On 09/23/2016 03:20 AM, Robert Haas wrote:
> On Thu, Sep 22, 2016 at 7:44 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> I don't dare to suggest rejecting the patch, but I don't see how
>> we could commit any of the patches at this point. So perhaps
>> "returned with feedback" and resubmitting in the next CF (along
>> with analysis of improvedworkloads) would be appropriate.
>
> I think it would be useful to have some kind of theoretical analysis
> of how much time we're spending waiting for various locks. So, for
> example, suppose we one run of these tests with various client
> counts - say, 1, 8, 16, 32, 64, 96, 128, 192, 256 - and we run
> "select wait_event from pg_stat_activity" once per second throughout
> the test. Then we see how many times we get each wait event,
> including NULL (no wait event). Now, from this, we can compute the
> approximate percentage of time we're spending waiting on
> CLogControlLock and every other lock, too, as well as the percentage
> of time we're not waiting for lock. That, it seems to me, would give
> us a pretty clear idea what the maximum benefit we could hope for
> from reducing contention on any given lock might be.
>

Yeah, I think that might be a good way to analyze the locks in general, 
not just got these patches. 24h run with per-second samples should give 
us about 86400 samples (well, multiplied by number of clients), which is 
probably good enough.

We also have LWLOCK_STATS, that might be interesting too, but I'm not 
sure how much it affects the behavior (and AFAIK it also only dumps the 
data to the server log).
>
> Now, we could also try that experiment with various patches. If we
> can show that some patch reduces CLogControlLock contention without
> increasing TPS, they might still be worth committing for that
> reason. Otherwise, you could have a chicken-and-egg problem. If
> reducing contention on A doesn't help TPS because of lock B and
> visca-versa, then does that mean we can never commit any patch to
> reduce contention on either lock? Hopefully not. But I agree with you
> that there's certainly not enough evidence to commit any of these
> patches now. To my mind, these numbers aren't convincing.
>

Yes, the chicken-and-egg problem is why the tests were done with 
unlogged tables (to work around the WAL lock).

regards

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

On Fri, Sep 23, 2016 at 7:17 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/23/2016 03:20 AM, Robert Haas wrote:
>>
>> On Thu, Sep 22, 2016 at 7:44 PM, Tomas Vondra
>> <tomas.vondra@2ndquadrant.com> wrote:
>>>
>>> I don't dare to suggest rejecting the patch, but I don't see how
>>> we could commit any of the patches at this point. So perhaps
>>> "returned with feedback" and resubmitting in the next CF (along
>>> with analysis of improvedworkloads) would be appropriate.
>>
>>
>> I think it would be useful to have some kind of theoretical analysis
>> of how much time we're spending waiting for various locks. So, for
>> example, suppose we one run of these tests with various client
>> counts - say, 1, 8, 16, 32, 64, 96, 128, 192, 256 - and we run
>> "select wait_event from pg_stat_activity" once per second throughout
>> the test. Then we see how many times we get each wait event,
>> including NULL (no wait event). Now, from this, we can compute the
>> approximate percentage of time we're spending waiting on
>> CLogControlLock and every other lock, too, as well as the percentage
>> of time we're not waiting for lock. That, it seems to me, would give
>> us a pretty clear idea what the maximum benefit we could hope for
>> from reducing contention on any given lock might be.
>>
>
> Yeah, I think that might be a good way to analyze the locks in general, not
> just got these patches. 24h run with per-second samples should give us about
> 86400 samples (well, multiplied by number of clients), which is probably
> good enough.
>
> We also have LWLOCK_STATS, that might be interesting too, but I'm not sure
> how much it affects the behavior (and AFAIK it also only dumps the data to
> the server log).
>

Right, I think LWLOCK_STATS give us the count of how many time we have
blocked due to particular lock like below where *blk* gives that
number.

PID 164692 lwlock main 11: shacq 2734189 exacq 146304 blk 73808
spindelay 73 dequeue self 57241

I think doing some experiments with both the techniques can help us to
take a call on these patches.

Do we want these experiments on different kernel versions or are we
okay with the current version on cthulhu (3.10) or we want to only
consider the results with latest kernel?

>>
>>
>> Now, we could also try that experiment with various patches. If we
>> can show that some patch reduces CLogControlLock contention without
>> increasing TPS, they might still be worth committing for that
>> reason. Otherwise, you could have a chicken-and-egg problem. If
>> reducing contention on A doesn't help TPS because of lock B and
>> visca-versa, then does that mean we can never commit any patch to
>> reduce contention on either lock? Hopefully not. But I agree with you
>> that there's certainly not enough evidence to commit any of these
>> patches now. To my mind, these numbers aren't convincing.
>>
>
> Yes, the chicken-and-egg problem is why the tests were done with unlogged
> tables (to work around the WAL lock).
>

Yeah, but I suspect still there was a impact due to ProcArrayLock.



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

On Fri, Sep 23, 2016 at 5:14 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> On 09/21/2016 08:04 AM, Amit Kapila wrote:
>>
>
> (c) Although it's not visible in the results, 4.5.5 almost perfectly
> eliminated the fluctuations in the results. For example when 3.2.80 produced
> this results (10 runs with the same parameters):
>
>     12118 11610 27939 11771 18065
>     12152 14375 10983 13614 11077
>
> we get this on 4.5.5
>
>     37354 37650 37371 37190 37233
>     38498 37166 36862 37928 38509
>
> Notice how much more even the 4.5.5 results are, compared to 3.2.80.
>

how long each run was?  Generally, I do half-hour run to get stable results.

> (d) There's no sign of any benefit from any of the patches (it was only
> helpful >= 128 clients, but that's where the tps actually dropped on 3.2.80
> - apparently 4.5.5 fixes that and the benefit is gone).
>
> It's a bit annoying that after upgrading from 3.2.80 to 4.5.5, the
> performance with 32 and 64 clients dropped quite noticeably (by more than
> 10%). I believe that might be a kernel regression, but perhaps it's a price
> for improved scalability for higher client counts.
>
> It of course begs the question what kernel version is running on the machine
> used by Dilip (i.e. cthulhu)? Although it's a Power machine, so I'm not sure
> how much the kernel matters on it.
>

cthulhu is a x86 m/c and the kernel version is 3.10.  Seeing, the
above results I think kernel version do matter, but does that mean we
ignore the benefits we are seeing on somewhat older kernel version.  I
think right answer here is to do some experiments which can show the
actual contention as suggested by Robert and you.

> I'll ask someone else with access to this particular machine to repeat the
> tests, as I have a nagging suspicion that I've missed something important
> when compiling / running the benchmarks. I'll also retry the benchmarks on
> 3.2.80 to see if I get the same numbers.
>
>>
>> Okay, but I think it is better to see the results between 64~128
>> client count and may be greater than128 client counts, because it is
>> clear that patch won't improve performance below that.
>>
>
> There are results for 64, 128 and 192 clients. Why should we care about
> numbers in between? How likely (and useful) would it be to get improvement
> with 96 clients, but no improvement for 64 or 128 clients?
>

The only point to take was to see from where we have started seeing
improvement, saying that the TPS has improved from >=72 client count
is different from saying that it has improved from >=128.

>> No issues, I have already explained why I think it is important to
>> reduce the remaining CLOGControlLock contention in yesterday's and
>> this mail. If none of you is convinced, then I think we have no
>> choice but to drop this patch.
>>
>
> I agree it's useful to reduce lock contention in general, but considering
> the last set of benchmarks shows no benefit with recent kernel, I think we
> really need a better understanding of what's going on, what workloads /
> systems it's supposed to improve, etc.
>
> I don't dare to suggest rejecting the patch, but I don't see how we could
> commit any of the patches at this point. So perhaps "returned with feedback"
> and resubmitting in the next CF (along with analysis of improved workloads)
> would be appropriate.
>

Agreed with your conclusion and changed the status of patch in CF accordingly.

Many thanks for doing the tests.

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

On 09/23/2016 05:10 AM, Amit Kapila wrote:
> On Fri, Sep 23, 2016 at 5:14 AM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> On 09/21/2016 08:04 AM, Amit Kapila wrote:
>>>
>>
>> (c) Although it's not visible in the results, 4.5.5 almost perfectly
>> eliminated the fluctuations in the results. For example when 3.2.80 produced
>> this results (10 runs with the same parameters):
>>
>>     12118 11610 27939 11771 18065
>>     12152 14375 10983 13614 11077
>>
>> we get this on 4.5.5
>>
>>     37354 37650 37371 37190 37233
>>     38498 37166 36862 37928 38509
>>
>> Notice how much more even the 4.5.5 results are, compared to 3.2.80.
>>
>
> how long each run was?  Generally, I do half-hour run to get stable results.
>

10 x 5-minute runs for each client count. The full shell script driving 
the benchmark is here: http://bit.ly/2doY6ID and in short it looks like 
this:
    for r in `seq 1 $runs`; do        for c in 1 8 16 32 64 128 192; do            psql -c checkpoint
pgbench-j 8 -c $c ...        done    done
 

>>
>> It of course begs the question what kernel version is running on
>> the machine used by Dilip (i.e. cthulhu)? Although it's a Power
>> machine, so I'm not sure how much the kernel matters on it.
>>
>
> cthulhu is a x86 m/c and the kernel version is 3.10. Seeing, the
> above results I think kernel version do matter, but does that mean
> we ignore the benefits we are seeing on somewhat older kernel
> version. I think right answer here is to do some experiments which
> can show the actual contention as suggested by Robert and you.
>

Yes, I think it'd be useful to test a new kernel version. Perhaps try 
4.5.x so that we can compare it to my results. Maybe even try using my 
shell script

>>
>> There are results for 64, 128 and 192 clients. Why should we care
>> about numbers in between? How likely (and useful) would it be to
>> get improvement with 96 clients, but no improvement for 64 or 128
>> clients?>>
>
> The only point to take was to see from where we have started seeing
> improvement, saying that the TPS has improved from >=72 client count
> is different from saying that it has improved from >=128.
>

I find the exact client count rather uninteresting - it's going to be 
quite dependent on hardware, workload etc.
>>
>> I don't dare to suggest rejecting the patch, but I don't see how
>> we could commit any of the patches at this point. So perhaps
>> "returned with feedback" and resubmitting in the next CF (along
>> with analysis of improvedworkloads) would be appropriate.
>>
>
> Agreed with your conclusion and changed the status of patch in CF
> accordingly.
>

+1


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

On 09/23/2016 01:44 AM, Tomas Vondra wrote:
>...
> The 4.5 kernel clearly changed the results significantly:
>
...>
> (c) Although it's not visible in the results, 4.5.5 almost perfectly
> eliminated the fluctuations in the results. For example when 3.2.80
> produced this results (10 runs with the same parameters):
>
>     12118 11610 27939 11771 18065
>     12152 14375 10983 13614 11077
>
> we get this on 4.5.5
>
>     37354 37650 37371 37190 37233
>     38498 37166 36862 37928 38509
>
> Notice how much more even the 4.5.5 results are, compared to 3.2.80.
>

The more I think about these random spikes in pgbench performance on 
3.2.80, the more I find them intriguing. Let me show you another example 
(from Dilip's workload and group-update patch on 64 clients).

This is on 3.2.80:
  44175  34619  51944  38384  49066  37004  47242  36296  46353  36180

and on 4.5.5 it looks like this:
  34400  35559  35436  34890  34626  35233  35756  34876  35347  35486

So the 4.5.5 results are much more even, but overall clearly below 
3.2.80. How does 3.2.80 manage to do ~50k tps in some of the runs? 
Clearly we randomly do something right, but what is it and why doesn't 
it happen on the new kernel? And how could we do it every time?

regards

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