Thread: Additional options for Sync Replication
Proposed changes: Make synchronous_replication into an enum, so we can now also say synchronous_replication = recv, flush or apply as well as on or off. synchronous_replication = on is the same as "flush" Benefit: Allows 2 additional wait modes for sync rep: wait for receive and wait for apply. Rationale: I was hoping to fine tune/tweak Sync Rep after feedback during beta, but my understanding of current consensus is that that will be too late to make user visible changes. So I'm proposing this change now, before Beta, rather than during Beta. Since we now have replies from standby arriving when we receive data, not just fsync, we may as well do something useful with them in this release. So I reintroduce changes made in the original patch submission that were split out for piece-by-piece commit. This was also part of the originally agreed functionality for Sync Rep. So I don't expect any procedural difficulties in accepting this patch. Heikki was right to request removal of that code while we got things correct. Adding it back now is much simpler. The internal changes are minor, simply changing things so we have 3 queues rather than 1. The user backend path is identical in length, the sync standby path very slightly longer. Replies from standby continue to be sent every 100ms until all flushed WAL has been applied, then it falls back to the wal_receiver_status_interval, again a minor change. The parameter is an enum since multiple people called Josh asked for a very simple interface "on" or "off", while others requested multiple favours. This gives us both. I expect "recv" mode to be even more useful in next release, with WALWriter active during recovery. Patch feedback please. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Sun, Mar 27, 2011 at 5:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > I was hoping to fine tune/tweak Sync Rep after feedback during beta, > but my understanding of current consensus is that that will be too > late to make user visible changes. So I'm proposing this change now, > before Beta, rather than during Beta. This is completely inappropriate. The deadline for new feature patches has long since passed. It was January 15th. The discussion you are referring to had to do with fixing the behavior of features we already have, not adding new ones. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Mar 27, 2011 at 10:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > I was hoping to fine tune/tweak Sync Rep after feedback during beta, > but my understanding of current consensus is that that will be too > late to make user visible changes. So I'm proposing this change now, > before Beta, rather than during Beta. > For what it's worth I think this is a simplification. We have: 1) Development when new features are added 2) Feature freeze - when those features are tweaked and fixed based on our own testing but no new features added 3) Beta - when features are tweaked and fixed in response to user suggestions but no new features added 4) Release - when only bugs are fixed So the question becomes, what is a new feature versus a behaviour change to an existing feature or a bug-fix. These are subjective questions with a lot of room for ambiguity. Is this a new feature? Is this existing code broken or unusable in some way that we don't want to release and have to support? We're not in a vacuum here and we all see Tom reworking major portions of the collation patch while you're being told you can't squeak this relatively small feature in. But the collation behaviour is something we'll have to deal with and support for years and will have trouble changing in subsequent versions without compatibility issues. This behaviour is clearly something that can be added onto in subsequent versions and the existing feature set is usable as it is. Also, for what it's worth I prefer thinking of synchronous_commit/synchronous_replication as one big multi-way variable: synchronous_commit = memory | disk | replica-memory | replica-disk | replica-visible -- greg
On Sun, Mar 27, 2011 at 11:27 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Mar 27, 2011 at 5:45 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> I was hoping to fine tune/tweak Sync Rep after feedback during beta, >> but my understanding of current consensus is that that will be too >> late to make user visible changes. So I'm proposing this change now, >> before Beta, rather than during Beta. > > This is completely inappropriate. The deadline for new feature > patches has long since passed. It was January 15th. The discussion > you are referring to had to do with fixing the behavior of features we > already have, not adding new ones. You skipped the bit that said this code was part of the original patch submission, so this code met your deadline. That was stated clearly in the next paragraph of my email. It's also a minor change and one that others had agreed we want. You have no basis on which to prevent this. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Mar 27, 2011 at 11:55 PM, Greg Stark <gsstark@mit.edu> wrote: > Also, for what it's worth I prefer thinking of > synchronous_commit/synchronous_replication as one big multi-way > variable: > > synchronous_commit = memory | disk | replica-memory | replica-disk | > replica-visible That's close enough to my thinking for me to say yes to. I'd prefer to call it "commit_durability" though. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 28, 2011 at 10:52 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > > You have no basis on which to prevent this. It's also already on the Open Items list, put there by you. Why is this even a discussion point? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 28.03.2011 13:14, Simon Riggs wrote: > On Mon, Mar 28, 2011 at 10:52 AM, Simon Riggs<simon@2ndquadrant.com> wrote: >> >> You have no basis on which to prevent this. > > It's also already on the Open Items list, put there by you. > > Why is this even a discussion point? FWIW, I agree this is an additional feature that we shouldn't be messing with at this point in the release cycle. The 'apply' mode would be quite interesting, it would make it easier to build load-balancing clusters. But the patch isn't up to the task on that yet - the 'apply' status report is only sent after wal_receiver_status_interval fills up, so you get long delays. PS. you're missing some break's in the switch-statement in SyncRepWaitForLSN(), effectively making the patch do nothing. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Mon, Mar 28, 2011 at 6:14 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Mon, Mar 28, 2011 at 10:52 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> >> You have no basis on which to prevent this. > > It's also already on the Open Items list, put there by you. Huh? There is an open item about whether we should merge synchronous_commit and synchronous_replication into a single GUC, which might be a better interface since it would typically give the user one less thing to have to fiddle with, but there is certainly no open item for adding additional sync rep modes. Merging those two GUCs is a reasonable thing to consider even at this late date, because once we cut beta - and certainly once we cut final - we'll be stuck supporting whatever interface we release for 5+ years. There is no similar risk for this patch - the only risk of not committing this feature now is that we won't have this feature in 9.1. But if we accept that as valid justification for committing it, then everything is fair game, and that is not going to lead to a timely release. > Why is this even a discussion point? Adding more new code is likely to add more new bugs, and we're trying not to do that right now, so we can make a release. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 28, 2011 at 12:05 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 28.03.2011 13:14, Simon Riggs wrote: >> >> On Mon, Mar 28, 2011 at 10:52 AM, Simon Riggs<simon@2ndquadrant.com> >> wrote: >>> >>> You have no basis on which to prevent this. >> >> It's also already on the Open Items list, put there by you. >> >> Why is this even a discussion point? > > FWIW, I agree this is an additional feature that we shouldn't be messing > with at this point in the release cycle. There is an open item about what the UI is for sync commit/sync rep, which is the subject of this patch. > The 'apply' mode would be quite interesting, it would make it easier to > build load-balancing clusters. But the patch isn't up to the task on that > yet - the 'apply' status report is only sent after > wal_receiver_status_interval fills up, so you get long delays. Yes it's up to the task, you misread it. It will continue sending replies while apply <> flush and then it will fall back to the behaviour you mention. > PS. you're missing some break's in the switch-statement in > SyncRepWaitForLSN(), effectively making the patch do nothing. OK, thanks. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 28, 2011 at 10:59 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Sun, Mar 27, 2011 at 11:55 PM, Greg Stark <gsstark@mit.edu> wrote: > >> Also, for what it's worth I prefer thinking of >> synchronous_commit/synchronous_replication as one big multi-way >> variable: >> >> synchronous_commit = memory | disk | replica-memory | replica-disk | >> replica-visible > > That's close enough to my thinking for me to say yes to. Patch to implement this new UI attached, exactly as you suggest above. Words can be changed easily without changing the music. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On 28.03.2011 15:34, Simon Riggs wrote: > On Mon, Mar 28, 2011 at 12:05 PM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> On 28.03.2011 13:14, Simon Riggs wrote: >>> >>> On Mon, Mar 28, 2011 at 10:52 AM, Simon Riggs<simon@2ndquadrant.com> >>> wrote: >>>> >>>> You have no basis on which to prevent this. >>> >>> It's also already on the Open Items list, put there by you. >>> >>> Why is this even a discussion point? >> >> FWIW, I agree this is an additional feature that we shouldn't be messing >> with at this point in the release cycle. > > There is an open item about what the UI is for sync commit/sync rep, > which is the subject of this patch. plus new functionality. For the UI part, you just need to change GUCs. >> The 'apply' mode would be quite interesting, it would make it easier to >> build load-balancing clusters. But the patch isn't up to the task on that >> yet - the 'apply' status report is only sent after >> wal_receiver_status_interval fills up, so you get long delays. > > Yes it's up to the task, you misread it. It will continue sending > replies while apply<> flush and then it will fall back to the > behaviour you mention. There's nothing to wake up the walreceiver after applying a commit record. Oh, you're relying on the periodic wakeups specified by NAPTIME_PER_CYCLE (100ms). That's still on average a 50ms delay to every commit. We should try to eliminate these polling loops, not make them more important. In fact, we should raise NAPTIME_PER_CYCLE to, say, 1000ms, to reduce spurious wakeups on an idle system. Am I reading the patch correctly that if the standby hasn't applied all WAL yet, you send a reply message at every wakeup, whether or not any progress has been made since last time? So if you have a long-running-transaction in the standby, for example, conflicting with WAL recovery, the standby will keep sending a status report to the master every 100ms. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Mon, Mar 28, 2011 at 1:14 PM, Robert Haas <robertmhaas@gmail.com> wrote: > but there is certainly no > open item for adding additional sync rep modes. In your opinion. We will have to live with the UI for a long time, yes. I'm trying to get it right, whether that includes adding an obvious/easy omission or renaming things to make better sense. Your other changes make this sensible, and feedback I received after a recent presentation tells me that people will expect it to work with the two additional options. I would prefer further feedback before solidifying this design, but if we must solidify it now, then I prefer to do that with all 5 options. As originally submitted for this commit fest. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 28, 2011 at 8:54 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Mon, Mar 28, 2011 at 1:14 PM, Robert Haas <robertmhaas@gmail.com> wrote: > >> but there is certainly no >> open item for adding additional sync rep modes. > > In your opinion. Well, as you just pointed out yourself a few minutes ago, I did write the open item in question... I fancy I knew what I meant. > I would prefer further feedback before solidifying this design, but if > we must solidify it now, then I prefer to do that with all 5 options. > As originally submitted for this commit fest. Even if it were true that these options were in the patch submitted for this CommitFest, that wouldn't be a reason to commit them now, because the CommitFest is over and has been for several weeks. But it turns out they weren't. http://archives.postgresql.org/message-id/1295127631.3282.100.camel@ebony +/* + * Queuing code is written to allow later extension to multiple + * queues. Currently, we use just one queue (==FSYNC). + * + * XXX We later expect to have RECV, FSYNC and APPLY modes. + */ -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 28.03.2011 15:54, Simon Riggs wrote: > On Mon, Mar 28, 2011 at 1:14 PM, Robert Haas<robertmhaas@gmail.com> wrote: > >> but there is certainly no >> open item for adding additional sync rep modes. > > In your opinion. Huh? First you say that Robert added an open item about this to the list, he says that the open item wasn't about additional sync rep modes, and you say "in your opinion". > We will have to live with the UI for a long time, yes. I'm trying to > get it right, whether that includes adding an obvious/easy omission or > renaming things to make better sense. > > Your other changes make this sensible, and feedback I received after a > recent presentation tells me that people will expect it to work with > the two additional options. > > I would prefer further feedback before solidifying this design, but if > we must solidify it now, then I prefer to do that with all 5 options. > As originally submitted for this commit fest. It's too late to be doing this. The patch isn't ready to be committed, and there's high potential to introduce new bugs or usability issues. And regarding the UI, I'm not totally convinced that a four-state GUC set in the master is the way go. It would feel at least as logical to control this in the standby. I don't really want to get into that discussion, though. My point is that if we wanted to still sneak this in, then we would have to have those discussions. -1. Let's fix the existing issues, and release. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Mon, Mar 28, 2011 at 1:51 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: >>> The 'apply' mode would be quite interesting, it would make it easier to >>> build load-balancing clusters. But the patch isn't up to the task on that >>> yet - the 'apply' status report is only sent after >>> wal_receiver_status_interval fills up, so you get long delays. >> >> Yes it's up to the task, you misread it. It will continue sending >> replies while apply<> flush and then it will fall back to the >> behaviour you mention. > > There's nothing to wake up the walreceiver after applying a commit record. > > Oh, you're relying on the periodic wakeups specified by NAPTIME_PER_CYCLE > (100ms). That's still on average a 50ms delay to every commit. Rubbish. "Every commit", no way. How could you think that? That delay affects only the last commit of a sequence of commits after which the server goes quiet. And that only applies to people that have specifically chosen to wait for the apply, which as you say means they may have fairly long waits anyway. > We should try > to eliminate these polling loops, not make them more important. In fact, we > should raise NAPTIME_PER_CYCLE to, say, 1000ms, to reduce spurious wakeups > on an idle system. I'm OK with changing the polling loops. Is there a big problem there? I think it would take you about an hour to rewrite that, if you wanted to do so. But its not a necessary step to do that, especially when we're discussing whether Latches are actually as portable as we think. That sounds like more risk for a slight gain in performance. > Am I reading the patch correctly that if the standby hasn't applied all WAL > yet, you send a reply message at every wakeup, whether or not any progress > has been made since last time? So if you have a long-running-transaction in > the standby, for example, conflicting with WAL recovery, the standby will > keep sending a status report to the master every 100ms. Sure. First, is that a problem? Why? The WalReceiver isn't busy doing anything else at that point, by definition. The WalSender isn't doing anything then either, by definition. Both are used to higher send rates. Second, if that really is a problem, sounds like a simple "if" test added to reply code. Third, the conflict issues are much reduced as well in this release. For this exact purpose. So I don't see any blockers in what you say. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 28, 2011 at 2:05 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > It would feel at least as logical to control this in the standby. Now you are being ridiculous. You've spoken strongly against this at every single step of this journey. We're well passed the stage of putting anything in that could do that, as well you know. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 28.03.2011 16:11, Simon Riggs wrote: > On Mon, Mar 28, 2011 at 2:05 PM, Heikki Linnakangas > <heikki.linnakangas@enterprisedb.com> wrote: >> It would feel at least as logical to control this in the standby. > > Now you are being ridiculous. You've spoken strongly against this at > every single step of this journey. I was thinking specifically about whether flush vs. write (vs. apply, maybe) here. It would make sense to set that in the standby. You might even want to set it differently on different standbys. What I was strongly against is the action at a distance, where setting a GUC in a standby suddenly makes the master to wait for acks from that server. That's dangerous, but I don't see such danger in setting the level of synchronicity in the standby, once you've decided that it's a synchronous standby. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Mon, Mar 28, 2011 at 10:11 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 28.03.2011 16:11, Simon Riggs wrote: >> >> On Mon, Mar 28, 2011 at 2:05 PM, Heikki Linnakangas >> <heikki.linnakangas@enterprisedb.com> wrote: >>> >>> It would feel at least as logical to control this in the standby. >> >> Now you are being ridiculous. You've spoken strongly against this at >> every single step of this journey. > > I was thinking specifically about whether flush vs. write (vs. apply, maybe) > here. It would make sense to set that in the standby. You might even want to > set it differently on different standbys. > > What I was strongly against is the action at a distance, where setting a GUC > in a standby suddenly makes the master to wait for acks from that server. > That's dangerous, but I don't see such danger in setting the level of > synchronicity in the standby, once you've decided that it's a synchronous > standby. It might not be dangerous, but the standby currently sends write, flush, and apply positions back separately, so the master must decide which of those to pay attention to, unless we rework the whole design.I actually think the current design is quite nice andam in no hurry to rejigger that particular part of it. However, I do think that we may need or want to rejigger the timing of replies for performance. It might be, for example, that when waiting for the "write", the master should somehow indicate to the standby the earliest write-LSN that could release waiters, so that a standby can send back a reply the instant it hits that LSN, without waiting to finish reading everything that's buffered up as it does now. For apply, I agree with your feeling that the startup process needs to wake walreceiver via a latch when the apply position advances, but here again it might be beneficial to send the first interesting LSN from master to standby to cut down unnecessary wakeups. We also need to consider the issue raised elsewhere - that a naive implementation of this could allow the commit to become visible on the standby before it becomes visible on the master. That would be expensive to prevent, because you'd need an additional set of master-standby interlocks, but I think at least one person was arguing that it was necessary for correctness - my memory of the details is fuzzy at the moment. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 28, 2011 at 3:11 PM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > On 28.03.2011 16:11, Simon Riggs wrote: >> >> On Mon, Mar 28, 2011 at 2:05 PM, Heikki Linnakangas >> <heikki.linnakangas@enterprisedb.com> wrote: >>> >>> It would feel at least as logical to control this in the standby. >> >> Now you are being ridiculous. You've spoken strongly against this at >> every single step of this journey. > > I was thinking specifically about whether flush vs. write (vs. apply, maybe) > here. It would make sense to set that in the standby. You might even want to > set it differently on different standbys. > > What I was strongly against is the action at a distance, where setting a GUC > in a standby suddenly makes the master to wait for acks from that server. > That's dangerous, but I don't see such danger in setting the level of > synchronicity in the standby, once you've decided that it's a synchronous > standby. The action at a distance thought still applies, since you would wait more or less time depending upon how this parameter was set on the standby. I can't see how this situation differs. Your own argument still applies. I would point out that I argued against you, but was persuaded towards your approach. The code won't easily allow what you suggest. There were multiple approaches to implementation, but there aren't anymore. So you can't say the UI is unclear; its very clear how we implement things from here - the way this patch implements it - on the master. This is a simple patch, containing functionality already discussed and agreed and for which code was submitted in this CF. There's no reason to block this, only for us to decide the final naming of parameter/s and options. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Robert Haas <robertmhaas@gmail.com> wrote: > We also need to consider the issue raised elsewhere - that a naive > implementation of this could allow the commit to become visible on > the standby before it becomes visible on the master. That would > be expensive to prevent, because you'd need an additional set of > master-standby interlocks, but I think at least one person was > arguing that it was necessary for correctness - my memory of the > details is fuzzy at the moment. I remember expressing concern about that; I don't know if anyone else did. After some discussion, I was persuaded that the use cases where it would matter are narrow enough that documentation of the issue should be enough. -Kevin
On Mon, Mar 28, 2011 at 10:58 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > This is a simple patch, containing functionality already discussed and > agreed and for which code was submitted in this CF. These statements are simply not accurate. It isn't a simple patch, the details of how the write and apply modes should work haven't been fully discussed, and there is no version of your patch submitted for the last CommitFest which contains any of this functionality. Moreover, that CommitFest is OVER, so your repeated references to it as "this CF" rather than "the last CF" do not match my view of how the world works. > There's no reason to block this, only for us to decide the final > naming of parameter/s and options. At the end of the day, we make these decisions by consensus, and three people have said quite clearly that they believe it is too late to apply something like this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 28, 2011 at 3:42 PM, Robert Haas <robertmhaas@gmail.com> wrote: > It might not be dangerous, but the standby currently sends write, > flush, and apply positions back separately, so the master must decide > which of those to pay attention to, unless we rework the whole design. > I actually think the current design is quite nice and am in no hurry > to rejigger that particular part of it. In particular what I like about the current design is that there's no reason you shouldn't be able to change the commit durability setting per-transacion. You might want to have logging records be asynchronous, regular operations be synchronous on the master, and opeations involving money block until the slave has received them or synced them or even applied them. Or you might want to mark just the transactions affecting the data that your read-only queries which are load-balanced on slaves as blocking until the slave has applied them so people don't see inconsistent old data making it look like the transaction failed. -- greg
On Mon, Mar 28, 2011 at 4:15 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 28, 2011 at 10:58 AM, Simon Riggs <simon@2ndquadrant.com> wrote: >> This is a simple patch, containing functionality already discussed and >> agreed and for which code was submitted in this CF. > > These statements are simply not accurate. Then you are mistaken on every single point. > It isn't a simple patch, Yes, it is. Most of the patch is docs and GUC changes. The current patch was specifically designed by me to allow this to be added. One queue is replaced easily by 3 queues, which essentially touches only 2 places in the current code, sleep and wakeup. And it touches them in very simple ways. Variable to Array. Easy. > the details of how the write and apply modes should work haven't been > fully discussed, The original patch had all 3 modes side-by-side, all working identically. There has never been any objection or discussion about that and its been part of the design for months. What different approach is there? > and there is no version of your patch submitted for > the last CommitFest which contains any of this functionality. v9, submitted on 15 Jan contains this functionality. > Moreover, that CommitFest is OVER, so your repeated references to it > as "this CF" rather than "the last CF" do not match my view of how the > world works. We are still applying patches, for various reasons. I must have missed your objections to Tom's proposals to fix un-neat things about collations, or his additions to the extensions features. Go say what you've said here to him as well. >> There's no reason to block this, only for us to decide the final >> naming of parameter/s and options. > > At the end of the day, we make these decisions by consensus, and three > people have said quite clearly that they believe it is too late to > apply something like this. "Something like this". Well given that all of the facts on which you've based your decision are wrong, I expect you to revise your decision. There is nothing about this patch that makes it possible or sensible to exclude it. You wish to continue to discuss Network timeouts. Why are they "in" and this "out". Both were submitted as part of my original patch.... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
> On Sun, Mar 27, 2011 at 10:45 PM, Simon Riggs<simon@2ndquadrant.com> wrote: >> I was hoping to fine tune/tweak Sync Rep after feedback during beta, >> but my understanding of current consensus is that that will be too >> late to make user visible changes. So I'm proposing this change now, >> before Beta, rather than during Beta. >> > For what it's worth I think this is a simplification. We have: > > 1) Development when new features are added > > 2) Feature freeze - when those features are tweaked and fixed based on > our own testing but no new features added > > 3) Beta - when features are tweaked and fixed in response to user > suggestions but no new features added How to say this short: when testing the latest syncrep patches I've wasted time looking where the recv|fsync|apply api was, to find out it was gone. *shrug* didn't need it for my use case. But for others it might well be frustration to find out that what's currently called syncrep cannot be configured in a way it's suitable, and that they might have wasted considerable time while finding that out. The dba interface for recv|fsync|apply seems to be pretty stable, so supporting that for years should be without risk. How it works under the hood - the beta period seems like *the* opportunity to attrach mayor testing from all people waiting to get their hands on syncrep. An aspect of a good product is that it doesn't waste users time, like a good piece of code needs little comment. Alarm bells should go off when somebody is about to write a large piece of documentation writing what a feature doesn't support. It would be better to just support it (recv|fsync|apply), or no syncrep at all. Syncrep is incomplete without it. -- Yeb Havinga http://www.mgrid.net/ Mastering Medical Data
Yeb Havinga <yebhavinga@gmail.com> writes: > The dba interface for recv|fsync|apply seems to be pretty stable, so > supporting that for years should be without risk. How it works under the > hood - the beta period seems like *the* opportunity to attrach mayor testing > from all people waiting to get their hands on syncrep. +1 > It would be better to just support it (recv|fsync|apply), > or no syncrep at all. Syncrep is incomplete without it. Agreed. More than that, I think we should evaluate this patch on a cost/benefit ratio, rather than trying to apply to it all those procedural fences that we don't have, and that we don't have the size to benefit from. This whole thread only managed to raise the cost of the feature, but compared to its benefits, it's still a wash. Is there any good reason that I missed to ask all our users to wait for at best another year to get the SyncRep waiting behavior that makes sense and has been agreed on for a very long time already? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support PS : we already tweaked the UI in such a way that controling this feature from the standby makes no sense. What we talkedabout was to setup on the master which durability level you need, and on each standby which one you're able toprovide. Then you mix&match. That won't fly with current way to setup the sync standby list. So current recv|fsync|apply patch is IMO finishing the 9.1 work.
On Tue, Mar 29, 2011 at 3:49 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: >> It would be better to just support it (recv|fsync|apply), >> or no syncrep at all. Syncrep is incomplete without it. > > Agreed. I have trouble viewing the idea that it would be better not to ship sync rep at all than to add more features to it as a serious proposal.Presumably, anyone who is sad that sync rep doesn'thave all of the options they might want would be even sadder to hear that we went through a whole development cycle and ended up with nothing at all. Even if we did agree to take this patch, there will certainly be more features that someone might want and not have, such as the ability to sync with multiple standbys at once. > More than that, I think we should evaluate this patch on a cost/benefit > ratio, rather than trying to apply to it all those procedural fences > that we don't have, and that we don't have the size to benefit from. As a community, we've adopted a development plan that proceeds in cycles. For the last several releases, we have had four CommitFests in each release cycle, followed by a feature freeze and eventually by beta and final release. It's certainly a valid question to ask how well that procedure has served us. It does not seem likely to me that we can continue to produce quality releases if we don't at some point cut off the flow of new features into the source tree and work on stabilizing the code we've already got, and I believe the point for that was agreed by a large number of developers who sat in a room at PGCon last year to be on or about February 15th. We ended up extending that by a couple of weeks, to make sure that we had a process that was FAIR: we didn't want patches that had been in the pipeline for a very long time to get postponed to 9.2 because no committer had had a chance to work on them yet. However, we also bumped MANY patches to 9.2 because they weren't in sufficiently good shape soon enough. If we accept this patch now because a bunch of people say they really, really want it, isn't that unfair to the people to whom we've already said "sorry, the deadline has passed"? Of course, there is always going to be some gray area. I argued for committing the replication_timeout patch because I believe the fact that we haven't got that feature is almost a bug - it interferes significantly with the usability of replication in general, and it will be an even more serious problem with sync rep, where a hung standby connection will not only mean that nothing is replicating but also that no write transactions can be processed at all. However, you could make the opposite argument - that it's really a new feature - and therefore we ought not to commit it. So far no one has taken that position, but it's certainly a reasonable argument. Likewise, there is ongoing discussion on the collations thread about which of those changes are necessary for this release, and which ones are things that ought to be postponed to a future release. I haven't gotten too involved in those discussions because I don't really understand the underlying issues, but I think that's an important discussion. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > If we accept this patch now because a bunch of > people say they really, really want it, isn't that unfair to the > people to whom we've already said "sorry, the deadline has passed"? No, because each time we're talking procedures we're forgetting about a simple fact. Commiters have the direct responsibility of the code, that is why pushing work from non-commiters takes so much time. Commiting your own code, you don't have a steep learning curve, and you don't have to understand the use case and get convinced. So the rules are not the same for commiter patches and contributor patches, and there's no good in trying to have them the same or pretending they are. In particular, only commiters are able to finish and polish the work between the last commit fest and beta, and then they will be on the hook to get to release candidate and release. But you know all that better than I do. I don't want a release as soon as possible, I want the best we are able to provide, and I think adding in current $subject patch helps reaching this goal. <include "baring show stoppers" QA disclaimer> Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Tue, Mar 29, 2011 at 12:57 PM, Robert Haas <robertmhaas@gmail.com> wrote: > However, we also > bumped MANY patches to 9.2 because they weren't in sufficiently good > shape soon enough. If we accept this patch now because a bunch of > people say they really, really want it, isn't that unfair to the > people to whom we've already said "sorry, the deadline has passed"? > > Of course, there is always going to be some gray area. I argued for > committing the replication_timeout patch because I believe the fact > that we haven't got that feature is almost a bug - it interferes > significantly with the usability of replication in general, and it > will be an even more serious problem with sync rep, where a hung > standby connection will not only mean that nothing is replicating but > also that no write transactions can be processed at all. However, you > could make the opposite argument - that it's really a new feature - > and therefore we ought not to commit it. So far no one has taken that > position, but it's certainly a reasonable argument. The questions and issues you raise are real and I have no idea how to judge them. All I know is that if we spent less time discussing procedural issues, we'd get a lot more done and much more amicably also. I think "What makes PostgreSQL better?". I think about a rounded feature set and treat that just as I would a bug. I know that's not the same for everybody. I'm happy there are people looking at replication timeouts also. Regrettably I don't have enough time for everything I would like to see. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Mar 29, 2011 at 10:48 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > So the rules are not the same for commiter patches and contributor > patches, and there's no good in trying to have them the same or > pretending they are. In particular, only commiters are able to finish > and polish the work between the last commit fest and beta, and then they > will be on the hook to get to release candidate and release. > > But you know all that better than I do. Committers can and do get away with slipping things in later than non-committers, and to some extent that's OK for the reasons you mention. But Alvaro was very gracious in conceding that it was a bit too late to push in his key lock patch, as was his employer, JD. They didn't like it, but they accepted that it was necessary to move the community, overall, forward, and to avoid a really long beta period during which, really, nobody gets to do anything at all interesting. We cannot have one standard for features that CommandPrompt really wants committed and a different standard for features that 2ndQuadrant or, say, EnterpriseDB, really want committed. I completely disagree that committers are the only ones who can finish and polish work between the last CommiFest and beta. Fujii Masao, Kevin Grittner, Yeb Havinga, and Yamamoto Takashi all come to mind as people who have been very, very helpful in moving us toward beta through careful testing and code review. I have no fear at all about our ability to maintain SSI even though there is not one committer who fully understands it all, because every bug report that comes in gets a response within hours and a patch within days. The limiting factor there has actually been how long it's taken someone to look and test those patches, not how quickly they've been produced. I think the reality is exactly the other way around: committers are not the people who get the opportunity to fix other people's bugs; they are the people who are *expected* to fix other people's bugs when no one else will. If it's your perception that the (mostly quite minor) changes that I've made to sync rep are somehow for purposes of self-aggrandizement or a desire to micromanage everything that happens in the backend, then I'm sorry for that. I'll readily admit that I have strong opinions on lots of topics, especially but not only PostgreSQL-related topics; but I would be way happier to have spent the last couple of weeks developing new features than swatting bugs. Had I done that, though, I think that not as many bugs would have gotten swatted. So I did it. Whether that makes me a helpful community guy who tries to ensure a quality release or a total jerk who interjects his nose into other people's business is, of course, a matter of opinion. Even today, anyone who would like to write a patch to address more than one of the open items is more than welcome to do so, and I would really appreciate it, even I or someone else ends up having to adjust it a bit before committing. There are at least three issues on the open items list that are obvious candidates for someone to pick up: - fix attinhcount tracking - Typed-tables patch broke pg_upgrade - comments on SQL/MED objects I volunteered to pick up the last one, but I'd be more than happy if the person who reported the problem had already provided the patch. Or if someone else wanted to write the patch. That would be awesome. In my view, the question we should be asking ourselves here is not - why are Tom and Robert getting to make all these commits? - but - where is everybody else who should be helping out? If the answer is "well we don't have time to work on this because we all have day jobs we have to do to get paid", then I accept that. But that moves getting to commit changes at a late date from the "privilege" bucket into the "responsibility" bucket. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 29, 2011 at 5:40 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Mar 29, 2011 at 10:48 AM, Dimitri Fontaine > <dimitri@2ndquadrant.fr> wrote: >> So the rules are not the same for commiter patches and contributor >> patches, and there's no good in trying to have them the same or >> pretending they are. In particular, only commiters are able to finish >> and polish the work between the last commit fest and beta, and then they >> will be on the hook to get to release candidate and release. >> >> But you know all that better than I do. > > Committers can and do get away with slipping things in later than > non-committers, and to some extent that's OK for the reasons you > mention. But Alvaro was very gracious in conceding that it was a bit > too late to push in his key lock patch, as was his employer, JD. They > didn't like it, but they accepted that it was necessary to move the > community, overall, forward, and to avoid a really long beta period > during which, really, nobody gets to do anything at all interesting. > We cannot have one standard for features that CommandPrompt really > wants committed and a different standard for features that 2ndQuadrant > or, say, EnterpriseDB, really want committed. > > I completely disagree that committers are the only ones who can finish > and polish work between the last CommiFest and beta. Fujii Masao, > Kevin Grittner, Yeb Havinga, and Yamamoto Takashi all come to mind as > people who have been very, very helpful in moving us toward beta > through careful testing and code review. I have no fear at all about > our ability to maintain SSI even though there is not one committer who > fully understands it all, because every bug report that comes in gets > a response within hours and a patch within days. The limiting factor > there has actually been how long it's taken someone to look and test > those patches, not how quickly they've been produced. I think the > reality is exactly the other way around: committers are not the people > who get the opportunity to fix other people's bugs; they are the > people who are *expected* to fix other people's bugs when no one else > will. If it's your perception that the (mostly quite minor) changes > that I've made to sync rep are somehow for purposes of > self-aggrandizement or a desire to micromanage everything that happens > in the backend, then I'm sorry for that. I'll readily admit that I > have strong opinions on lots of topics, especially but not only > PostgreSQL-related topics; but I would be way happier to have spent > the last couple of weeks developing new features than swatting bugs. > Had I done that, though, I think that not as many bugs would have > gotten swatted. So I did it. Whether that makes me a helpful > community guy who tries to ensure a quality release or a total jerk > who interjects his nose into other people's business is, of course, a > matter of opinion. > > Even today, anyone who would like to write a patch to address more > than one of the open items is more than welcome to do so, and I would > really appreciate it, even I or someone else ends up having to adjust > it a bit before committing. There are at least three issues on the > open items list that are obvious candidates for someone to pick up: > > - fix attinhcount tracking > - Typed-tables patch broke pg_upgrade > - comments on SQL/MED objects > > I volunteered to pick up the last one, but I'd be more than happy if > the person who reported the problem had already provided the patch. > Or if someone else wanted to write the patch. That would be awesome. > In my view, the question we should be asking ourselves here is not - > why are Tom and Robert getting to make all these commits? - but - > where is everybody else who should be helping out? If the answer is > "well we don't have time to work on this because we all have day jobs > we have to do to get paid", then I accept that. But that moves > getting to commit changes at a late date from the "privilege" bucket > into the "responsibility" bucket. Robert, Everybody wants us to be polite and respectful with each other. Writing such long emails seems to be just filibustering to me. I doubt anyone has read and considered every word, there are just too many. A form of disrespect. Main thing I note is that you could have reviewed my patch in the time its taken to discuss these procedural "issues". Why are they more important? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 3/29/11 7:48 AM, Dimitri Fontaine wrote: > I don't want a release as soon as possible, I want the best we are able > to provide, and I think adding in current $subject patch helps reaching > this goal. <include "baring show stoppers" QA disclaimer> There will *always* be more work we can do on sync rep. If we hold the release until we're done tinkering with it, we'll never release. Our project has had a chronic issue with not being able to progress from feature freeze to release in a timely fashion for years, and the sort of argument expressed above isn't helping. The relevant question is: is sync rep unusable by a large portion of our users because this feature was stripped from what got committed, or is it a value-add feature which makes synch rep nicer? If the former, it's a bug and we should patch it; if the latter, it should wait until 9.2. > Writing such long emails seems to be just filibustering to me. I doubt > anyone has read and considered every word, there are just too many. A > form of disrespect. Simon, Robert has been nothing but respectful to you. You can't accuse him of being rude and hostile just because he disagrees with you. Overselling his case, certainly. But very politely. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Tue, Mar 29, 2011 at 2:48 PM, Josh Berkus <josh@agliodbs.com> wrote: >> Writing such long emails seems to be just filibustering to me. I doubt >> anyone has read and considered every word, there are just too many. A >> form of disrespect. > > Simon, Robert has been nothing but respectful to you. You can't accuse > him of being rude and hostile just because he disagrees with you. > Overselling his case, certainly. But very politely. If hostile's wanted, then we can come up with something hostile ;-). Much better is to try to take a different perspective. There are a number of features that were *turned down* for 9.1, deferred for 9.2, because they weren't sufficiently ready at the appointed time. There were 26 patches "Returned with Feedback"; there are likely several of them which, if given the special handling given to Synchronous Replication, could have been drawn into 9.1. It's only fair to try to get the release out, so that these people that have been waiting patiently have opportunity to get their submissions into 9.2. The longer they wait, the more reason to get hostile for better reason... -- http://linuxfinances.info/info/linuxdistributions.html
On Tue, Mar 29, 2011 at 7:48 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 3/29/11 7:48 AM, Dimitri Fontaine wrote: >> I don't want a release as soon as possible, I want the best we are able >> to provide, and I think adding in current $subject patch helps reaching >> this goal. <include "baring show stoppers" QA disclaimer> > > There will *always* be more work we can do on sync rep. If we hold the > release until we're done tinkering with it, we'll never release. Our > project has had a chronic issue with not being able to progress from > feature freeze to release in a timely fashion for years, and the sort of > argument expressed above isn't helping. > > The relevant question is: is sync rep unusable by a large portion of our > users because this feature was stripped from what got committed, or is > it a value-add feature which makes synch rep nicer? If the former, it's > a bug and we should patch it; if the latter, it should wait until 9.2. That's not relevant. Can something small be added for large benefit: yes! It's a complete misrepresentation to suggest this would hold up the release in any way and all the actual technical comments have been easily dismissed. We've already used more time writing these emails than it took me to write the patch (< 2 hours). This still can be reviewed and committed easily. This is not a new feature. It's something that's been in the design for months, was submitted in the recent CF and is a minor change with low risk. Given we've all been here before, I've done my homework on what is acceptable at this stage and this passes. There is nothing different between this and the replication timeout patch. I'm not suggesting that should be yanked as well, BTW. And I'm not getting paid a single penny for this. Just me, trying to add value for the PostgreSQL project. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Mar 29, 2011 at 7:48 PM, Josh Berkus <josh@agliodbs.com> wrote: >> Writing such long emails seems to be just filibustering to me. I doubt >> anyone has read and considered every word, there are just too many. A >> form of disrespect. > > Simon, Robert has been nothing but respectful to you. You can't accuse > him of being rude and hostile just because he disagrees with you. > Overselling his case, certainly. But very politely. I haven't accused Robert of being rude or hostile, when did I say that? He seems perfectly polite to me. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Mar 29, 2011 at 8:57 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Mar 29, 2011 at 3:49 AM, Dimitri Fontaine > <dimitri@2ndquadrant.fr> wrote: >>> It would be better to just support it (recv|fsync|apply), >>> or no syncrep at all. Syncrep is incomplete without it. >> >> Agreed. > > I have trouble viewing the idea that it would be better not to ship > sync rep at all than to add more features to it as a serious proposal. > Presumably, anyone who is sad that sync rep doesn't have all of the > options they might want would be even sadder to hear that we went > through a whole development cycle and ended up with nothing at all. > Even if we did agree to take this patch, there will certainly be more > features that someone might want and not have, such as the ability to > sync with multiple standbys at once. > >> More than that, I think we should evaluate this patch on a cost/benefit >> ratio, rather than trying to apply to it all those procedural fences >> that we don't have, and that we don't have the size to benefit from. > > As a community, we've adopted a development plan that proceeds in > cycles. For the last several releases, we have had four CommitFests > in each release cycle, followed by a feature freeze and eventually by > beta and final release. It's certainly a valid question to ask how > well that procedure has served us. It does not seem likely to me that > we can continue to produce quality releases if we don't at some point > cut off the flow of new features into the source tree and work on > stabilizing the code we've already got, and I believe the point for > that was agreed by a large number of developers who sat in a room at > PGCon last year to be on or about February 15th. We ended up > extending that by a couple of weeks, to make sure that we had a > process that was FAIR: we didn't want patches that had been in the > pipeline for a very long time to get postponed to 9.2 because no > committer had had a chance to work on them yet. However, we also > bumped MANY patches to 9.2 because they weren't in sufficiently good > shape soon enough. If we accept this patch now because a bunch of > people say they really, really want it, isn't that unfair to the > people to whom we've already said "sorry, the deadline has passed"? > > Of course, there is always going to be some gray area. I argued for > committing the replication_timeout patch because I believe the fact > that we haven't got that feature is almost a bug - it interferes > significantly with the usability of replication in general, and it > will be an even more serious problem with sync rep, where a hung > standby connection will not only mean that nothing is replicating but > also that no write transactions can be processed at all. However, you > could make the opposite argument - that it's really a new feature - > and therefore we ought not to commit it. So far no one has taken that > position, but it's certainly a reasonable argument. Likewise, there > is ongoing discussion on the collations thread about which of those > changes are necessary for this release, and which ones are things that > ought to be postponed to a future release. I haven't gotten too > involved in those discussions because I don't really understand the > underlying issues, but I think that's an important discussion. I'm very excited about new options, especially recv. But I agree with Robert and Heikki because what the patch provides looks like new feature rather than bug fix. And I think that we still require some discussions of the design; how far transactions must wait for sync rep in recv mode? In the patch, they wait for WAL to be written in the standby, but I think that they should wait until walreceiver has recieved WAL instead. That would increase the performance of sync rep. Anyway, I don't think now is time to discuss about such a design except for bug fix. I like those additional options, but I believe that sync rep which we worked out is still useful without them. Replication timeout looks like a bug fix rather than new feature. Without that, walsender might unexpectedly remain for a while when the standby crashes or the network outage happens. As Robert said, sync rep can get stuck for a long while because of such a remaining walsender. What's the worse is that when hot_standby_feedback is enabled, such a remaining walsender would prevent oldest xmin from advancing and interfere with vacuuming on the master. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
On Wed, Mar 30, 2011 at 5:29 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > I'm very excited about new options, especially recv. But I agree with > Robert and Heikki because what the patch provides looks like new > feature rather than bug fix. And I think that we still require some > discussions of the design; how far transactions must wait for sync > rep in recv mode? In the patch, they wait for WAL to be written in > the standby, but I think that they should wait until walreceiver has > recieved WAL instead. That would increase the performance of sync > rep. Anyway, I don't think now is time to discuss about such a design > except for bug fix. Not waiting for write would just be much less safe and would not have any purpose as a sync rep option. The difference in time would be very marginal also. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services