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

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

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

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/

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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&cid=4647152)

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

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


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

http://archives.postgresql.org

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


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

"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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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

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

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