Async Commit, v21

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Simon Riggs wrote:

Latest version of synchronous_commit = on | off

Applies cleanly to CVS HEAD. Passes make check with/without
synchronous_commit on in postgresql.conf, while running
--enable-cassert.

I expect to review this tomorrow to make sure everything is correctly
changed before requesting final review/commit.

Docs included at top of main patch.

All comments welcome.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Tue, 2007-07-17 at 01:28 -0400, Bruce Momjian wrote:

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Simon Riggs wrote:

Latest version of synchronous_commit = on | off

Applies cleanly to CVS HEAD. Passes make check with/without
synchronous_commit on in postgresql.conf, while running
--enable-cassert.

I expect to review this tomorrow to make sure everything is correctly
changed before requesting final review/commit.

Docs included at top of main patch.

All comments welcome.

Here's the latest version. I've reviewed this to check that this does
what I want it to do, re-written various comments and changed a few
minor points in the code.

I've also added a chunk to transam/README that describes the workings of
the patch from a high level.

Now ready for final review.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Simon Riggs wrote:

On Tue, 2007-07-17 at 01:28 -0400, Bruce Momjian wrote:

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Simon Riggs wrote:

Latest version of synchronous_commit = on | off

Applies cleanly to CVS HEAD. Passes make check with/without
synchronous_commit on in postgresql.conf, while running
--enable-cassert.

I expect to review this tomorrow to make sure everything is correctly
changed before requesting final review/commit.

Docs included at top of main patch.

All comments welcome.

Here's the latest version. I've reviewed this to check that this does
what I want it to do, re-written various comments and changed a few
minor points in the code.

I've also added a chunk to transam/README that describes the workings of
the patch from a high level.

Now ready for final review.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Am Dienstag, 17. Juli 2007 20:31 schrieb Simon Riggs:

Here's the latest version. I've reviewed this to check that this does
what I want it to do, re-written various comments and changed a few
minor points in the code.

I've also added a chunk to transam/README that describes the workings of
the patch from a high level.

Now ready for final review.

I'm not sure the following explanation is all that clear:

+    <para>
+     Asynchronous commit provides different behaviour to setting
+     <varname>fsync</varname> = off, since that is a server-wide
+     setting that will alter the behaviour of all transactions,
+     overriding the setting of <varname>synchronous_commit</varname>,
+     as well as risking much wider data loss.  With <varname>fsync</varname>
+       = off the WAL written but not fsynced, so data is lost only in case
+       of a system crash. With asynchronous commit the WAL is not written
+       to disk at all by the user, so data is lost if there is a database
+       server crash, as well as when the system crashes.
+    </para>

On the one hand, it claims that fsync off has much wider data loss
implications than asynchronous commit, on the other hand, it states that the
risk of a loss due to asynchronous commit happening is larger than fsync off.
I *think* I know what this is trying to say, but I suspect that the average
user might not be able to make a good choice of settings.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Simon Riggs wrote:

Here's the latest version. I've reviewed this to check that this does
what I want it to do, re-written various comments and changed a few
minor points in the code.

I've also added a chunk to transam/README that describes the workings of
the patch from a high level.

Now ready for final review.

Just a couple of quick comments:

Having to call XLogBackgroundFlush(2) to flush the WAL up to the latest
async commit is a weird interface. How about passing a "bool force"
argument instead? Or adding another function "XLogFlushCommits".

walwriter.h is missing the

#ifndef XXXX_H
#define XXXX_H

boilerplate.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Wed, 2007-07-18 at 16:44 +0100, Heikki Linnakangas wrote:

Simon Riggs wrote:

Here's the latest version. I've reviewed this to check that this does
what I want it to do, re-written various comments and changed a few
minor points in the code.

I've also added a chunk to transam/README that describes the workings of
the patch from a high level.

Now ready for final review.

Just a couple of quick comments:

Having to call XLogBackgroundFlush(2) to flush the WAL up to the latest
async commit is a weird interface. How about passing a "bool force"
argument instead? Or adding another function "XLogFlushCommits".

