CLOG contention, part 2

On Sun, Jan 8, 2012 at 2:25 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

Why do we need this in 9.2?

We now have clog_buffers = 32 and we have write rates ~16,000 tps. At
those write rates we fill a clog buffer every 2 seconds, so the clog
cache completely churns every 64 seconds.

If we wish to achieve those rates in the real world, any access to
data that was written by a transaction more than a minute ago will
cause clog cache page faults, leading to stalls in new transactions.

To avoid those problems we need
* background writing of the clog LRU (already posted as a separate patch)
* a way of separating access to historical data from the main commit
path (this patch)

And to evaluate such situations, we need a way to simulate data that
contains many transactions. 32 buffers can hold just over 1 million
transaction ids, so benchmarks against databases containing > 10
million separate transactions are recommended (remembering that this
is just 10 mins of data on high TPS systems). A pgbench patch is
provided separately to aid in the evaluation.

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

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

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

On Fri, Jan 20, 2012 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

OT: It would save lots of time if we had 2 things for the CF app:

1. Emails that go to appropriate people when status changes. e.g. when
someone sets "Waiting for Author" the author gets an email so they
know the reviewer is expecting something. No knowing that wastes lots
of days, so if we want to do this in less days that seems like a great
place to start.

2. Something that automatically tests patches. If you submit a patch
we run up a blank VM and run patch applies on all patches. As soon as
we get a fail, an email goes to patch author. That way authors know as
soon as a recent commit invalidates something.

Those things have wasted time for me in the past, so they're
opportunities to improve the process, not must haves.

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

On Fri, Jan 20, 2012 at 9:44 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 20, 2012 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

OT: It would save lots of time if we had 2 things for the CF app:

1. Emails that go to appropriate people when status changes. e.g. when
someone sets "Waiting for Author" the author gets an email so they
know the reviewer is expecting something. No knowing that wastes lots
of days, so if we want to do this in less days that seems like a great
place to start.

2. Something that automatically tests patches. If you submit a patch
we run up a blank VM and run patch applies on all patches. As soon as
we get a fail, an email goes to patch author. That way authors know as
soon as a recent commit invalidates something.

Those things have wasted time for me in the past, so they're
opportunities to improve the process, not must haves.

Yeah, I agree that that would be nice. I just haven't had time to
implement much of anything for the CF application in a long time. My
management has been very interested in the performance and scalability
stuff, so that's been my main focus for 9.2. I'm going to see if I
can carve out some time for this once the dust settles.

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

On Fri, Jan 20, 2012 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

Still applies and compiles cleanly for me.

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

On Fri, Jan 20, 2012 at 10:16 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 20, 2012 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

Still applies and compiles cleanly for me.

D'oh. You're right. Looks like I accidentally tried to apply this to
the 9.1 sources. Sigh...

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

On Fri, Jan 20, 2012 at 3:32 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jan 20, 2012 at 10:16 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 20, 2012 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

Still applies and compiles cleanly for me.

D'oh.  You're right.  Looks like I accidentally tried to apply this to
the 9.1 sources.  Sigh...

No worries. It's Friday.

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

On Fri, Jan 20, 2012 at 10:38 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 20, 2012 at 3:32 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jan 20, 2012 at 10:16 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 20, 2012 at 1:37 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Jan 8, 2012 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

I've taken that idea and used it to build a second Clog cache, known
as ClogHistory which allows access to the read-only tail of pages in
the clog. Once a page has been written to for the last time, it will
be accessed via the ClogHistory Slru in preference to the normal Clog
Slru. This separates historical accesses by readers from current write
access by committers. Historical access doesn't force dirty writes,
nor are commits made to wait when historical access occurs.

This seems to need a rebase.

Still applies and compiles cleanly for me.

D'oh.  You're right.  Looks like I accidentally tried to apply this to
the 9.1 sources.  Sigh...

No worries. It's Friday.

http://www.youtube.com/watch?v=kfVsfOSbJY0

