Moving more work outside WALInsertLock
I've been looking at various ways to make WALInsertLock less of a
bottleneck on multi-CPU servers. The key is going to be to separate the
two things that are done while holding the WALInsertLock: a) allocating
the required space in the WAL, and b) calculating the CRC of the record
header and copying the data to the WAL page. a) needs to be serialized,
but b) could be done in parallel.
I've been experimenting with different approaches to do that, but one
thing is common among all of them: you need to know the total amount of
WAL space needed for the record, including backup blocks, before you
take the lock. So, here's a patch to move things around in XLogInsert()
a bit, to accomplish that.
This patch doesn't seem to have any performance or scalability impact. I
must admit I expected it to give a tiny gain in scalability by
shortening the time WALInsertLock is held by a few instructions, but I
can't measure any. But IMO it makes the code more readable, so this is
worthwhile for that reason alone.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
xloginsert-bkp-refactor-1.patchtext/x-diff; name=xloginsert-bkp-refactor-1.patchDownload+174-157
On Thursday, December 15, 2011 02:51:33 PM Heikki Linnakangas wrote:
I've been looking at various ways to make WALInsertLock less of a
bottleneck on multi-CPU servers. The key is going to be to separate the
two things that are done while holding the WALInsertLock: a) allocating
the required space in the WAL, and b) calculating the CRC of the record
header and copying the data to the WAL page. a) needs to be serialized,
but b) could be done in parallel.I've been experimenting with different approaches to do that, but one
thing is common among all of them: you need to know the total amount of
WAL space needed for the record, including backup blocks, before you
take the lock. So, here's a patch to move things around in XLogInsert()
a bit, to accomplish that.This patch doesn't seem to have any performance or scalability impact. I
must admit I expected it to give a tiny gain in scalability by
shortening the time WALInsertLock is held by a few instructions, but I
can't measure any. But IMO it makes the code more readable, so this is
worthwhile for that reason alone.
Thats great! I did (or at least tried) something similar when I was playing
around with another crc32 implementation (which I plan to finish sometime). My
changes where totally whacky but I got rather big improvements when changing
the crc computation from incremental to one big swoop.
I started to hack up an api which buffered xlog data in statically sized buffer
in each backend and only submitted that every now and then. Never got that to
actually work correctly in more than the simplest cases though ;). In many
cases were taking the wal insert lock way to often during a single insert...
(you obviously know that...)
Andres
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
I've been experimenting with different approaches to do that, but one
thing is common among all of them: you need to know the total amount of
WAL space needed for the record, including backup blocks, before you
take the lock. So, here's a patch to move things around in XLogInsert()
a bit, to accomplish that.
This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Please try again to explain what you're doing?
regards, tom lane
On Thu, Dec 15, 2011 at 7:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
I've been experimenting with different approaches to do that, but one
thing is common among all of them: you need to know the total amount of
WAL space needed for the record, including backup blocks, before you
take the lock. So, here's a patch to move things around in XLogInsert()
a bit, to accomplish that.This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Please try again to explain what you're doing?
Currently the CRC of all the data minus the header is computed outside the lock,
but then the header's computation is added and the CRC is finalized
inside the lock.
Cheers,
Jeff
Jeff Janes <jeff.janes@gmail.com> writes:
On Thu, Dec 15, 2011 at 7:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Please try again to explain what you're doing?
Currently the CRC of all the data minus the header is computed outside the lock,
but then the header's computation is added and the CRC is finalized
inside the lock.
Quite. AFAICS that is not optional, unless you are proposing to remove
the prev_link from the scope of the CRC, which is not exactly a
penalty-free change.
regards, tom lane
On 15.12.2011 18:48, Tom Lane wrote:
Jeff Janes<jeff.janes@gmail.com> writes:
On Thu, Dec 15, 2011 at 7:34 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Please try again to explain what you're doing?Currently the CRC of all the data minus the header is computed outside the lock,
but then the header's computation is added and the CRC is finalized
inside the lock.Quite. AFAICS that is not optional,
Right, my patch did not change that.
unless you are proposing to remove
the prev_link from the scope of the CRC, which is not exactly a
penalty-free change.
We could CRC the rest of the record header before getting the lock,
though, and only include the prev-link while holding the lock. I
micro-benchmarked that a little bit, but didn't see much benefit from
doing just that. Once you do more drastic changes so that the lock
doesn't need to be held while copying the data and calculating the CRC
of the record header, so that those things can be done in parallel, it
matters even less.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On 15.12.2011 17:34, Tom Lane wrote:
Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:
I've been experimenting with different approaches to do that, but one
thing is common among all of them: you need to know the total amount of
WAL space needed for the record, including backup blocks, before you
take the lock. So, here's a patch to move things around in XLogInsert()
a bit, to accomplish that.This patch may or may not be useful, but this description of it is utter
nonsense, because we already do compute that before taking the lock.
Nope. Without this patch, the total length including the backup blocks,
write_len, is added up in the loop that creates the rdata entries for
backup blocks. That is done while holding the lock (see code beginning
with comment "Make additional rdata chain entries for the backup blocks").
Admittedly you could sum up the total length quite easily in the earlier
loop, before we grab the lock, where we calculate the CRC of the backup
blocks ("Now add the backup block headers and data into the CRC"). That
would be a smaller patch.
Please try again to explain what you're doing?
Ok: I'm moving the creation of rdata entries for backup blocks outside
the critical section, so that it's done before grabbing the lock. I'm
also moving the CRC calculation so that it's done after all the rdata
entries have been created, including the ones for backup blocks. It's
more readable to do it that way, as a separate step, instead of
sprinkling the COMP_CRC macros in many places.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Thu, Dec 15, 2011 at 7:06 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Please try again to explain what you're doing?
Ok: I'm moving the creation of rdata entries for backup blocks outside the
critical section, so that it's done before grabbing the lock. I'm also
moving the CRC calculation so that it's done after all the rdata entries
have been created, including the ones for backup blocks. It's more readable
to do it that way, as a separate step, instead of sprinkling the COMP_CRC
macros in many places.
There's a comment that says we can't undo the linking of the rdata
chains, but it looks like a reversible process to me.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Dec 15, 2011 at 6:50 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
unless you are proposing to remove
the prev_link from the scope of the CRC, which is not exactly a
penalty-free change.We could CRC the rest of the record header before getting the lock, though,
and only include the prev-link while holding the lock. I micro-benchmarked
that a little bit, but didn't see much benefit from doing just that. Once
you do more drastic changes so that the lock doesn't need to be held while
copying the data and calculating the CRC of the record header, so that those
things can be done in parallel, it matters even less.
You missed your cue to discuss leaving the prev link out of the CRC altogether.
On its own that sounds dangerous, but its not. When we need to confirm
the prev link we already know what we expect it to be, so CRC-ing it
is overkill. That isn't true of any other part of the WAL record, so
the prev link is the only thing we can relax, but thats OK because we
can CRC check everything else outside of the locked section.
That isn't my idea, but I'm happy to put it on the table since I'm not shy.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
You missed your cue to discuss leaving the prev link out of the CRC altogether.
On its own that sounds dangerous, but its not. When we need to confirm
the prev link we already know what we expect it to be, so CRC-ing it
is overkill. That isn't true of any other part of the WAL record, so
the prev link is the only thing we can relax, but thats OK because we
can CRC check everything else outside of the locked section.
That isn't my idea, but I'm happy to put it on the table since I'm not shy.
I'm glad it's not your idea, because it's a bad one. A large part of
the point of CRC'ing WAL records is to guard against torn-page problems
in the WAL files, and doing things like that would give up a significant
part of that protection, because there would no longer be any assurance
that the body of a WAL record had anything to do with its prev_link.
Consider a scenario like this:
* We write a WAL record that starts 8 bytes before a sector boundary,
so that the prev_link is in one sector and the rest of the record in
the next one(s).
* Time passes, and we recycle that WAL file.
* We write another WAL record that starts 8 bytes before the same sector
boundary, so that the prev_link is in one sector and the rest of the
record in the next one(s).
* System crashes, after having written out the earlier sector but not
the later one(s).
On restart, the replay code will see a prev_link that matches what it
expects. If the CRC for the remainder of the record is not dependent
on the prev_link, then the remainder of the old record will look good
too, and we'll attempt to replay it, n*16MB too late.
Including the prev_link in the CRC adds a significant amount of
protection against such problems. We should not remove this protection
in the name of shaving a few cycles.
regards, tom lane
On 16.12.2011 05:27, Tom Lane wrote:
* We write a WAL record that starts 8 bytes before a sector boundary,
so that the prev_link is in one sector and the rest of the record in
the next one(s).
prev-link is not the first field in the header. The CRC is.
* Time passes, and we recycle that WAL file.
* We write another WAL record that starts 8 bytes before the same sector
boundary, so that the prev_link is in one sector and the rest of the
record in the next one(s).* System crashes, after having written out the earlier sector but not
the later one(s).On restart, the replay code will see a prev_link that matches what it
expects. If the CRC for the remainder of the record is not dependent
on the prev_link, then the remainder of the old record will look good
too, and we'll attempt to replay it, n*16MB too late.
The CRC would be in the previous sector with the prev-link, so the CRC
of the old record would have to match the CRC of the new record. I guess
that's not totally impossible, though - there could be some WAL-logged
operations where the payload of the WAL record is often exactly the
same. Like a heap clean record, when the same page is repeatedly pruned.
Including the prev_link in the CRC adds a significant amount of
protection against such problems. We should not remove this protection
in the name of shaving a few cycles.
Yeah. I did some quick testing with a patch to leave prev-link out of
the calculation, and move the record CRC calculation outside the lock,
too. I don't remember the numbers, but while it did make some
difference, it didn't seem worthwhile.
Anyway, I'm looking at ways to make the memcpy() of the payload happen
without the lock, in parallel, and once you do that the record header
CRC calculation can be done in parallel, too. That makes it irrelevant
from a performance point of view whether the prev-link is included in
the CRC or not.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Fri, Dec 16, 2011 at 12:07 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Anyway, I'm looking at ways to make the memcpy() of the payload happen
without the lock, in parallel, and once you do that the record header CRC
calculation can be done in parallel, too. That makes it irrelevant from a
performance point of view whether the prev-link is included in the CRC or
not.
Better plan. So we keep the prev link in the CRC.
I already proposed a design for that using page-level share locks any
reason not to go with that?
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 16.12.2011 14:37, Simon Riggs wrote:
On Fri, Dec 16, 2011 at 12:07 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:Anyway, I'm looking at ways to make the memcpy() of the payload happen
without the lock, in parallel, and once you do that the record header CRC
calculation can be done in parallel, too. That makes it irrelevant from a
performance point of view whether the prev-link is included in the CRC or
not.Better plan. So we keep the prev link in the CRC.
I already proposed a design for that using page-level share locks any
reason not to go with that?
Sorry, I must've missed that. Got a link?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Fri, Dec 16, 2011 at 12:50 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 16.12.2011 14:37, Simon Riggs wrote:
On Fri, Dec 16, 2011 at 12:07 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:Anyway, I'm looking at ways to make the memcpy() of the payload happen
without the lock, in parallel, and once you do that the record header CRC
calculation can be done in parallel, too. That makes it irrelevant from a
performance point of view whether the prev-link is included in the CRC or
not.Better plan. So we keep the prev link in the CRC.
I already proposed a design for that using page-level share locks any
reason not to go with that?Sorry, I must've missed that. Got a link?
From nearly 4 years ago.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 16.12.2011 15:03, Simon Riggs wrote:
On Fri, Dec 16, 2011 at 12:50 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 16.12.2011 14:37, Simon Riggs wrote:
I already proposed a design for that using page-level share locks any
reason not to go with that?Sorry, I must've missed that. Got a link?
From nearly 4 years ago.
Ah, thanks. That is similar to what I'm experimenting, but a second
lwlock is still fairly heavy-weight. I think with many backends, you
will be beaten badly by contention on the spinlocks alone.
I'll polish up and post what I've been experimenting with, so we can
discuss that.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On 16.12.2011 15:42, Heikki Linnakangas wrote:
On 16.12.2011 15:03, Simon Riggs wrote:
On Fri, Dec 16, 2011 at 12:50 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 16.12.2011 14:37, Simon Riggs wrote:
I already proposed a design for that using page-level share locks any
reason not to go with that?Sorry, I must've missed that. Got a link?
From nearly 4 years ago.
Ah, thanks. That is similar to what I'm experimenting, but a second
lwlock is still fairly heavy-weight. I think with many backends, you
will be beaten badly by contention on the spinlocks alone.I'll polish up and post what I've been experimenting with, so we can
discuss that.
So, here's a WIP patch of what I've been working on. The WAL insertions
is split into two stages:
1. Reserve the space from the WAL stream. This is done while holding a
spinlock. The page holding the reserved space doesn't necessary need to
be in cache yet, the reservation can run ahead of the WAL buffer cache.
(quick testing suggests that a lwlock is too heavy-weight for this)
2. Ensure the page is in the WAL buffer cache. If not, initialize it,
evicting old pages if needed. Then finish the CRC calculation of the
header and memcpy the record in place. (if the record spans multiple
pages, it operates on one page at a time, to avoid problems with running
out of WAL buffers)
As long as wal_buffers is high enough, and the I/O can keep up, stage 2
can happen in parallel in many backends. The WAL writer process
pre-initializes new pages ahead of the insertions, so regular backends
rarely need to do that.
When a page is written out, with XLogWrite(), you need to wait for any
in-progress insertions to the pages you're about to write out to finish.
For that, every backend has slot with an XLogRecPtr in shared memory.
Iẗ́'s set to the position where that backend is currently inserting to.
If there's no insertion in-progress, it's invalid, but when it's valid
it acts like a barrier, so that no-one is allowed to XLogWrite() beyond
that position. That's very lightweight to the backends, but I'm using
busy-waiting to wait on an insertion to finish ATM. That should be
replaced with something smarter, that's the biggest missing part of the
patch.
One simple way to test the performance impact of this is:
psql -c "DROP TABLE IF EXISTS foo; CREATE TABLE foo (id int4);
CHECKPOINT" postgres
echo "BEGIN; INSERT INTO foo SELECT i FROM generate_series(1, 10000) i;
ROLLBACK" > parallel-insert-test.sql
pgbench -n -T 10 -c4 -f parallel-insert-test.sql postgres
On my dual-core laptop, this patch increases the tps on that from about
60 to 110.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On 23.12.2011 10:13, Heikki Linnakangas wrote:
So, here's a WIP patch of what I've been working on.
And here's the patch I forgot to attach..
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
xloginsert-scale-1.patchtext/x-diff; name=xloginsert-scale-1.patchDownload+1211-1105
On Fri, Dec 23, 2011 at 3:15 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
On 23.12.2011 10:13, Heikki Linnakangas wrote:
So, here's a WIP patch of what I've been working on.
And here's the patch I forgot to attach..
Fails regression tests for me. I found this in postmaster.log:
PANIC: could not find WAL buffer for 0/2ECA438STATEMENT: ANALYZE onek2;
LOG: stuck waiting upto 0/3000000
LOG: server process (PID 34529) was terminated by signal 6: Aborted
DETAIL: Failed process was running: ANALYZE onek2;
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 23.12.2011 15:23, Robert Haas wrote:
On Fri, Dec 23, 2011 at 3:15 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:On 23.12.2011 10:13, Heikki Linnakangas wrote:
So, here's a WIP patch of what I've been working on.
And here's the patch I forgot to attach..
Fails regression tests for me. I found this in postmaster.log:
PANIC: could not find WAL buffer for 0/2ECA438STATEMENT: ANALYZE onek2;
LOG: stuck waiting upto 0/3000000
LOG: server process (PID 34529) was terminated by signal 6: Aborted
DETAIL: Failed process was running: ANALYZE onek2;
Sorry. Last minute changes, didn't retest properly.. Here's another attempt.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Attachments:
xloginsert-scale-2.patchtext/x-diff; name=xloginsert-scale-2.patchDownload+1211-1112
On Sat, Dec 24, 2011 at 4:54 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
Sorry. Last minute changes, didn't retest properly.. Here's another attempt.
When I tested the patch, initdb failed:
$ initdb -D data
....
initializing dependencies ... PANIC: could not locate a valid checkpoint record
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
