Обсуждение: 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