Of course, I even ran git log to check that I had the latest
sources... but what I had, of course, was the latest 9.1 sources,
which still have recently-timestamped commits, and I didn't look
carefully enough. Sigh.

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

On Fri, Jan 20, 2012 at 10:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:

D'oh.  You're right.  Looks like I accidentally tried to apply this to
the 9.1 sources.  Sigh...

No worries. It's Friday.

Server passed 'make check' with this patch, but when I tried to fire
it up for some test runs, it fell over with:

FATAL: no more LWLockIds available

I assume that it must be dependent on the config settings used. Here are mine:

shared_buffers = 8GB
maintenance_work_mem = 1GB
synchronous_commit = off
checkpoint_segments = 300
checkpoint_timeout = 15min
checkpoint_completion_target = 0.9
wal_writer_delay = 20ms

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

On Sat, Jan 21, 2012 at 1:57 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jan 20, 2012 at 10:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:

D'oh.  You're right.  Looks like I accidentally tried to apply this to
the 9.1 sources.  Sigh...

No worries. It's Friday.

Server passed 'make check' with this patch, but when I tried to fire
it up for some test runs, it fell over with:

FATAL:  no more LWLockIds available

I assume that it must be dependent on the config settings used.  Here are mine:

shared_buffers = 8GB
maintenance_work_mem = 1GB
synchronous_commit = off
checkpoint_segments = 300
checkpoint_timeout = 15min
checkpoint_completion_target = 0.9
wal_writer_delay = 20ms

Yes, it was. Sorry about that. New version attached, retesting while
you read this.

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

On Fri, Jan 20, 2012 at 6:44 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

OT: It would save lots of time if we had 2 things for the CF app:

..

2. Something that automatically tests patches. If you submit a patch
we run up a blank VM and run patch applies on all patches. As soon as
we get a fail, an email goes to patch author. That way authors know as
soon as a recent commit invalidates something.

Well, first the CF app would need to reliably be able to find the
actual patch. That is currently not a given.

Also, it seems that OID collisions are a dime a dozen, and I'm
starting to doubt that they are even worth reporting in the absence of
a more substantive review. And in the patches I've looked at, it
seems like the OID is not even cross-referenced anywhere else in the
patch, the cross-references are all based on symbolic names. I freely
admit I have no idea what I am talking about, but it seems like the
only purpose of OIDs is to create bit rot.

Cheers,

Jeff

On Sat, Jan 21, 2012 at 7:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Yes, it was. Sorry about that. New version attached, retesting while
you read this.

In my hands I could never get this patch to do anything. The new
cache was never used.

I think that that was because RecentXminPageno never budged from -1.

I think that that, in turn, is because the comparison below can never
return true, because the comparison is casting both sides to uint, and
-1 cast to uint is very large

/* When we commit advance ClogCtl's shared RecentXminPageno if needed */
if (ClogCtl->shared->RecentXminPageno < TransactionIdToPage(RecentXmin))
ClogCtl->shared->RecentXminPageno =
TransactionIdToPage(RecentXmin);

Also, I think the general approach is wrong. The only reason to have
these pages in shared memory is that we can control access to them to
prevent write/write and read/write corruption. Since these pages are
never written, they don't need to be in shared memory. Just read
each page into backend-local memory as it is needed, either
palloc/pfree each time or using a single reserved block for the
lifetime of the session. Let the kernel worry about caching them so
that the above mentioned reads are cheap.

Cheers,

Jeff

On Fri, Jan 27, 2012 at 4:05 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

Also, I think the general approach is wrong.  The only reason to have
these pages in shared memory is that we can control access to them to
prevent write/write and read/write corruption.  Since these pages are
never written, they don't need to be in shared memory.   Just read
each page into backend-local memory as it is needed, either
palloc/pfree each time or using a single reserved block for the
lifetime of the session.  Let the kernel worry about caching them so
that the above mentioned reads are cheap.

