Re: Speed up Clog Access by increasing CLOG buffers

After reducing ProcArrayLock contention in commit
(0e141c0fbb211bdd23783afa731e3eef95c9ad7a), the other lock
which seems to be contentious in read-write transactions is
CLogControlLock. In my investigation, I found that the contention
is mainly due to two reasons, one is that while writing the transaction
status in CLOG (TransactionIdSetPageStatus()), it acquires EXCLUSIVE
CLogControlLock which contends with every other transaction which
tries to access the CLOG for checking transaction status and to reduce it
already a patch [1] is proposed by Simon; Second contention is due to
the reason that when the CLOG page is not found in CLOG buffers, it
needs to acquire CLogControlLock in Exclusive mode which again contends
with shared lockers which tries to access the transaction status.

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.

Performance Data
-----------------------------
RAM - 500GB
8 sockets, 64 cores(Hyperthreaded128 threads total)

Non-default parameters
------------------------------------
max_connections = 300
shared_buffers=8GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout =35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB

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.

HEAD - commit 253de7e1
Patch-1 - increase_clog_bufs_v1

Client Count/Patch_ver 128 256 HEAD 16657 10512 Patch-1 16694 10477

I think the benefit of this patch would be more significant along
with the other patch to reduce CLogControlLock contention [1]
(I have not tested both the patches together as still there are
few issues left in the other patch), however it has it's own independent
value, so can be considered separately.

Thoughts?

[1] -
http://www.postgresql.org/message-id/CANP8+j+imQfHxkChFyfnXDyi6k-arAzRV+ZG-V_OFxEtJjOL2Q@mail.gmail.com

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

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 7:04 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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?
> >

Yes, I am working on it, what I have in mind is to create a table with
large number of rows (say 50000000) and have each row with different
transaction id. Now each transaction should try to update rows that
are at least 1048576 (number of transactions whose status can be held in
32 CLog buffers) distance apart, that way for each update it will try to
access
Clog page that is not in-memory. Let me know if you can think of any
better or simpler way.

> > 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.
>

There could be some benefits of it, but I think we still have to acquire
Exclusive lock while committing transaction or while Extending Clog
which are also major sources of contention in this area. I think the
benefits of moving it to shared_buffers could be that the upper limit on
number of pages that can be retained in memory could be increased and even
if we have to replace the page, responsibility to flush it could be
delegated
to checkpoint. So yes, there could be benefits with this idea, but not sure
if they are worth investigating this idea, one thing we could try if you
think
that is beneficial is that just skip fsync during write of clog pages and
if thats
beneficial, then we can think of pushing it to checkpoint (something similar
to what Andres has mentioned on nearby thread).

Yet another way could be to have configuration variable for clog buffers
(Clog_Buffers).

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

On Mon, Sep 7, 2015 at 9:34 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 Thu, Sep 3, 2015 at 5:11 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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
> >
>
> 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?
>

Okay, I have tried one such test, but what I could come up with is on an
average every 100th access is a disk access and then tested it with
different number of clog buffers and client count. Below is the result:

Non-default parameters
------------------------------------
max_connections = 300
shared_buffers=32GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout =35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB
autovacuum=off

HEAD - commit 49124613
Patch-1 - Clog Buffers - 64
Patch-2 - Clog Buffers - 128

Client Count/Patch_ver 1 8 64 128 HEAD 1395 8336 37866 34463 Patch-1 1615
8180 37799 35315 Patch-2 1409 8219 37068 34729
So there is not much difference in test results with different values for
Clog
buffers, probably because the I/O has dominated the test and it shows that
increasing the clog buffers won't regress the current behaviour even though
there are lot more accesses for transaction status outside CLOG buffers.

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.

Test
-------
1. Attached file clog_prep.sh should create and populate the required
table and create the function used to access the CLOG pages. You
might want to update the no_of_rows based on the rows you want to
create in table
2. Attached file access_clog_disk.sql is used to execute the function
with random values. You might want to update nrows variable based
on the rows created in previous step.
3. Use pgbench as follows with different client count
./pgbench -c 4 -j 4 -n -M prepared -f "access_clog_disk.sql" -T 300 postgres
4. To ensure that clog access function always accesses same data
during each run, the test ensures to copy the data_directory created by
step-1
before each run.

I have checked by adding some instrumentation that approximately
every 100th access is disk access, attached patch clog_info-v1.patch
adds the necessary instrumentation in code.

As an example, pgbench test yields below results:
./pgbench -c 4 -j 4 -n -M prepared -f "access_clog_disk.sql" -T 180 postgres

LOG: trans_status(3169396)
LOG: trans_status_disk(29546)
LOG: trans_status(3054952)
LOG: trans_status_disk(28291)
LOG: trans_status(3131242)
LOG: trans_status_disk(28989)
LOG: trans_status(3155449)
LOG: trans_status_disk(29347)

Here 'trans_status' is the number of times the process went for accessing
the CLOG status and 'trans_status_disk' is the number of times it went
to disk for accessing CLOG page.

