Thread: synchronous_commit and synchronous_replication Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.

On Fri, Mar 25, 2011 at 8:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Sat, Mar 19, 2011 at 12:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Mar 18, 2011 at 10:55 AM, Greg Stark <gsstark@mit.edu> wrote:
>>> On Thu, Mar 17, 2011 at 5:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> What makes more sense to me after having thought about this more
>>>> carefully is to simply make a blanket rule that when
>>>> synchronous_replication=on, synchronous_commit has no effect.  That is
>>>> easy to understand and document.
>>>
>>> For what it's worth "has no effect" doesn't make much sense to me.
>>> It's a boolean, either commits are going to block or they're not.
>>>
>>> What happened to the idea of a three-way switch?
>>>
>>> synchronous_commit = off
>>> synchronous_commit = disk
>>> synchronous_commit = replica
>>>
>>> With "on" being a synonym for "disk" for backwards compatibility.
>>>
>>> Then we could add more options later for more complex conditions like
>>> waiting for one server in each data centre or waiting for one of a
>>> certain set of servers ignoring the less reliable mirrors, etc.
>>
>> This is similar to what I suggested upthread, except that I suggested
>> on/local/off, with the default being on.  That way if you set
>> synchronous_standby_names, you get synchronous replication without
>> changing another setting, but you can say local instead if for some
>> reason you want the middle behavior.  If we're going to do it all with
>> one GUC, I think that way makes more sense.  If you're running sync
>> rep, you might still have some transactions that you don't care about,
>> but that's what async commit is for.  It's a funny kind of transaction
>> that we're OK with losing if we have a failover but we're not OK with
>> losing if we have a local crash from which we recover without failing
>> over.
>
> I'm OK with this.

The attached patch merges synchronous_replication into synchronous_commit.
With the patch, valid values of synchronous_commit are "on" (waits for local
flush and sync rep), "off" (waits for neither local flush nor sync
rep), and "local"
(waits for only local flush).

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Apr 4, 2011 at 4:54 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Mar 25, 2011 at 8:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Sat, Mar 19, 2011 at 12:07 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Fri, Mar 18, 2011 at 10:55 AM, Greg Stark <gsstark@mit.edu> wrote:
>>>> On Thu, Mar 17, 2011 at 5:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>>> What makes more sense to me after having thought about this more
>>>>> carefully is to simply make a blanket rule that when
>>>>> synchronous_replication=on, synchronous_commit has no effect.  That is
>>>>> easy to understand and document.
>>>>
>>>> For what it's worth "has no effect" doesn't make much sense to me.
>>>> It's a boolean, either commits are going to block or they're not.
>>>>
>>>> What happened to the idea of a three-way switch?
>>>>
>>>> synchronous_commit = off
>>>> synchronous_commit = disk
>>>> synchronous_commit = replica
>>>>
>>>> With "on" being a synonym for "disk" for backwards compatibility.
>>>>
>>>> Then we could add more options later for more complex conditions like
>>>> waiting for one server in each data centre or waiting for one of a
>>>> certain set of servers ignoring the less reliable mirrors, etc.
>>>
>>> This is similar to what I suggested upthread, except that I suggested
>>> on/local/off, with the default being on.  That way if you set
>>> synchronous_standby_names, you get synchronous replication without
>>> changing another setting, but you can say local instead if for some
>>> reason you want the middle behavior.  If we're going to do it all with
>>> one GUC, I think that way makes more sense.  If you're running sync
>>> rep, you might still have some transactions that you don't care about,
>>> but that's what async commit is for.  It's a funny kind of transaction
>>> that we're OK with losing if we have a failover but we're not OK with
>>> losing if we have a local crash from which we recover without failing
>>> over.
>>
>> I'm OK with this.
>
> The attached patch merges synchronous_replication into synchronous_commit.
> With the patch, valid values of synchronous_commit are "on" (waits for local
> flush and sync rep), "off" (waits for neither local flush nor sync
> rep), and "local"
> (waits for only local flush).

Committed with some additional hacking.  In particular, I believe that
your version made SYNCHRONOUS_COMMIT_LOCAL equivalent to
SYNCHRONOUS_COMMIT_OFF, which was wrong; and your replacement of
synchronous_replication by synchronous_commit in the docs was a bit
too formulaic; in particular, the section on setting up a basic sync
rep configuration said that all you needed to do was set
synchronous_commit=on, which clearly made no sense, since that was
neither necessary (since that's the default) nor sufficient (since you
have to set synchronous_standby_names).

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