walwriter.h is missing the

#ifndef XXXX_H
#define XXXX_H

boilerplate.

OK. Will do, thanks.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:

OK. Will do, thanks.

Make sure to remove the bogus comment about "pgstat considers our data
as gone" in walwriter.c as well (in the sigjmp block)

--
Alvaro Herrera Developer, http://www.PostgreSQL.org/
"Those who use electric razors are infidels destined to burn in hell while
we drink from rivers of beer, download free vids and mingle with naked
well shaved babes." (http://slashdot.org/comments.pl?sid=44793&amp;cid=4647152)

On Wed, 2007-07-18 at 12:04 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

OK. Will do, thanks.

Make sure to remove the bogus comment about "pgstat considers our data
as gone" in walwriter.c as well (in the sigjmp block)

Thanks, its gone in v23.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Wed, 2007-07-18 at 17:16 +0100, Simon Riggs wrote:

On Wed, 2007-07-18 at 12:04 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

OK. Will do, thanks.

Make sure to remove the bogus comment about "pgstat considers our data
as gone" in walwriter.c as well (in the sigjmp block)

Thanks, its gone in v23.

Thanks to everyone for the various review comments.

Here's v23, including all suggested changes, plus some reworking of the
transaction APIs to reduce the footprint of the patch.

Performance tests: pgbench -c 4 -t 10000
- pgbench standard, sync 800 tps, async 1200 tps
- single insert only, sync 3000 tps, async 9000 tps

Async performance appears to be just short of fsync=off

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Wed, 2007-07-18 at 12:12 +0200, Peter Eisentraut wrote:

Am Dienstag, 17. Juli 2007 20:31 schrieb Simon Riggs:

Here's the latest version. I've reviewed this to check that this does
what I want it to do, re-written various comments and changed a few
minor points in the code.

I've also added a chunk to transam/README that describes the workings of
the patch from a high level.

Now ready for final review.

I'm not sure the following explanation is all that clear:

+    <para>
+     Asynchronous commit provides different behaviour to setting
+     <varname>fsync</varname> = off, since that is a server-wide
+     setting that will alter the behaviour of all transactions,
+     overriding the setting of <varname>synchronous_commit</varname>,
+     as well as risking much wider data loss.  With <varname>fsync</varname>
+       = off the WAL written but not fsynced, so data is lost only in case
+       of a system crash. With asynchronous commit the WAL is not written
+       to disk at all by the user, so data is lost if there is a database
+       server crash, as well as when the system crashes.
+    </para>

On the one hand, it claims that fsync off has much wider data loss
implications than asynchronous commit, on the other hand, it states that the
risk of a loss due to asynchronous commit happening is larger than fsync off.
I *think* I know what this is trying to say, but I suspect that the average
user might not be able to make a good choice of settings.

Thanks for reading. Updated version in new patch.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon@2ndquadrant.com> writes:

Thanks for reading. Updated version in new patch.

What was the reasoning for basing walwriter.c on autovacuum (which needs
to be able to execute transactions) rather than bgwriter (which does
not)? The shutdown logic in particular seems all wrong; you can't have
a process connected to shared memory that is going to outlive the
postmaster.

regards, tom lane

On Mon, 2007-07-23 at 18:59 -0400, Tom Lane wrote:

"Simon Riggs" <simon@2ndquadrant.com> writes:

Thanks for reading. Updated version in new patch.

What was the reasoning for basing walwriter.c on autovacuum (which needs
to be able to execute transactions) rather than bgwriter (which does
not)?

Writing WAL means we have to have xlog access and therefore shared
memory access. Don't really need the ability to execute transactions
though, tis true, but I wasn't aware there was a distinction.

The shutdown logic in particular seems all wrong; you can't have
a process connected to shared memory that is going to outlive the
postmaster.

It seemed to work cleanly when I tested it initially, but I'll take
another look tomorrow. By version 23 I was going code-blind.