>
> > /*
> > * Number of shared CLOG buffers.
> > *
>
>
>
> I think the comment should be more drastically rephrased to not
> reference individual versions and numbers.
>

Updated comments and the patch (increate_clog_bufs_v2.patch)
containing the same is attached.

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

On Fri, Sep 11, 2015 at 10:31 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Sep 11, 2015 at 10:31 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?
>

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?

Now another possibility here could be that we try by commenting out fsync
in CLOG path to see how much it impact the performance of this test and
then for pgbench test. I am not sure there will be any impact because even
every 100th transaction goes to disk access that is still less as compare
WAL fsync which we have to perform for each transaction.

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

On Fri, Sep 11, 2015 at 11:01 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Fri, Sep 18, 2015 at 11:08 PM, Jesper Pedersen <
jesper(dot)pedersen(at)redhat(dot)com> wrote:

> 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,).
>
>
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?

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

On Mon, Aug 31, 2015 at 9:49 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Fri, Sep 11, 2015 at 8:01 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Fri, Sep 11, 2015 at 9:21 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >
> > On Fri, Sep 11, 2015 at 10:31 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?
> >
>
> 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.
>

That guarantees that every xid occupies its own 32-contiguous-pages chunk
of clog.

But clog pages are not pulled in and out in 32-page chunks, but one page
chunks. So you would only need 32,768 differences to get every real
transaction to live on its own clog page, which means every look up of a
different real transaction would have to do a page replacement. (I think
your references to disk access here are misleading. Isn't the issue here
the contention on the lock that controls the page replacement, not the
actual IO?)

I've attached a patch that allows you set the guc "JJ_xid",which makes it
burn the given number of xids every time one new one is asked for. (The
patch introduces lots of other stuff as well, but I didn't feel like
ripping the irrelevant parts out--if you don't set any of the other gucs it
introduces from their defaults, they shouldn't cause you trouble.) I think
there are other tools around that do the same thing, but this is the one I
know about. It is easy to drive the system into wrap-around shutdown with
this, so lowering autovacuum_vacuum_cost_delay is a good idea.

Actually I haven't attached it, because then the commitfest app will list
it as the patch needing review, instead I've put it here
https://drive.google.com/file/d/0Bzqrh1SO9FcERV9EUThtT3pacmM/view?usp=sharing

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?
>
> Now another possibility here could be that we try by commenting out fsync
> in CLOG path to see how much it impact the performance of this test and
> then for pgbench test. I am not sure there will be any impact because even
> every 100th transaction goes to disk access that is still less as compare
> WAL fsync which we have to perform for each transaction.
>

You mentioned that your clog is not on ssd, but surely at this scale of
hardware, the hdd the clog is on has a bbu in front of it, no?

But I thought Andres' concern was not about fsync, but about the fact that
the SLRU does linear scans (repeatedly) of the buffers while holding the
control lock? At some point, scanning more and more buffers under the lock
is going to cause more contention than scanning fewer buffers and just
evicting a page will.

Cheers,

Jeff

On Mon, Oct 5, 2015 at 6:34 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:

> On Fri, Sep 11, 2015 at 8:01 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>>
>
>
> That guarantees that every xid occupies its own 32-contiguous-pages chunk
> of clog.
>
> But clog pages are not pulled in and out in 32-page chunks, but one page
> chunks. So you would only need 32,768 differences to get every real
> transaction to live on its own clog page, which means every look up of a
> different real transaction would have to do a page replacement.
>

Agreed, but that doesn't effect the test result with the test done above.

> (I think your references to disk access here are misleading. Isn't the
> issue here the contention on the lock that controls the page replacement,
> not the actual IO?)
>
>
The point is that if there is no I/O needed, then all the read-access for
transaction status will just use Shared locks, however if there is an I/O,
then it would need an Exclusive lock.

> I've attached a patch that allows you set the guc "JJ_xid",which makes it
> burn the given number of xids every time one new one is asked for. (The
> patch introduces lots of other stuff as well, but I didn't feel like
> ripping the irrelevant parts out--if you don't set any of the other gucs it
> introduces from their defaults, they shouldn't cause you trouble.) I think
> there are other tools around that do the same thing, but this is the one I
> know about. It is easy to drive the system into wrap-around shutdown with
> this, so lowering autovacuum_vacuum_cost_delay is a good idea.
>
> Actually I haven't attached it, because then the commitfest app will list
> it as the patch needing review, instead I've put it here
> https://drive.google.com/file/d/0Bzqrh1SO9FcERV9EUThtT3pacmM/view?usp=sharing
>
>
Thanks, I think probably this could also be used for testing.

> 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?
>>
>> Now another possibility here could be that we try by commenting out fsync
>> in CLOG path to see how much it impact the performance of this test and
>> then for pgbench test. I am not sure there will be any impact because
>> even
>> every 100th transaction goes to disk access that is still less as compare
>> WAL fsync which we have to perform for each transaction.
>>
>
> You mentioned that your clog is not on ssd, but surely at this scale of
> hardware, the hdd the clog is on has a bbu in front of it, no?
>
>
Yes.

> But I thought Andres' concern was not about fsync, but about the fact that
> the SLRU does linear scans (repeatedly) of the buffers while holding the
> control lock? At some point, scanning more and more buffers under the lock
> is going to cause more contention than scanning fewer buffers and just
> evicting a page will.
>
>

Yes, at some point, that could matter, but I could not see the impact
at 64 or 128 number of Clog buffers.

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

On Mon, Sep 21, 2015 at 6:25 PM, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com
> wrote:

> 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.
>

I think the main focus for test in this area would be at higher client
count. At what scale factors have you taken the data and what are
the other non-default settings you have used. By the way, have you
tried by dropping and recreating the database and restarting the server
after each run, can you share the exact steps you have used to perform
the tests. I am not sure why it is not showing the benefit in your testing,
may be the benefit is on some what more higher end m/c or it could be
that some of the settings used for test are not same as mine or the way
to test the read-write workload of pgbench is different.

In anycase, I went ahead and tried further reducing the CLogControlLock
contention by grouping the transaction status updates. The basic idea
is same as is used to reduce the ProcArrayLock contention [1] which is to
allow one of the proc to become leader and update the transaction status for
other active transactions in system. This has helped to reduce the
contention
around CLOGControlLock. Attached patch group_update_clog_v1.patch
implements this idea.

I have taken performance data with this patch to see the impact at
various scale-factors. All the data is for cases when data fits in shared
buffers and is taken against commit - 5c90a2ff on server with below
configuration and non-default postgresql.conf settings.

Performance Data
-----------------------------
RAM - 500GB
8 sockets, 64 cores(Hyperthreaded128 threads total)

Non-default parameters
------------------------------------
max_connections = 300
shared_buffers=8GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout =35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB

Refer attached files for performance data.

sc_300_perf.png - This data indicates that at scale_factor 300, there is a
gain of ~15% at higher client counts, without degradation at lower client
count.
different_sc_perf.png - At various scale factors, there is a gain from
~15% to 41% at higher client counts and in some cases we see gain
of ~5% at somewhat moderate client count (64) as well.
perf_write_clogcontrollock_data_v1.ods - Detailed performance data at
various client counts and scale factors.

Feel free to ask for more details if the data in attached files is not
clear.

Below is the LWLock_Stats information with and without patch:

Stats Data
---------
A. scale_factor = 300; shared_buffers=32GB; client_connections - 128

HEAD - 5c90a2ff
----------------
CLogControlLock Data
------------------------
PID 94100 lwlock main 11: shacq 678672 exacq 326477 blk 204427 spindelay
8532 dequeue self 93192
PID 94129 lwlock main 11: shacq 757047 exacq 363176 blk 207840 spindelay
8866 dequeue self 96601
PID 94115 lwlock main 11: shacq 721632 exacq 345967 blk 207665 spindelay
8595 dequeue self 96185
PID 94011 lwlock main 11: shacq 501900 exacq 241346 blk 173295 spindelay
7882 dequeue self 78134
PID 94087 lwlock main 11: shacq 653701 exacq 314311 blk 201733 spindelay
8419 dequeue self 92190

After Patch group_update_clog_v1
----------------
CLogControlLock Data
------------------------
PID 100205 lwlock main 11: shacq 836897 exacq 176007 blk 116328 spindelay
1206 dequeue self 54485
PID 100034 lwlock main 11: shacq 437610 exacq 91419 blk 77523 spindelay 994
dequeue self 35419
PID 100175 lwlock main 11: shacq 748948 exacq 158970 blk 114027 spindelay
1277 dequeue self 53486
PID 100162 lwlock main 11: shacq 717262 exacq 152807 blk 115268 spindelay
1227 dequeue self 51643
PID 100214 lwlock main 11: shacq 856044 exacq 180422 blk 113695 spindelay
1202 dequeue self 54435

The above data indicates that contention due to CLogControlLock is
reduced by around 50% with this patch.

The reasons for remaining contention could be:

1. Readers of clog data (checking transaction status data) can take
Exclusive CLOGControlLock when reading the page from disk, this can
contend with other Readers (shared lockers of CLogControlLock) and with
exclusive locker which updates transaction status. One of the ways to
mitigate this contention is to increase the number of CLOG buffers for which
patch has been already posted on this thread.

2. Readers of clog data (checking transaction status data) takes shared
CLOGControlLock which can contend with exclusive locker (Group leader) which
updates transaction status. I have tried to reduce the amount of work done
by group leader, by allowing group leader to just read the Clog page once
for all the transactions in the group which updated the same CLOG page
(idea similar to what we currently we use for updating the status of
transactions
having sub-transaction tree), but that hasn't given any further performance
boost,
so I left it.

I think we can use some other ways as well to reduce the contention around
CLOGControlLock by doing somewhat major surgery around SLRU like using
buffer pools similar to shared buffers, but this idea gives us moderate
improvement without much impact on exiting mechanism.

Thoughts?

[1] -
http://www.postgresql.org/message-id/CAA4eK1JbX4FzPHigNt0JSaz30a85BPJV+ewhk+wg_o-T6xufEA@mail.gmail.com

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

On Mon, Sep 21, 2015 at 6:34 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>
> On Mon, Aug 31, 2015 at 9:49 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>

I could see benefits of doing compression for CLOG, but I think it won't
be straight forward, other than handling of compression and decompression,
currently code relies on transaction id to find the clog page, that will
not work after compression or we need to do some changes in that mapping
to make it work. Also I think it could avoid the increase of clog buffers
which
can help readers, but it won't help much for contention around clog
updates for transaction status.

Overall this idea sounds promising, but I think the work involved is more
than the benefit I am expecting for the current optimization we are
discussing.

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

On Tue, Nov 17, 2015 at 1:32 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Mon, Sep 21, 2015 at 6:34 AM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> >
> > On Mon, Aug 31, 2015 at 9:49 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
> >
>
> Overall this idea sounds promising, but I think the work involved is more
> than the benefit I am expecting for the current optimization we are
> discussing.
>

Sorry, I think last line is slightly confusing, let me try to again write
it:

Overall this idea sounds promising, but I think the work involved is more
than the benefit expected from the current optimization we are
discussing.

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

On 17 November 2015 at 06:50, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> In anycase, I went ahead and tried further reducing the CLogControlLock
> contention by grouping the transaction status updates. The basic idea
> is same as is used to reduce the ProcArrayLock contention [1] which is to
> allow one of the proc to become leader and update the transaction status
> for
> other active transactions in system. This has helped to reduce the
> contention
> around CLOGControlLock.
>

Sounds good. The technique has proved effective with proc array and makes
sense to use here also.

> Attached patch group_update_clog_v1.patch
> implements this idea.
>

I don't think we should be doing this only for transactions that don't have
subtransactions. We are trying to speed up real cases, not just benchmarks.

So +1 for the concept, patch is going in right direction though lets do the
full press-up.

The above data indicates that contention due to CLogControlLock is
> reduced by around 50% with this patch.
>
> The reasons for remaining contention could be:
>
> 1. Readers of clog data (checking transaction status data) can take
> Exclusive CLOGControlLock when reading the page from disk, this can
> contend with other Readers (shared lockers of CLogControlLock) and with
> exclusive locker which updates transaction status. One of the ways to
> mitigate this contention is to increase the number of CLOG buffers for
> which
> patch has been already posted on this thread.
>
> 2. Readers of clog data (checking transaction status data) takes shared
> CLOGControlLock which can contend with exclusive locker (Group leader)
> which
> updates transaction status. I have tried to reduce the amount of work done
> by group leader, by allowing group leader to just read the Clog page once
> for all the transactions in the group which updated the same CLOG page
> (idea similar to what we currently we use for updating the status of
> transactions
> having sub-transaction tree), but that hasn't given any further
> performance boost,
> so I left it.
>
> I think we can use some other ways as well to reduce the contention around
> CLOGControlLock by doing somewhat major surgery around SLRU like using
> buffer pools similar to shared buffers, but this idea gives us moderate
> improvement without much impact on exiting mechanism.
>

My earlier patch to reduce contention by changing required lock level is
still valid here. Increasing the number of buffers doesn't do enough to
remove that.

I'm working on a patch to use a fast-update area like we use for GIN. If a
page is not available when we want to record commit, just store it in a
hash table, when not in crash recovery. I'm experimenting with writing WAL
for any xids earlier than last checkpoint, though we could also trickle
writes and/or flush them in batches at checkpoint time - your code would
help there.

The hash table can also be used for lookups. My thinking is that most reads
of older xids are caused by long running transactions, so they cause a page
fault at commit and then other page faults later when people read them back
in. The hash table works for both kinds of page fault.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 17, 2015 at 2:45 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 17 November 2015 at 06:50, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>
>> In anycase, I went ahead and tried further reducing the CLogControlLock
>> contention by grouping the transaction status updates. The basic idea
>> is same as is used to reduce the ProcArrayLock contention [1] which is to
>> allow one of the proc to become leader and update the transaction status
>> for
>> other active transactions in system. This has helped to reduce the
>> contention
>> around CLOGControlLock.
>>
>
> Sounds good. The technique has proved effective with proc array and makes
> sense to use here also.
>
>
>> Attached patch group_update_clog_v1.patch
>> implements this idea.
>>
>
> I don't think we should be doing this only for transactions that don't
> have subtransactions.
>

The reason for not doing this optimization for subtransactions is that we
need to advertise the information that Group leader needs for updating
the transaction status and if we want to do it for sub transactions, then
all the subtransaction id's needs to be advertised. Now here the tricky
part is that number of subtransactions for which the status needs to
be updated is dynamic, so reserving memory for it would be difficult.
However, we can reserve some space in Proc like we do for XidCache
(cache of sub transaction ids) and then use that to advertise that many
Xid's at-a-time or just allow this optimization if number of subtransactions
is lesser than or equal to the size of this new XidCache. I am not sure
if it is good idea to use the existing XidCache for this purpose in which
case we need to have a separate space in PGProc for this purpose. I
don't see allocating space for 64 or so subxid's as a problem, however
doing it for bigger number could be cause of concern.

> We are trying to speed up real cases, not just benchmarks.
>
> So +1 for the concept, patch is going in right direction though lets do
> the full press-up.
>
>
I have mentioned above the reason for not doing it for sub transactions, if
you think it is viable to reserve space in shared memory for this purpose,
then
I can include the optimization for subtransactions as well.

> The above data indicates that contention due to CLogControlLock is
>> reduced by around 50% with this patch.
>>
>> The reasons for remaining contention could be:
>>
>> 1. Readers of clog data (checking transaction status data) can take
>> Exclusive CLOGControlLock when reading the page from disk, this can
>> contend with other Readers (shared lockers of CLogControlLock) and with
>> exclusive locker which updates transaction status. One of the ways to
>> mitigate this contention is to increase the number of CLOG buffers for
>> which
>> patch has been already posted on this thread.
>>
>> 2. Readers of clog data (checking transaction status data) takes shared
>> CLOGControlLock which can contend with exclusive locker (Group leader)
>> which
>> updates transaction status. I have tried to reduce the amount of work
>> done
>> by group leader, by allowing group leader to just read the Clog page once
>> for all the transactions in the group which updated the same CLOG page
>> (idea similar to what we currently we use for updating the status of
>> transactions
>> having sub-transaction tree), but that hasn't given any further
>> performance boost,
>> so I left it.
>>
>> I think we can use some other ways as well to reduce the contention around
>> CLOGControlLock by doing somewhat major surgery around SLRU like using
>> buffer pools similar to shared buffers, but this idea gives us moderate
>> improvement without much impact on exiting mechanism.
>>
>
> My earlier patch to reduce contention by changing required lock level is
> still valid here. Increasing the number of buffers doesn't do enough to
> remove that.
>
>
I understand that increasing alone the number of buffers is not
enough, that's why I have tried this group leader idea. However
if we do something on lines what you have described below
(handling page faults) could avoid the need for increasing buffers.

> I'm working on a patch to use a fast-update area like we use for GIN. If a
> page is not available when we want to record commit, just store it in a
> hash table, when not in crash recovery. I'm experimenting with writing WAL
> for any xids earlier than last checkpoint, though we could also trickle
> writes and/or flush them in batches at checkpoint time - your code would
> help there.
>
> The hash table can also be used for lookups. My thinking is that most
> reads of older xids are caused by long running transactions, so they cause
> a page fault at commit and then other page faults later when people read
> them back in. The hash table works for both kinds of page fault.
>
>

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

On 17 November 2015 at 11:27, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> Attached patch group_update_clog_v1.patch
>>> implements this idea.
>>>
>>
>> I don't think we should be doing this only for transactions that don't
>> have subtransactions.
>>
>
> The reason for not doing this optimization for subtransactions is that we
> need to advertise the information that Group leader needs for updating
> the transaction status and if we want to do it for sub transactions, then
> all the subtransaction id's needs to be advertised. Now here the tricky
> part is that number of subtransactions for which the status needs to
> be updated is dynamic, so reserving memory for it would be difficult.
> However, we can reserve some space in Proc like we do for XidCache
> (cache of sub transaction ids) and then use that to advertise that many
> Xid's at-a-time or just allow this optimization if number of
> subtransactions
> is lesser than or equal to the size of this new XidCache. I am not sure
> if it is good idea to use the existing XidCache for this purpose in which
> case we need to have a separate space in PGProc for this purpose. I
> don't see allocating space for 64 or so subxid's as a problem, however
> doing it for bigger number could be cause of concern.
>
>
>> We are trying to speed up real cases, not just benchmarks.
>>
>> So +1 for the concept, patch is going in right direction though lets do
>> the full press-up.
>>
>>
> I have mentioned above the reason for not doing it for sub transactions, if
> you think it is viable to reserve space in shared memory for this purpose,
> then
> I can include the optimization for subtransactions as well.
>

The number of subxids is unbounded, so as you say, reserving shmem isn't
viable.

I'm interested in real world cases, so allocating 65 xids per process isn't
needed, but we can say is that the optimization shouldn't break down
abruptly in the presence of a small/reasonable number of subtransactions.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 17, 2015 at 5:04 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 17 November 2015 at 11:27, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> We are trying to speed up real cases, not just benchmarks.
>>>
>>> So +1 for the concept, patch is going in right direction though lets do
>>> the full press-up.
>>>
>>>
>> I have mentioned above the reason for not doing it for sub transactions,
>> if
>> you think it is viable to reserve space in shared memory for this
>> purpose, then
>> I can include the optimization for subtransactions as well.
>>
>
> The number of subxids is unbounded, so as you say, reserving shmem isn't
> viable.
>
> I'm interested in real world cases, so allocating 65 xids per process
> isn't needed, but we can say is that the optimization shouldn't break down
> abruptly in the presence of a small/reasonable number of subtransactions.
>
>
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?

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

On Tue, Nov 17, 2015 at 5:18 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Tue, Nov 17, 2015 at 5:04 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
>
>> On 17 November 2015 at 11:27, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>>
>> We are trying to speed up real cases, not just benchmarks.
>>>>
>>>> So +1 for the concept, patch is going in right direction though lets do
>>>> the full press-up.
>>>>
>>>>
>>> I have mentioned above the reason for not doing it for sub transactions,
>>> if
>>> you think it is viable to reserve space in shared memory for this
>>> purpose, then
>>> I can include the optimization for subtransactions as well.
>>>
>>
>> The number of subxids is unbounded, so as you say, reserving shmem isn't
>> viable.
>>
>> I'm interested in real world cases, so allocating 65 xids per process
>> isn't needed, but we can say is that the optimization shouldn't break down
>> abruptly in the presence of a small/reasonable number of subtransactions.
>>
>>
> 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.
>

I think this won't work as it is, because subxids in XidCache could be
on different pages in which case we either need an additional flag
in XidCache array or a separate array to indicate for which subxids
we want to update the status. I don't see any better way to do this
optimization for sub transactions, do you have something else in
mind?

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

On 17 November 2015 at 11:48, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Tue, Nov 17, 2015 at 5:04 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
>
>> On 17 November 2015 at 11:27, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>>
>> We are trying to speed up real cases, not just benchmarks.
>>>>
>>>> So +1 for the concept, patch is going in right direction though lets do
>>>> the full press-up.
>>>>
>>>>
>>> I have mentioned above the reason for not doing it for sub transactions,
>>> if
>>> you think it is viable to reserve space in shared memory for this
>>> purpose, then
>>> I can include the optimization for subtransactions as well.
>>>
>>
>> The number of subxids is unbounded, so as you say, reserving shmem isn't
>> viable.
>>
>> I'm interested in real world cases, so allocating 65 xids per process
>> isn't needed, but we can say is that the optimization shouldn't break down
>> abruptly in the presence of a small/reasonable number of subtransactions.
>>
>>
> 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

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 17, 2015 at 6:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 17 November 2015 at 11:48, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>> On Tue, Nov 17, 2015 at 5:04 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
>> wrote:
>>
>>> On 17 November 2015 at 11:27, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>>> wrote:
>>>
>>> We are trying to speed up real cases, not just benchmarks.
>>>>>
>>>>> So +1 for the concept, patch is going in right direction though lets
>>>>> do the full press-up.
>>>>>
>>>>>
>>>> I have mentioned above the reason for not doing it for sub
>>>> transactions, if
>>>> you think it is viable to reserve space in shared memory for this
>>>> purpose, then
>>>> I can include the optimization for subtransactions as well.
>>>>
>>>
>>> The number of subxids is unbounded, so as you say, reserving shmem isn't
>>> viable.
>>>
>>> I'm interested in real world cases, so allocating 65 xids per process
>>> isn't needed, but we can say is that the optimization shouldn't break down
>>> abruptly in the presence of a small/reasonable number of subtransactions.
>>>
>>>
>> 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)
>
>
Yes, that is what the above idea is, but the problem with that is leader
won't be able to collect the subxids of member proc's (from each member
proc's XidCache) as it doesn't have information which of those subxid's
needs to be update as part of current transaction status update (for
subtransactions on different clog pages, we update the status of those
in multiple phases). I think it could only be possible to use the above
idea
if all the subtransactions are on same page, which we can identify in
function TransactionIdSetPageStatus(). Though it looks okay that we
can apply this optimization when number of subtransactions is lesser
than 65 and all exist on same page, still it would be better if we can
apply it generically for all cases when number of subtransactions is small
(say 32 or 64). Does this explanation clarify the problem with the above
idea to handle subtransactions?

No additional shmem is required
>
>
If we want to do it for all cases when number of subtransactions
are small then we need extra memory.

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

On Tue, Nov 17, 2015 at 6:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 17 November 2015 at 11:48, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

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

On Thu, Nov 26, 2015 at 11:32 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Nov 17, 2015 at 6:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>
>> On 17 November 2015 at 11:48, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Sun, Nov 29, 2015 at 1:47 AM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>
> On Thu, Nov 26, 2015 at 11:32 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Tue, Nov 17, 2015 at 6:30 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
wrote:
> >>
> >> On 17 November 2015 at 11:48, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?
>

This is in addition to increasing the clog buffers to 64, but with this
patch
(Group Clog updation), the effect of increasing the clog buffers will be
lesser.

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

On Fri, Nov 27, 2015 at 2:32 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Wed, Dec 2, 2015 at 8:59 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>
> 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.
>

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.

> 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.

>
> 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.
>

Agreed and I have already handled it in the last version of patch posted
by me.

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

On Thu, Dec 3, 2015 at 1:48 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Wed, Dec 9, 2015 at 1:02 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Dec 3, 2015 at 1:48 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>

Okay, attached patch group_update_clog_v3.patch implements the above.

> >> 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 have implemented this idea as well in the attached patch
group_slots_update_clog_v3.patch

> 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.

Note - I have used attached patch transaction_burner_v1.patch (extracted
from Jeff's patch upthread) to verify the transactions that fall into
different
page boundaries.

On Sat, Dec 12, 2015 at 8:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Thu, Dec 17, 2015 at 12:01 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sat, Dec 12, 2015 at 8:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>
>
Unfortunately, I didn't have access to high end Intel m/c on which I took
the performance data last time, so I took on Power-8 m/c where I/O
sub-system is not that good, so the write performance data at lower scale
factor like 300 is reasonably good and at higher scale factor (>= 1000)
it is mainly I/O bound, so there is not much difference with or without
patch.

Performance Data
-----------------------------
M/c configuration:
IBM POWER-8 24 cores, 192 hardware threads
RAM = 492GB

Non-default parameters
------------------------------------
max_connections = 300
shared_buffers=32GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout =35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB

Attached files show the performance data with both the patches at
scale factor 300 and 1000.

Read Patch-1 and Patch-2 in graphs as below:

Patch-1 - group_update_clog_v3.patch
Patch-2 - group_slots_update_v3.patch

Observations
----------------------
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.

> 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?

> 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.

> 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.
>
>
Got it, I will do so.

Let me know what you think about whether we need to proceed with slots
approach and try some more performance data?

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

On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

> and then
> start all of these identifiers with clogGroup as you propose.
>

I have changed them accordingly in the attached patch
(group_update_clog_v4.patch) and addressed other comments given by
you.

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

On Mon, Dec 21, 2015 at 1:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Tue, Dec 22, 2015 at 10:43 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Dec 21, 2015 at 1:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> >>
> >> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

> Otherwise it might be
> confused with some other time of grouping of PGPROCs.
>

Won't procArray prefix distinguish it from other type of groupings?

About CLogControlLock patch, yesterday I noticed that SLRU code
can return error in some cases which can lead to hang for group
members, as once group leader errors out, there is no one to wake
them up. However on further looking into code, I found out that
this path (TransactionIdSetPageStatus()) is always called in critical
section (RecordTransactionCommit() ensures the same), so any
ERROR in this path will be converted to PANIC which don't require
any wakeup mechanism for group members. In any case, if you
find any other way which can lead to error (not being converted to
PANIC), I have already handled the error case in the attached patch
(group_update_clog_error_handling_v4.patch) and if you also don't
find any case, then previous patch stands good.

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

On Wed, Dec 23, 2015 at 6:16 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Dec 22, 2015 at 10:43 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Mon, Dec 21, 2015 at 1:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> > On Fri, Dec 18, 2015 at 9:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
>> > wrote:
>> >>
>> >> On Fri, Dec 18, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Fri, Dec 25, 2015 at 6:36 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Dec 23, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

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

On 7 January 2016 at 05:24, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Dec 25, 2015 at 6:36 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Wed, Dec 23, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 7:26 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>
> On 7 January 2016 at 05:24, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Fri, Dec 25, 2015 at 6:36 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> >>
> >> On Wed, Dec 23, 2015 at 1:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
>

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?

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

On Tue, Feb 9, 2016 at 11:14 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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(at)gmail(dot)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 Thu, Feb 11, 2016 at 8:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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.
>

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
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.

Do you have anything else in mind?

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

On Fri, Feb 12, 2016 at 12:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Sat, Feb 13, 2016 at 10:10 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Feb 12, 2016 at 12:55 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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".
>

Yes.

> > 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.
>

Okay, I have updated the comments for such a possibility and the
possible improvement, if we ever face such a situation. I also
once again verified that even if group contains transaction status for
multiple pages, it works fine.

Performance data with attached patch is as below.

M/c configuration
-----------------------------
RAM - 500GB
8 sockets, 64 cores(Hyperthreaded128 threads total)

Non-default parameters
------------------------------------
max_connections = 300
shared_buffers=8GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout =35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB

Client_Count/Patch_Ver 1 64 128 256
HEAD(481725c0) 963 28145 28593 26447
Patch-1 938 28152 31703 29402

We can see 10~11% performance improvement as observed
previously. You might see 0.02% performance difference with
patch as regression, but that is just a run-to-run variation.

Note - To take this performance data, I have to revert commit
ac1d7945 which is known issue in HEAD as reported here [1].

[1] -
http://www.postgresql.org/message-id/CAB-SwXZh44_2ybvS5Z67p_CDz=XFn4hNAD=CnMEF+QqkXwFrGg@mail.gmail.com

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

On Sun, Feb 21, 2016 at 10:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

>
> Client_Count/Patch_Ver 1 64 128 256
> HEAD(481725c0) 963 28145 28593 26447
> Patch-1 938 28152 31703 29402
>
>
> We can see 10~11% performance improvement as observed
> previously. You might see 0.02% performance difference with
> patch as regression, but that is just a run-to-run variation.
>

Don't the single-client numbers show about a 3% regresssion? Surely not
0.02%.

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

On Sun, Feb 21, 2016 at 12:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, Feb 21, 2016 at 10:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>>
>> Client_Count/Patch_Ver 1 64 128 256
>> HEAD(481725c0) 963 28145 28593 26447
>> Patch-1 938 28152 31703 29402
>>
>>
>> We can see 10~11% performance improvement as observed
>> previously. You might see 0.02% performance difference with
>> patch as regression, but that is just a run-to-run variation.
>>
>
> Don't the single-client numbers show about a 3% regresssion? Surely not
> 0.02%.
>

Sorry, you are right, it is ~2.66%, but in read-write pgbench tests, I
could see such fluctuation. Also patch doesn't change single-client
case. However, if you still feel that there could be impact by patch,
I can re-run the single client case once again with different combinations
like first with HEAD and then patch and vice versa.

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

On Sun, Feb 21, 2016 at 12:24 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Sun, Feb 21, 2016 at 12:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
>
>> On Sun, Feb 21, 2016 at 10:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>>
>>>
>>> Client_Count/Patch_Ver 1 64 128 256
>>> HEAD(481725c0) 963 28145 28593 26447
>>> Patch-1 938 28152 31703 29402
>>>
>>>
>>> We can see 10~11% performance improvement as observed
>>> previously. You might see 0.02% performance difference with
>>> patch as regression, but that is just a run-to-run variation.
>>>
>>
>> Don't the single-client numbers show about a 3% regresssion? Surely not
>> 0.02%.
>>
>
>
> Sorry, you are right, it is ~2.66%, but in read-write pgbench tests, I
> could see such fluctuation. Also patch doesn't change single-client
> case. However, if you still feel that there could be impact by patch,
> I can re-run the single client case once again with different combinations
> like first with HEAD and then patch and vice versa.
>

Are these results from a single run, or median-of-three?

I mean, my basic feeling is that I would not accept a 2-3% regression in
the single client case to get a 10% speedup in the case where we have 128
clients. A lot of people will not have 128 clients; quite a few will have
a single session, or just a few. Sometimes just making the code more
complex can hurt performance in subtle ways, e.g. by making it fit into the
L1 instruction cache less well. If the numbers you have here are accurate,
I'd vote to reject the patch.

Note that we already have apparently regressed single-client performance
noticeably between 9.0 and 9.5:

http://bonesmoses.org/2016/01/08/pg-phriday-how-far-weve-come/

I bet that wasn't a single patch but a series of patches which made things
more complex to improve concurrency behavior, but in the process each one
made the single-client case a tiny bit slower. In the end, that adds up.
I think we need to put some effort into figuring out if there is a way we
can get some of that single-client performance (and ideally more) back.

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

On Sun, Feb 21, 2016 at 2:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, Feb 21, 2016 at 12:24 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> On Sun, Feb 21, 2016 at 12:02 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
>> wrote:
>>
>>> On Sun, Feb 21, 2016 at 10:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>>> wrote:
>>>
>>>>
>>>> Client_Count/Patch_Ver 1 64 128 256
>>>> HEAD(481725c0) 963 28145 28593 26447
>>>> Patch-1 938 28152 31703 29402
>>>>
>>>>
>>>> We can see 10~11% performance improvement as observed
>>>> previously. You might see 0.02% performance difference with
>>>> patch as regression, but that is just a run-to-run variation.
>>>>
>>>
>>> Don't the single-client numbers show about a 3% regresssion? Surely not
>>> 0.02%.
>>>
>>
>>
>> Sorry, you are right, it is ~2.66%, but in read-write pgbench tests, I
>> could see such fluctuation. Also patch doesn't change single-client
>> case. However, if you still feel that there could be impact by patch,
>> I can re-run the single client case once again with different combinations
>> like first with HEAD and then patch and vice versa.
>>
>
> Are these results from a single run, or median-of-three?
>
>
This was median-of-three, but the highest tps with patch is 1119
and with HEAD, it is 969 which shows a gain at single client-count.
Sometimes, I see such differences, it could be due to auto vacuum
getting triggered at some situations which lead to such variations.
However, if I try 2-3 times, the difference generally gets disappeared
unless there is some real regression or if just switch off auto vacuum
and do manual vacuum after each run. This time, I haven't run the
tests multiple times.

> I mean, my basic feeling is that I would not accept a 2-3% regression in
> the single client case to get a 10% speedup in the case where we have 128
> clients.
>

I understand your point. I think to verify whether it is run-to-run
variation or an actual regression, I will re-run these tests on single
client multiple times and post the result.

> A lot of people will not have 128 clients; quite a few will have a
> single session, or just a few. Sometimes just making the code more complex
> can hurt performance in subtle ways, e.g. by making it fit into the L1
> instruction cache less well. If the numbers you have here are accurate,
> I'd vote to reject the patch.
>
>
One point to note is that this patch along with first patch which I
posted in this thread to increase clog buffers can make significant
reduction in contention on CLogControlLock. OTOH, I think introducing
regression at single-client is also not a sane thing to do, so lets
first try to find if there is actually any regression and if it is, can
we mitigate it by writing code with somewhat fewer instructions or
in a slightly different way and then we can decide whether it is good
to reject the patch or not. Does that sound reasonable to you?

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

On Sun, Feb 21, 2016 at 7:45 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> I mean, my basic feeling is that I would not accept a 2-3% regression in
>> the single client case to get a 10% speedup in the case where we have 128
>> clients.
>>
>
> I understand your point. I think to verify whether it is run-to-run
> variation or an actual regression, I will re-run these tests on single
> client multiple times and post the result.
>

Perhaps you could also try it on a couple of different machines (e.g.
MacBook Pro and a couple of different large servers).

>
> A lot of people will not have 128 clients; quite a few will have a
>> single session, or just a few. Sometimes just making the code more complex
>> can hurt performance in subtle ways, e.g. by making it fit into the L1
>> instruction cache less well. If the numbers you have here are accurate,
>> I'd vote to reject the patch.
>>
> One point to note is that this patch along with first patch which I
> posted in this thread to increase clog buffers can make significant
> reduction in contention on CLogControlLock. OTOH, I think introducing
> regression at single-client is also not a sane thing to do, so lets
> first try to find if there is actually any regression and if it is, can
> we mitigate it by writing code with somewhat fewer instructions or
> in a slightly different way and then we can decide whether it is good
> to reject the patch or not. Does that sound reasonable to you?
>

Yes.

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

On Tue, Feb 23, 2016 at 7:06 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, Feb 21, 2016 at 7:45 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>
>> I mean, my basic feeling is that I would not accept a 2-3% regression in
>>> the single client case to get a 10% speedup in the case where we have 128
>>> clients.
>>>
>>
>>
When I tried by running the pgbench first with patch and then with Head, I
see 1.2% performance increase with patch. TPS with patch is 976 and with
Head it is 964. For 3, 30 mins TPS data, refer "Patch –
group_clog_update_v5" and before that "HEAD – Commit 481725c0"
in perf_write_clogcontrollock_data_v6.ods attached with this mail.

Nonetheless, I have observed that below new check has been added by the
patch which can effect single client performance. So I have changed it
such that new check is done only when we there is actually a need of group
update which means when multiple clients tries to update clog at-a-time.

+ if (!InRecovery &&
+ all_trans_same_page &&
+ nsubxids < PGPROC_MAX_CACHED_SUBXIDS &&
+ !IsGXactActive())

> I understand your point. I think to verify whether it is run-to-run
>> variation or an actual regression, I will re-run these tests on single
>> client multiple times and post the result.
>>
>
> Perhaps you could also try it on a couple of different machines (e.g.
> MacBook Pro and a couple of different large servers).
>

Okay, I have tried latest patch (group_update_clog_v6.patch) on 2 different
big servers and then on Mac-Pro. The detailed data for various runs can be
found in attached document perf_write_clogcontrollock_data_v6.ods. I have
taken the performance data for higher client-counts with somewhat larger
scale factor (1000) and data for median of same is as below:

M/c configuration
-----------------------------
RAM - 500GB
8 sockets, 64 cores(Hyperthreaded128 threads total)

Non-default parameters
------------------------------------
max_connections = 1000
shared_buffers=32GB
min_wal_size=10GB
max_wal_size=15GB
checkpoint_timeout =35min
maintenance_work_mem = 1GB
checkpoint_completion_target = 0.9
wal_buffers = 256MB

Client_Count/Patch_ver 1 8 64 128 256
HEAD 871 5090 17760 17616 13907
PATCH 900 5110 18331 20277 19263

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.

Thanks to Dilip Kumar for helping me in conducting test of this patch on
MacBook-Pro.

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

On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

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

On Fri, Feb 26, 2016 at 11:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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 Mon, Feb 29, 2016 at 11:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Feb 26, 2016 at 11:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

I am not sure if I understood your question, do you want to know why at the
first place transactions spanning more than one-page call the
function TransactionIdSetPageStatus()? If we want to avoid trying the
transaction status update when they are on different page, then I think we
need some
major changes in TransactionIdSetTreeStatus().

> 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.
>

In my tests, the check related to same page
in TransactionGroupUpdateXidStatus() doesn't impact performance in any way
and I think the reason is that, it happens rarely that group contain
multiple pages and even it occurs, there is hardly much impact. So, I will
remove that check and I think thats what you also want for now.

> + /*
> + * 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.
>

Okay, will change.

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

On Mon, Feb 29, 2016 at 11:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Feb 26, 2016 at 11:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?
>

This is to save the case when sub-transactions belonging to a transaction
are on different pages, and the reason for same is that currently I am
using XidCache as stored in each proc to pass the information of
subtransactions to TransactionIdSetPageStatusInternal(), now if we allow
subtransactions from different pages then I need to extract subxid's from
that cache which belong to the page on which we are trying to update the
status. Now this will add few more cycles in the code path under
ExclusiveLock without any clear benefit, thats why I have not implemented
it. I have explained the same in code comments as well:

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 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.

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

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

> On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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(at)pgmasters(dot)net

On Tue, Mar 15, 2016 at 12:00 AM, David Steele <david(at)pgmasters(dot)net> wrote:
>
> On 2/26/16 11:37 PM, Amit Kapila wrote:
>
>> On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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
>

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.

>
> 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.
>

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.

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

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

> On Tue, Mar 15, 2016 at 12:00 AM, David Steele <david(at)pgmasters(dot)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(at)pgmasters(dot)net

On Tue, Mar 15, 2016 at 7:54 PM, David Steele <david(at)pgmasters(dot)net> wrote:
>
> On 3/15/16 1:17 AM, Amit Kapila wrote:
>
> > On Tue, Mar 15, 2016 at 12:00 AM, David Steele <david(at)pgmasters(dot)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).
>

Thanks for verification.

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

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

On Wed, Mar 16, 2016 at 11:57 PM, Jesper Pedersen <
jesper(dot)pedersen(at)redhat(dot)com> wrote:
>
> 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.
>

Can you provide the details of test, like is this pgbench read-write test
and if possible steps for doing test execution.

I wonder if you can do the test with unlogged tables (if you are using
pgbench, then I think you need to change the Create Table command to use
Unlogged option).

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

Can you provide CPU information (probably by using lscpu).

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

On Thu, Mar 17, 2016 at 11:39 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

I have reviewed the patch.. here are some review comments, I will continue
to review..

1.

+
+ /*
+ * Add the proc to list, if the clog page where we need to update the

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

Should we clear all these structure variable what we set above in case we
are not adding our self to group, I can see it will not have any problem
even if we don't clear them,
I think if we don't want to clear we can add some comment mentioning the
same.

+ proc->clogGroupMember = true;
+ proc->clogGroupMemberXid = xid;
+ proc->clogGroupMemberXidStatus = status;
+ proc->clogGroupMemberPage = pageno;
+ proc->clogGroupMemberLsn = lsn;

2.

Here we are updating in our own proc, I think we don't need atomic
operation here, we are not yet added to the list.

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

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

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(). Going for 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 Tue, Mar 22, 2016 at 4:22 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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.
>

Okay, I will try to do so in next version.

> 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. Now, I am not denying from the fact that there will
be cases where subtransactions won't fall into different pages, but I think
chances of such transactions to participate in group mode will be less and
this patch is mainly targeting scalability for short transactions.

>
> > * 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(). 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.

> 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
>

Considering above reply to (a), do you want to see it done as a separate
else if loop in patch?

>
> > +
> > +/*
> > + * 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?
>

Why this won't lead to ABA problem is explained below in comments. Refer

+ /*

+ * 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.

+ */