right -- exactly. but why stop at one page?

merlin

On Fri, Jan 27, 2012 at 3:16 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

On Fri, Jan 27, 2012 at 4:05 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

Also, I think the general approach is wrong.  The only reason to have
these pages in shared memory is that we can control access to them to
prevent write/write and read/write corruption.  Since these pages are
never written, they don't need to be in shared memory.   Just read
each page into backend-local memory as it is needed, either
palloc/pfree each time or using a single reserved block for the
lifetime of the session.  Let the kernel worry about caching them so
that the above mentioned reads are cheap.

right -- exactly.  but why stop at one page?

If you have more than one, you need code to decide which one to evict
(just free) every time you need a new one. And every process needs to
be running this code, while the kernel is still going to need make its
own decisions for the entire system. It seems simpler to just let the
kernel do the job for everyone. Are you worried that a read syscall
is going to be slow even when the data is presumably cached in the OS?

Cheers,

Jeff

On Fri, Jan 27, 2012 at 10:05 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Sat, Jan 21, 2012 at 7:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Yes, it was. Sorry about that. New version attached, retesting while
you read this.

In my hands I could never get this patch to do anything.  The new
cache was never used.

I think that that was because RecentXminPageno never budged from -1.

I think that that, in turn, is because the comparison below can never
return true, because the comparison is casting both sides to uint, and
-1 cast to uint is very large

       /* When we commit advance ClogCtl's shared RecentXminPageno if needed */
       if (ClogCtl->shared->RecentXminPageno < TransactionIdToPage(RecentXmin))
                ClogCtl->shared->RecentXminPageno =
TransactionIdToPage(RecentXmin);

Thanks, will look again.

Also, I think the general approach is wrong.  The only reason to have
these pages in shared memory is that we can control access to them to
prevent write/write and read/write corruption.  Since these pages are
never written, they don't need to be in shared memory.   Just read
each page into backend-local memory as it is needed, either
palloc/pfree each time or using a single reserved block for the
lifetime of the session.  Let the kernel worry about caching them so
that the above mentioned reads are cheap.

Will think on that.

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

On Sat, Jan 28, 2012 at 1:52 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

Also, I think the general approach is wrong.  The only reason to have
these pages in shared memory is that we can control access to them to
prevent write/write and read/write corruption.  Since these pages are
never written, they don't need to be in shared memory.   Just read
each page into backend-local memory as it is needed, either
palloc/pfree each time or using a single reserved block for the
lifetime of the session.  Let the kernel worry about caching them so
that the above mentioned reads are cheap.

Will think on that.

For me, there are arguments both ways as to whether it should be in
shared or local memory.

The one factor that makes the answer "shared" for me is that its much
easier to reuse existing SLRU code. We dont need to invent a new way
of cacheing/access etc. We just rewire what we already have. So
overall, the local/shared debate is much less important that the
robustness/code reuse angle. That's what makes this patch fairly
simple.

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

On Fri, Jan 27, 2012 at 10:05 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Sat, Jan 21, 2012 at 7:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Yes, it was. Sorry about that. New version attached, retesting while
you read this.

In my hands I could never get this patch to do anything.  The new
cache was never used.

I think that that was because RecentXminPageno never budged from -1.

I think that that, in turn, is because the comparison below can never
return true, because the comparison is casting both sides to uint, and
-1 cast to uint is very large

       /* When we commit advance ClogCtl's shared RecentXminPageno if needed */
       if (ClogCtl->shared->RecentXminPageno < TransactionIdToPage(RecentXmin))
                ClogCtl->shared->RecentXminPageno =
TransactionIdToPage(RecentXmin);

Thanks for looking at the patch.

The patch works fine. RecentXminPageno does move forwards as it is
supposed to and there are no uints anywhere in that calculation.

The pageno only moves forwards every 32,000 transactions, so I'm
guessing that your testing didn't go on for long enough to show it
working correctly.

