On Fri, Mar 25, 2011 at 8:53 PM, Fujii Masao wrote:
On Sat, Mar 19, 2011 at 12:07 AM, Robert Haas wrote:
On Fri, Mar 18, 2011 at 10:55 AM, Greg Stark wrote:
On Thu, Mar 17, 2011 at 5:46 PM, Robert Haas 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

Search Discussions

  • Robert Haas at Apr 4, 2011 at 8:25 pm

    On Mon, Apr 4, 2011 at 4:54 AM, Fujii Masao wrote:
    On Fri, Mar 25, 2011 at 8:53 PM, Fujii Masao wrote:
    On Sat, Mar 19, 2011 at 12:07 AM, Robert Haas wrote:
    On Fri, Mar 18, 2011 at 10:55 AM, Greg Stark wrote:
    On Thu, Mar 17, 2011 at 5:46 PM, Robert Haas 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
  • Robert Haas at Apr 4, 2011 at 9:16 pm

    On Mon, Apr 4, 2011 at 4:25 PM, Robert Haas 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
  • Fujii Masao at Apr 5, 2011 at 1:09 am

    On Tue, Apr 5, 2011 at 6:13 AM, Robert Haas 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
  • Dimitri Fontaine at Apr 5, 2011 at 7:53 am
    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
  • Fujii Masao at Apr 5, 2011 at 8:22 am

    On Tue, Apr 5, 2011 at 4:53 PM, Dimitri Fontaine 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
  • Dimitri Fontaine at Apr 5, 2011 at 8:31 am

    Fujii Masao 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
  • Heikki Linnakangas at Apr 5, 2011 at 8:33 am

    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
  • Dimitri Fontaine at Apr 5, 2011 at 8:44 am

    Heikki Linnakangas 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
  • Robert Haas at Apr 5, 2011 at 11:52 am

    On Tue, Apr 5, 2011 at 3:53 AM, Dimitri Fontaine 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
  • Greg Stark at Apr 5, 2011 at 2:02 pm
    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.

    The other question is whether it's "fair" that one kind of patch goes in and
    not the other. Personally I feel changes to GUCs are the kind of thing we
    most often want to do in alpha. Patches that change functionality require a
    higher barrier and need to be fixing user complaints or bugs. My perception
    was that Simon's patch was ggreenberg latter.
    On Apr 5, 2011 12:52 PM, "Robert Haas" wrote:
    On Tue, Apr 5, 2011 at 3:53 AM, Dimitri Fontaine 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
  • Kevin Grittner at Apr 5, 2011 at 3:03 pm

    Robert Haas wrote:
    Dimitri Fontaine 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
  • Tom Lane at Apr 5, 2011 at 3:13 pm

    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    Robert Haas wrote:
    Dimitri Fontaine 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
  • Robert Haas at Apr 5, 2011 at 3:26 pm

    On Tue, Apr 5, 2011 at 11:12 AM, Tom Lane wrote:
    "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
    Robert Haas wrote:
    Dimitri Fontaine 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
  • Robert Haas at Apr 5, 2011 at 4:38 pm

    On Tue, Apr 5, 2011 at 11:25 AM, Robert Haas 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.

    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
  • Tom Lane at Apr 5, 2011 at 6:51 pm

    Robert Haas writes:
    On Tue, Apr 5, 2011 at 11:25 AM, Robert Haas 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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedApr 4, '11 at 8:54a
activeApr 5, '11 at 6:51p
posts16
users7
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2022 Grokbase