>
> > 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.
>

I think it won't create a problem, but surely it is not good to return as
true, will change in next version of patch.

>
> > + 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.
>

It will not matter because we are waking the queued process only once we
are done with xid status update.

>
> 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?

>
> > + /*
> > + * 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?
>

We need to remember the head of list to wake up the processes due to which
I think above loop is required.

>
> > 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.
>

I have done considering this as a hot-path to save an additional function
call, but can change if you think so.

>
> > +#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.
>

Yes, thats the main case which will be targeted by this patch and I think
there are many such cases in OLTP workloads where there are very short
transactions.

>
> 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?
>

This is mainly to save the case where multiple pages are not-in-memory and
leader needs to perform the I/O serially. Refer mail [2] for point raised
by Robert.

> 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.

[1] -
http://www.postgresql.org/message-id/CAA4eK1KUVPxBcGTdOuKyvf5p1sQ0HeUbSMbTxtQc=P65OxiZog@mail.gmail.com
[2] -
http://www.postgresql.org/message-id/CA+TgmoahCx6XgprR=p5==cF0g9uhSHsJxVdWdUEHN9H2Mv0gkw@mail.gmail.com

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

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:29 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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?
>

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.

I will address your other review comments and send an updated patch.

Thanks for the review.

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

On Tue, Mar 22, 2016 at 6:52 AM, Andres Freund <andres(at)anarazel(dot)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(at)anarazel(dot)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 Tue, Mar 22, 2016 at 4:22 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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.
>

Okay, I have simplified the sentences in the comment.

>
>
> > * 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(). Going for 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
>

Changed.

>
>
> > 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.
>

Okay, I have tried to simplify the comment as well.

>
> > + 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.
>

Changed as per suggestion.

>
> > + 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.
>

Changed as per suggestion.

>
> > + /*
> > + * 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?
>

Changed as per suggestion.

>
> 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.

Attached patch contains all the changes suggested by you. Let me know if I
have missed anything or you want it differently.

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

On Wed, Mar 23, 2016 at 12:26 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Tue, Mar 22, 2016 at 4:22 PM, Andres Freund <andres(at)anarazel(dot)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.

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

On Tue, Mar 22, 2016 at 12:33 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
>
> On Thu, Mar 17, 2016 at 11:39 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> I have reviewed the patch.. here are some review comments, I will
continue to review..
>
> 1.
>
> +
> + /*
> + * Add the proc to list, if the clog page where we need to update the
>
> + */
> + if (nextidx != INVALID_PGPROCNO &&
> + ProcGlobal->allProcs[nextidx].clogGroupMemberPage !=
proc->clogGroupMemberPage)
> + return false;
>
> Should we clear all these structure variable what we set above in case we
are not adding our self to group, I can see it will not have any problem
even if we don't clear them,
> I think if we don't want to clear we can add some comment mentioning the
same.
>