On Mon, Apr 4, 2011 at 4:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> I'm OK with this.
>>
>> The attached patch merges synchronous_replication into synchronous_commit.
>> With the patch, valid values of synchronous_commit are "on" (waits for local
>> flush and sync rep), "off" (waits for neither local flush nor sync
>> rep), and "local"
>> (waits for only local flush).
>
> Committed with some additional hacking.  In particular, I believe that
> your version made SYNCHRONOUS_COMMIT_LOCAL equivalent to
> SYNCHRONOUS_COMMIT_OFF, which was wrong; and your replacement of
> synchronous_replication by synchronous_commit in the docs was a bit
> too formulaic; in particular, the section on setting up a basic sync
> rep configuration said that all you needed to do was set
> synchronous_commit=on, which clearly made no sense, since that was
> neither necessary (since that's the default) nor sufficient (since you
> have to set synchronous_standby_names).

Err, woops.  Actually, I'm wrong about the first point: your coding
worked, but I had to adjust it when I reordered the enum.  I think the
new ordering is more logical, but YMMV.

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

On Tue, Apr 5, 2011 at 6:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Committed with some additional hacking.  In particular, I believe that
>> your version made SYNCHRONOUS_COMMIT_LOCAL equivalent to
>> SYNCHRONOUS_COMMIT_OFF, which was wrong; and your replacement of
>> synchronous_replication by synchronous_commit in the docs was a bit
>> too formulaic; in particular, the section on setting up a basic sync
>> rep configuration said that all you needed to do was set
>> synchronous_commit=on, which clearly made no sense, since that was
>> neither necessary (since that's the default) nor sufficient (since you
>> have to set synchronous_standby_names).
>
> Err, woops.  Actually, I'm wrong about the first point: your coding
> worked, but I had to adjust it when I reordered the enum.  I think the
> new ordering is more logical

Yes. Thanks a lot!

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hi,

Robert Haas <robertmhaas@gmail.com> writes:
>> The attached patch merges synchronous_replication into synchronous_commit.
> Committed

Without discussion?  I would think that this patch is stepping on the
other one toes and that maybe would need to make a decision about sync
rep behavior before to commit this change.

Maybe it's just me, but I'm struggling to understand current community
processes and decisions.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On Tue, Apr 5, 2011 at 4:53 PM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Hi,
>
> Robert Haas <robertmhaas@gmail.com> writes:
>>> The attached patch merges synchronous_replication into synchronous_commit.
>> Committed
>
> Without discussion?  I would think that this patch is stepping on the
> other one toes and that maybe would need to make a decision about sync
> rep behavior before to commit this change.

Hmm.. I think that we reached the consensus about merging two GUCs
in previous discussion. You argue that synchronization level should be
controlled in separate two parameters?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao <masao.fujii@gmail.com> writes:
> Hmm.. I think that we reached the consensus about merging two GUCs
> in previous discussion. You argue that synchronization level should be
> controlled in separate two parameters?

No, sorry about confusion.  One GUC is better.  What I'm wondering is
why commit it *now*, because I think we didn't yet decide on what the
supported behaviors supported in 9.1 should be.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On 05.04.2011 11:31, Dimitri Fontaine wrote:
> Fujii Masao<masao.fujii@gmail.com>  writes:
>> Hmm.. I think that we reached the consensus about merging two GUCs
>> in previous discussion. You argue that synchronization level should be
>> controlled in separate two parameters?
>
> No, sorry about confusion.  One GUC is better.  What I'm wondering is
> why commit it *now*, because I think we didn't yet decide on what the
> supported behaviors supported in 9.1 should be.

What do you mean by "supported behaviors"?

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

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> No, sorry about confusion.  One GUC is better.  What I'm wondering is
>> why commit it *now*, because I think we didn't yet decide on what the
>> supported behaviors supported in 9.1 should be.
>
> What do you mean by "supported behaviors"?

Well, I'm thinking about Simon's patch that some decided on procedural
grounds only that it was too late to apply in 9.1, and some others were
saying that given the cost benefit ratio of such a small patch that the
design had already been agreed on, it is legible for 9.1.

I think that we just expressed opinions on the async|recv|fsync|apply
patch, and that we've yet to reach a consensus and make a decision.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On Tue, Apr 5, 2011 at 3:53 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>>> The attached patch merges synchronous_replication into synchronous_commit.
>> Committed
>
> Without discussion?  I would think that this patch is stepping on the
> other one toes and that maybe would need to make a decision about sync
> rep behavior before to commit this change.

Err, I thought we did.  We had a protracted discussion of Simon's
patch: 9 people expressed an opinion; 6 were opposed.

With respect to this patch, the basic design was discussed previously
and Simon, Fujii Masao, Greg Stark and myself all were basically in
favor of something along these lines, and to the best of my
recollection no one spoke against it.