As regards to effectiveness, you need to execute more than 1 million
transactions before the main clog cache fills, which might sound a
lot, but its approximately 1 minute of heavy transactions at the
highest rate Robert has published.

I've specifically designed the pgbench changes required to simulate
conditions of clog contention to help in the evaluation of this patch.

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

On Sun, Jan 29, 2012 at 12:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 27, 2012 at 10:05 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Sat, Jan 21, 2012 at 7:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Yes, it was. Sorry about that. New version attached, retesting while
you read this.

In my hands I could never get this patch to do anything.  The new
cache was never used.

I think that that was because RecentXminPageno never budged from -1.

I think that that, in turn, is because the comparison below can never
return true, because the comparison is casting both sides to uint, and
-1 cast to uint is very large

       /* When we commit advance ClogCtl's shared RecentXminPageno if needed */
       if (ClogCtl->shared->RecentXminPageno < TransactionIdToPage(RecentXmin))
                ClogCtl->shared->RecentXminPageno =
TransactionIdToPage(RecentXmin);

Thanks for looking at the patch.

The patch works fine. RecentXminPageno does move forwards as it is
supposed to and there are no uints anywhere in that calculation.

Maybe it is system dependent. Or, are you running this patch on top
of some other uncommitted patch (other than the pgbench one)?

RecentXmin is a TransactionID, which is a uint32.
I think the TransactionIdToPage macro preserves that.

If I cast to a int, then I see advancement:

if (ClogCtl->shared->RecentXminPageno < (int) TransactionIdToPage(RecentXmin))

...

I've specifically designed the pgbench changes required to simulate
conditions of clog contention to help in the evaluation of this patch.

Yep, I've used that one for the testing.

Cheers,

Jeff

On Sun, Jan 29, 2012 at 1:41 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Sun, Jan 29, 2012 at 12:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Fri, Jan 27, 2012 at 10:05 PM, Jeff Janes <jeff.janes@gmail.com> wrote:

On Sat, Jan 21, 2012 at 7:31 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Yes, it was. Sorry about that. New version attached, retesting while
you read this.

In my hands I could never get this patch to do anything.  The new
cache was never used.

I think that that was because RecentXminPageno never budged from -1.

I think that that, in turn, is because the comparison below can never
return true, because the comparison is casting both sides to uint, and
-1 cast to uint is very large

       /* When we commit advance ClogCtl's shared RecentXminPageno if needed */
       if (ClogCtl->shared->RecentXminPageno < TransactionIdToPage(RecentXmin))
                ClogCtl->shared->RecentXminPageno =
TransactionIdToPage(RecentXmin);

Thanks for looking at the patch.

The patch works fine. RecentXminPageno does move forwards as it is
supposed to and there are no uints anywhere in that calculation.

Maybe it is system dependent.  Or, are you running this patch on top
of some other uncommitted patch (other than the pgbench one)?

RecentXmin is a TransactionID, which is a uint32.
I think the TransactionIdToPage macro preserves that.

If I cast to a int, then I see advancement:

if (ClogCtl->shared->RecentXminPageno < (int) TransactionIdToPage(RecentXmin))

And to clarify, if I don't do the cast, I don't see advancement, using
this code:

elog(LOG, "JJJ RecentXminPageno %d, %d",
ClogCtl->shared->RecentXminPageno , TransactionIdToPage(RecentXmin));
if (ClogCtl->shared->RecentXminPageno <
TransactionIdToPage(RecentXmin))
ClogCtl->shared->RecentXminPageno =
TransactionIdToPage(RecentXmin);

Then using your pgbench -I -s 100 -c 8 -j8, I get tons of log entries like:

LOG: JJJ RecentXminPageno -1, 149
STATEMENT: INSERT INTO pgbench_accounts (aid, bid, abalance) VALUES
(nextval('pgbench_accounts_load_seq'), 1 + (lastval()/(100000)), 0);

Cheers,

Jeff