I have updated the patch to just clear clogGroupMember as that is what is
done when we wake the processes.

>
> 2.
>
> Here we are updating in our own proc, I think we don't need atomic
operation here, we are not yet added to the list.
>
> + if (nextidx != INVALID_PGPROCNO &&
> + ProcGlobal->allProcs[nextidx].clogGroupMemberPage !=
proc->clogGroupMemberPage)
> + return false;
> +
> + pg_atomic_write_u32(&proc->clogGroupNext, nextidx);
>
>

We won't be able to assign nextidx to clogGroupNext is of
type pg_atomic_uint32.

Thanks for the review.

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

On 2016-03-23 12:33:22 +0530, Amit Kapila wrote:
> On Wed, Mar 23, 2016 at 12:26 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> >
> > On Tue, Mar 22, 2016 at 4:22 PM, Andres Freund <andres(at)anarazel(dot)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 Thu, Mar 24, 2016 at 5:40 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> 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.
>

I have seen smaller benefits at 300 scale factor and somewhat larger
benefits at 1000 scale factor. Also Mithun has done similar testing with
unlogged tables and the results of same [1] also looks good.

>
> Which made me think. Running
> a bigger pgbench showed that most the interesting (i.e. long) lock waits
> were both via TransactionIdSetPageStatus *and* TransactionIdGetStatus().
>