> Maybe it's just me, but I'm struggling to understand current community
> processes and decisions.

Well, I've already spent a fair amount of time trying to explain my
understanding of it, and for my trouble I got accused of being
long-winded.  Which is probably true, but makes me think I should
probably keep this response short.  I'm not unwilling to talk about
it, though, and perhaps someone else would like to chime in.

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

<p>For what it's worth it seems to me this patch makrmes it at least conceptually easier to add new modes like Simon
plans,not harder. It's worth making sure we pick names that still make sense when the new functionality goes in of
course.<p>Theother question is whether it's "fair" that one kind of patch goes in and not the other. Personally I feel
changesto GUCs are the kind of thing we most often want to do in alpha. Patches that change functionality require a
higherbarrier and need to be fixing user complaints or bugs. My perception was that Simon's patch was ggreenberg
latter.<divclass="gmail_quote">On Apr 5, 2011 12:52 PM, "Robert Haas" <<a
href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>>wrote:<br type="attribution" />> On Tue, Apr 5,
2011at 3:53 AM, Dimitri Fontaine <<a href="mailto:dimitri@2ndquadrant.fr">dimitri@2ndquadrant.fr</a>> wrote:<br
/>>> Robert Haas <<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>> writes:<br
/>>>>>The attached patch merges synchronous_replication into synchronous_commit.<br />>>>
Committed<br/> >><br />>> Without discussion?  I would think that this patch is stepping on the<br
/>>>other one toes and that maybe would need to make a decision about sync<br />>> rep behavior before to
committhis change.<br /> > <br />> Err, I thought we did. We had a protracted discussion of Simon's<br />>
patch:9 people expressed an opinion; 6 were opposed.<br />> <br />> With respect to this patch, the basic design
wasdiscussed previously<br /> > and Simon, Fujii Masao, Greg Stark and myself all were basically in<br />> favor
ofsomething along these lines, and to the best of my<br />> recollection no one spoke against it.<br />> <br
/>>>Maybe it's just me, but I'm struggling to understand current community<br /> >> processes and
decisions.<br/>> <br />> Well, I've already spent a fair amount of time trying to explain my<br />>
understandingof it, and for my trouble I got accused of being<br />> long-winded. Which is probably true, but makes
methink I should<br /> > probably keep this response short. I'm not unwilling to talk about<br />> it, though,
andperhaps someone else would like to chime in.<br />> <br />> -- <br />> Robert Haas<br />> EnterpriseDB:
<ahref="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br /> > The Enterprise PostgreSQL Company<br
/></div>

Robert Haas <robertmhaas@gmail.com> wrote:
> Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
>> Maybe it's just me, but I'm struggling to understand current
>> community processes and decisions.
> Well, I've already spent a fair amount of time trying to explain
> my understanding of it, and for my trouble I got accused of being
> long-winded.  Which is probably true, but makes me think I should
> probably keep this response short.  I'm not unwilling to talk
> about it, though, and perhaps someone else would like to chime in.
I rather liked the brief comment in a recent post of yours where you
said that at this point we should only be accepting patches which
stabilize what has already been committed, rather than new features
which might require further stabilization.  I don't know whether the
patch under discussion satisfies that test, but that should be the
main consideration at this point in the release cycle, in my view.
Of course, with anything this complex there will be gray areas where
people could have honest disagreement.
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Robert Haas <robertmhaas@gmail.com> wrote:
>> Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
>>> Maybe it's just me, but I'm struggling to understand current
>>> community processes and decisions.
>> Well, I've already spent a fair amount of time trying to explain
>> my understanding of it, and for my trouble I got accused of being
>> long-winded.  Which is probably true, but makes me think I should
>> probably keep this response short.  I'm not unwilling to talk
>> about it, though, and perhaps someone else would like to chime in.
> I rather liked the brief comment in a recent post of yours where you
> said that at this point we should only be accepting patches which
> stabilize what has already been committed, rather than new features
> which might require further stabilization.

Quite.  While we're on the subject, why did that int->money patch get
committed so quickly?  I had assumed that was 9.2 material, because it
didn't seem to be addressing any new-in-9.1 issue.  I'm not going to ask
for it to be backed out, but I am wondering what the decision process
was.
        regards, tom lane