Autovac is the most clean implementation of a special process, so seemed
like a good prototype. I'd thought I'd combed out any pointless code
though.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:

Autovac is the most clean implementation of a special process, so seemed
like a good prototype. I'd thought I'd combed out any pointless code
though.

What, you mean there's pointless code in autovac? Hey, be sure to let
me know about it!

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Simon Riggs wrote:

Here's v23, including all suggested changes, plus some reworking of the
transaction APIs to reduce the footprint of the patch.

What's the thing about doing the flush twice in a couple of comments in
calls to XLogBackgroundFlush? Are they just leftover comments from
older code?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

"Simon Riggs" <simon@2ndquadrant.com> writes:

On Mon, 2007-07-23 at 18:59 -0400, Tom Lane wrote:

The shutdown logic in particular seems all wrong; you can't have
a process connected to shared memory that is going to outlive the
postmaster.

It seemed to work cleanly when I tested it initially, but I'll take
another look tomorrow. By version 23 I was going code-blind.

Leave it be for the moment --- I'm working on it now so it'd just be
duplicated effort. I'm inclined though to rebase at least the signal
handling on bgwriter.c; don't like different processes using different
signal interpretations unless there's a good reason for it.

I came across another point worthy of mention: as given, the patch turns
XLogWrite's "flexible write" logic into dead code, because there are no
callers that pass flexible = true. We could rip that out, but it seems
to me there's still some value to it under high load conditions ("high
load" meaning we fill multiple wal pages per wal_writer_delay). What
I'm inclined to do is modify XLogBackgroundFlush so that it uses
flexible = true when it's flushing whole pages. The downside to that
is that it could take as many as three walwriter cycles, instead of two,
to guararantee an async commit is down to disk:
* first write/flush stops at buffer wraparound point
* second one stops at last complete page
* third finally is certain to write any remaining commit record
This seems like no big problem to me, but does anyone want to object?

(BTW, in case you can't tell from the drift of my questions, I've
separated the patch into "add background wal writer" and "add async
commit", and am working on the first half.)

regards, tom lane

Alvaro Herrera <alvherre@commandprompt.com> writes:

What's the thing about doing the flush twice in a couple of comments in
calls to XLogBackgroundFlush? Are they just leftover comments from
older code?

I was wondering that too --- they looked like obsolete comments to me.

My current thinking BTW is that trying to make XLogBackgroundFlush serve
two purposes is counterproductive. It should be dedicated to use by the
walwriter only, and the checkpoint case should simply read the async
commit pointer and call regular XLogFlush with it.

regards, tom lane

I wrote:

(BTW, in case you can't tell from the drift of my questions, I've
separated the patch into "add background wal writer" and "add async
commit", and am working on the first half.)

I've committed the first half of that. Something that still needs
investigation is what the default wal_writer_delay should be. I left
it at 200ms as submitted, but in some crude testing here (just running
the regression tests and strace'ing the walwriter) it seemed that I had
to crank it down to 10ms before the walwriter was really doing the
majority of the wal-flushing work.

regards, tom lane

On Mon, 2007-07-23 at 20:02 -0400, Alvaro Herrera wrote:

Simon Riggs wrote:

Autovac is the most clean implementation of a special process, so seemed
like a good prototype. I'd thought I'd combed out any pointless code
though.

What, you mean there's pointless code in autovac? Hey, be sure to let
me know about it!

Nothing pointless in autovac. It's clean, which is why I used it for the
basis for walwriter.c. Once copied, some parts were not needed.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Mon, 2007-07-23 at 21:11 -0400, Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

What's the thing about doing the flush twice in a couple of comments in
calls to XLogBackgroundFlush? Are they just leftover comments from
older code?

I was wondering that too --- they looked like obsolete comments to me.

True, recent API change meant they were slightly off.

My current thinking BTW is that trying to make XLogBackgroundFlush serve
two purposes is counterproductive. It should be dedicated to use by the
walwriter only, and the checkpoint case should simply read the async
commit pointer and call regular XLogFlush with it.

OK

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com