Yes, this is same what I have observed as well.

>
> 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.
>

I have mainly done it with half-hour read-write tests. What do you want to
observe via smaller tests, sometimes it gives inconsistent data for
read-write tests?

>
> 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).
>

I have tried this patch on mainly 8 socket machine with 300 & 1000 scale
factor. I am hoping that you have tried this test on unlogged tables and
by the way at what client count, you have seen these results.

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

One difference was that I think Jesper has done testing with
synchronous_commit mode as off whereas my tests were with synchronous
commit mode on.

>
> 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().
>

I think we can try that as well and if you see better results by that
Approach, then we can use that instead of current patch.

[1] -
http://www.postgresql.org/message-id/CAD__OujrdwQdJdoVHahQLDg-6ivu6iBCi9iJ1qPu6AtUQpL4UQ@mail.gmail.com

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

On Thu, Mar 24, 2016 at 8:08 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Thu, Mar 24, 2016 at 5:40 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > 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).
> >
>

One more point, I wanted to say here which is that I think the benefit will
be shown mainly when the ClogControlLock has contention more than or near
to ProcArrayLock, otherwise even if patch reduces contention (you can see
via LWLock stats), the performance doesn't increase. From Mithun's data
[1], related to LWLocks, it seems like at 88 clients in his test, the
contention on CLOGControlLock becomes more than ProcArrayLock and that is
the point where it has started showing noticeable performance gain. I have
explained some more on that thread [2] about this point. Is it possible
for you to once test in similar situation and see the behaviour (like for
client count greater than number of cores) w.r.t locking contention and TPS.

[1] -
http://www.postgresql.org/message-id/CAD__Ouh6PahJ+q1mzzjDzLo4v9GjyufegtJNAyXc0_Lfh-4coQ@mail.gmail.com
[2] -
http://www.postgresql.org/message-id/CAA4eK1LBOQ4e3Ycge+Fe0euzVu89CqGTuGNeajOienxJR0AEKA@mail.gmail.com

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

On Thu, Mar 24, 2016 at 8:08 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> On Thu, Mar 24, 2016 at 5:40 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > 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.
> >
>
> I have mainly done it with half-hour read-write tests. What do you want
to observe via smaller tests, sometimes it gives inconsistent data for
read-write tests?
>