On Tue, Apr 5, 2011 at 11:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>> Robert Haas <robertmhaas@gmail.com> wrote:
>>> Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
>>>> Maybe it's just me, but I'm struggling to understand current
>>>> community processes and decisions.
>
>>> Well, I've already spent a fair amount of time trying to explain
>>> my understanding of it, and for my trouble I got accused of being
>>> long-winded.  Which is probably true, but makes me think I should
>>> probably keep this response short.  I'm not unwilling to talk
>>> about it, though, and perhaps someone else would like to chime in.
>
>> I rather liked the brief comment in a recent post of yours where you
>> said that at this point we should only be accepting patches which
>> stabilize what has already been committed, rather than new features
>> which might require further stabilization.
>
> Quite.  While we're on the subject, why did that int->money patch get
> committed so quickly?  I had assumed that was 9.2 material, because it
> didn't seem to be addressing any new-in-9.1 issue.  I'm not going to ask
> for it to be backed out, but I am wondering what the decision process
> was.

Well, I posted a note about this on Thursday:

http://archives.postgresql.org/pgsql-hackers/2011-03/msg01930.php

I didn't feel strongly that it needed to be done, but there seemed to
be some support for doing it:

http://archives.postgresql.org/pgsql-hackers/2011-03/msg01940.php
http://archives.postgresql.org/pgsql-hackers/2011-03/msg01943.php

But I'm wondering whether that was really the right decision.  It
might have been better just to drop it, and I'll certainly back it out
if people feel that's more appropriate.

I am also wondering about the open issue of supporting comments to
SQL/MED objects.  I thought that was pretty straightforward, but given
that it took me three commits to get servers and foreign data wrappers
squared away and then it turned out that we're still missing support
for user mappings, I've been vividly reminded of the danger of
seemingly harmless commits.  Now I'm thinking that I should have just
replied to the initial report with "good point, but it's not a new
regression, so we'll fix it in 9.2".  But given that part of the work
has already been done, I'm not sure whether I should (a) finish it, so
we don't have to revisit this in 9.2, (b) leave it well enough alone,
and we'll finish it in 9.2, or (c) back out what's already been done
and plan to fix the whole thing in 9.2.

Everything else on the open items list appears to be a bona fide bug,
though the generate_series thing is not a new regression.

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

On Tue, Apr 5, 2011 at 11:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> I am also wondering about the open issue of supporting comments to
> SQL/MED objects.  I thought that was pretty straightforward, but given
> that it took me three commits to get servers and foreign data wrappers
> squared away and then it turned out that we're still missing support
> for user mappings, I've been vividly reminded of the danger of
> seemingly harmless commits.  Now I'm thinking that I should have just
> replied to the initial report with "good point, but it's not a new
> regression, so we'll fix it in 9.2".  But given that part of the work
> has already been done, I'm not sure whether I should (a) finish it, so
> we don't have to revisit this in 9.2, (b) leave it well enough alone,
> and we'll finish it in 9.2, or (c) back out what's already been done
> and plan to fix the whole thing in 9.2.

On further review, I think (a) is not even an option worth discussing.The permissions-checking logic for user mappings
isquite different 
from what we do in the general case, and it seems likely to me that
cleaning this up is going to require far more time and thought than we
ought to be putting into what is really a relatively minor wart.  In
retrospect, it seems clear that this wasn't worth messing with in the
first place at this late date in the release cycle.

If there are any other items on the open items list that seem like
things we should not be worrying about right now, please point them
out.  I'm likely guilty of tunnel vision, as I have been heavily
focused on trying to make the list go to zero, and of course
committing stuff is only one of two possible ways to get them off the
list - the other is to decide that that they shouldn't have been added
in the first place.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 5, 2011 at 11:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I am also wondering about the open issue of supporting comments to
>> SQL/MED objects. �I thought that was pretty straightforward, but given
>> that it took me three commits to get servers and foreign data wrappers
>> squared away and then it turned out that we're still missing support
>> for user mappings, I've been vividly reminded of the danger of
>> seemingly harmless commits. �Now I'm thinking that I should have just
>> replied to the initial report with "good point, but it's not a new
>> regression, so we'll fix it in 9.2". �But given that part of the work
>> has already been done, I'm not sure whether I should (a) finish it, so
>> we don't have to revisit this in 9.2, (b) leave it well enough alone,
>> and we'll finish it in 9.2, or (c) back out what's already been done
>> and plan to fix the whole thing in 9.2.

> On further review, I think (a) is not even an option worth discussing.
>  The permissions-checking logic for user mappings is quite different
> from what we do in the general case, and it seems likely to me that
> cleaning this up is going to require far more time and thought than we
> ought to be putting into what is really a relatively minor wart.  In
> retrospect, it seems clear that this wasn't worth messing with in the
> first place at this late date in the release cycle.

I agree that we should leave user mappings alone at the moment.  I don't
see a need to back out the work that's been done for the other object
types, unless you think there may be flaws in that.
        regards, tom lane