Re: Sync Rep for 2011CF1

Here's the latest patch for sync rep.

>From here, I will be developing the patch further on public git
repository towards commit. My expectation is that commit is at least 2
weeks away, though there are no major unresolved problems. I expect
essential follow on patches to continue for a further 2-4 weeks after
that first commit.

I will add my own reviewer's notes tomorrow.

In terms of testing, the patch hasn't been tested further than my own
laptop as yet, so it seems likely there's a few trivial howlers in
there. That is simply because of my recent flu.

I've requested Heikki as main reviewer and he's accepted. Other comments
are also welcome about the user interface and the reply protocol are
also welcome. Please don't bother performance testing yet. I'll let you
know when that is appropriate.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Sat, Jan 15, 2011 at 22:40, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> Here's the latest patch for sync rep.
>
> >From here, I will be developing the patch further on public git
> repository towards commit. My expectation is that commit is at least 2

That's great. Just one tiny detail - which repository and which branch? ;)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

(grr, I wrote this on Monday already, but just found it in my drafts
folder, unsent)

On 15.01.2011 23:40, Simon Riggs wrote:
>
> Here's the latest patch for sync rep.
>
>> From here, I will be developing the patch further on public git
> repository towards commit. My expectation is that commit is at least 2
> weeks away, though there are no major unresolved problems. I expect
> essential follow on patches to continue for a further 2-4 weeks after
> that first commit.

Thanks! Some quick observations after first read-through:

* The docs for synchronous_replication still claim that it means two
different things in master and standby. Looking at the code, I believe
that's not true anymore.

* it seems like overkill to not let clients to even connect when
allow_standalone_primary=off and no synchronous standbys are available.
What if you just want to run a read-only query?

* Please separate the hot standby feedback loop into a separate patch on
top of the synch rep patch. I know it's not a lot of code, but it's
still easier to handle features separately.

* The UI differs from what was agreed on here:
http://archives.postgresql.org/message-id/4D1DCF5A.7070808@enterprisedb.com.

* Instead of the short-circuit for autovacuum in SyncRepWaitOnQueue(),
it's probably better to set synchronous_commit=off locally when the
autovacuum process starts.

* the "queue id" thing is dead code at the moment, as there is only one
queue. I gather this is a leftover from having different queues for
"apply", "sync", "write" modes, but I think it would be better to just
remove it for now.

PS, I'm surprised how small this patch is. Thinking about it some more,
I don't know why I expected this to be a big patch.

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

On Fri, 2011-01-21 at 14:45 +0200, Heikki Linnakangas wrote:
> (grr, I wrote this on Monday already, but just found it in my drafts
> folder, unsent)

No worries, thanks for commenting.

> Thanks! Some quick observations after first read-through:
>
> * The docs for synchronous_replication still claim that it means two
> different things in master and standby. Looking at the code, I believe
> that's not true anymore.

Probably. The docs changed so many times I had gone "code-blind".

> * it seems like overkill to not let clients to even connect when
> allow_standalone_primary=off and no synchronous standbys are available.
> What if you just want to run a read-only query?

That's what Aidan requested, I agreed and so its there. You're using
sync rep because of writes, so you have a read-write app. If you allow
connections then half of the app will work, half will not. Half-working
isn't very useful, as Aidan eloquently explained. If your app is all
read-only you wouldn't be using sync rep anyway. That's the argument,
but I've not got especially strong feelings it has to be this way.

Perhaps discuss that on a separate thread? See what everyone thinks?

> * Please separate the hot standby feedback loop into a separate patch on
> top of the synch rep patch. I know it's not a lot of code, but it's
> still easier to handle features separately.

I tried to do that initially, but there is interaction between those
features. The way I have it is that the replies from the standby act as
keepalives to the master. So the hot standby feedback is just an extra
parameter and an extra field. Removing that doesn't really make the
patch any easier to understand.

> * The UI differs from what was agreed on here:
> http://archives.postgresql.org/message-id/4D1DCF5A.7070808@enterprisedb.com.

You mean synchronous_standbys is not there yet? Yes, I know. It can be
added after we commit this, its only a small bit of code and no
dependencies. I figured we had bigger things to agree first.

> * Instead of the short-circuit for autovacuum in SyncRepWaitOnQueue(),
> it's probably better to set synchronous_commit=off locally when the
> autovacuum process starts.

Even better plan, thanks.

> * the "queue id" thing is dead code at the moment, as there is only one
> queue. I gather this is a leftover from having different queues for
> "apply", "sync", "write" modes, but I think it would be better to just
> remove it for now.

It's a trivial patch to add options to either fsync or apply, so I was
expecting to add that back in this release also.

> PS, I'm surprised how small this patch is. Thinking about it some more,
> I don't know why I expected this to be a big patch.

Yes, it's the decisions which seem fairly big this time.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 21, 2011 at 7:45 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> * it seems like overkill to not let clients to even connect when
> allow_standalone_primary=off and no synchronous standbys are available. What
> if you just want to run a read-only query?

For what it's worth, +1.

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

On Fri, Jan 21, 2011 at 14:24, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Fri, 2011-01-21 at 14:45 +0200, Heikki Linnakangas wrote:
>> * it seems like overkill to not let clients to even connect when
>> allow_standalone_primary=off and no synchronous standbys are available.
>> What if you just want to run a read-only query?
>
> That's what Aidan requested, I agreed and so its there. You're using
> sync rep because of writes, so you have a read-write app. If you allow
> connections then half of the app will work, half will not. Half-working
> isn't very useful, as Aidan eloquently explained. If your app is all
> read-only you wouldn't be using sync rep anyway. That's the argument,
> but I've not got especially strong feelings it has to be this way.
>
> Perhaps discuss that on a separate thread? See what everyone thinks?

I'll respond here once, and we'll see if more people want to comment
then we can move it :-)

Doesn't this make a pretty strange assumption - namely that you have a
single application? We support multiple databases, and multiple users,
and multiple pretty much anything - in most cases, people deploy
multiple apps. (They may well be part of the same "solution" or
whatever you want to call it, but parts may well be readonly - like a
reporting app, or even just a monitoring client)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On 21.01.2011 15:24, Simon Riggs wrote:
> On Fri, 2011-01-21 at 14:45 +0200, Heikki Linnakangas wrote:
>> * it seems like overkill to not let clients to even connect when
>> allow_standalone_primary=off and no synchronous standbys are available.
>> What if you just want to run a read-only query?
>
> That's what Aidan requested, I agreed and so its there. You're using
> sync rep because of writes, so you have a read-write app. If you allow
> connections then half of the app will work, half will not. Half-working
> isn't very useful, as Aidan eloquently explained. If your app is all
> read-only you wouldn't be using sync rep anyway. That's the argument,
> but I've not got especially strong feelings it has to be this way.

It's also possible that most of your transactions in fact do "set
synchronous_replication=off", and only a few actually do synchronous
replication. It would be pretty bad to not allow connections in that
case. And what if you want to connect to the server to diagnose the
issue? Oh, you can't... Besides, we're not kicking out existing
connections, are we? Seems inconsistent to let the old connections live.

IMHO the only reasonable option is to allow connections as usual, and
only fail (or block forever) at COMMIT.

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

On Fri, Jan 21, 2011 at 10:33 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> It's also possible that most of your transactions in fact do "set
> synchronous_replication=off", and only a few actually do synchronous
> replication. It would be pretty bad to not allow connections in that case.
> And what if you want to connect to the server to diagnose the issue? Oh, you
> can't... Besides, we're not kicking out existing connections, are we? Seems
> inconsistent to let the old connections live.
>
> IMHO the only reasonable option is to allow connections as usual, and only
> fail (or block forever) at COMMIT.

Another point is that the synchronous standby could come back at any
time. There's no reason not to let the client do all the work they
want up until the commit - maybe the standby will pop back up before
the COMMIT actually issued. Or even if it doesn't, as soon as it pops
back up, all those COMMITs get released.

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

On Fri, 2011-01-21 at 17:33 +0200, Heikki Linnakangas wrote:
> On 21.01.2011 15:24, Simon Riggs wrote:
> > On Fri, 2011-01-21 at 14:45 +0200, Heikki Linnakangas wrote:
> >> * it seems like overkill to not let clients to even connect when
> >> allow_standalone_primary=off and no synchronous standbys are available.
> >> What if you just want to run a read-only query?
> >
> > That's what Aidan requested, I agreed and so its there. You're using
> > sync rep because of writes, so you have a read-write app. If you allow
> > connections then half of the app will work, half will not. Half-working
> > isn't very useful, as Aidan eloquently explained. If your app is all
> > read-only you wouldn't be using sync rep anyway. That's the argument,
> > but I've not got especially strong feelings it has to be this way.
>
> It's also possible that most of your transactions in fact do "set
> synchronous_replication=off", and only a few actually do synchronous
> replication. It would be pretty bad to not allow connections in that
> case. And what if you want to connect to the server to diagnose the
> issue? Oh, you can't... Besides, we're not kicking out existing
> connections, are we? Seems inconsistent to let the old connections live.
>
> IMHO the only reasonable option is to allow connections as usual, and
> only fail (or block forever) at COMMIT.

We all think our own proposed options are the only reasonable thing, but
that helps us not at all in moving forwards. I've put much time into
delivering options many other people want, so there is a range of
function. I think we should hear from Aidan first before we decide to
remove that aspect.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, 2011-01-21 at 14:34 +0100, Magnus Hagander wrote:
> On Fri, Jan 21, 2011 at 14:24, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Fri, 2011-01-21 at 14:45 +0200, Heikki Linnakangas wrote:
> >> * it seems like overkill to not let clients to even connect when
> >> allow_standalone_primary=off and no synchronous standbys are available.
> >> What if you just want to run a read-only query?
> >
> > That's what Aidan requested, I agreed and so its there. You're using
> > sync rep because of writes, so you have a read-write app. If you allow
> > connections then half of the app will work, half will not. Half-working
> > isn't very useful, as Aidan eloquently explained. If your app is all
> > read-only you wouldn't be using sync rep anyway. That's the argument,
> > but I've not got especially strong feelings it has to be this way.
> >
> > Perhaps discuss that on a separate thread? See what everyone thinks?
>
> I'll respond here once, and we'll see if more people want to comment
> then we can move it :-)
>
> Doesn't this make a pretty strange assumption - namely that you have a
> single application? We support multiple databases, and multiple users,
> and multiple pretty much anything - in most cases, people deploy
> multiple apps. (They may well be part of the same "solution" or
> whatever you want to call it, but parts may well be readonly - like a
> reporting app, or even just a monitoring client)

There are various problems whatever we do. If we don't like one way, we
must balance that by judging what happens if we do things the other way.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 21, 2011 at 11:59 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> We all think our own proposed options are the only reasonable thing, but
> that helps us not at all in moving forwards. I've put much time into
> delivering options many other people want, so there is a range of
> function. I think we should hear from Aidan first before we decide to
> remove that aspect.

Since invited, I'll describe what I *want* do to do. I understand I
may not get it ;-)

When no sync slave is connected, yes, I want to stop things hard. I
don't mind read-only queries working, but what I want to avoid (if
possible) is having the master do lots of inserts/updates/deletes for
clients, fsyncing them all to disk (so on some strange event causing
recovery they'll be considered commit) and just delay the commit
return until it has a valid sync slave connected and caught up again.
And *I*'ld prefer if client transactions get errors right away rather
than begin to hang if a sync slave is not connected.

Even with single server, there's the window where stuff could be
"committed" but the client not notified yet. And that leads to
transactions which need to be verified. And with sync rep, that
window get's a little larger. But I'ld prefer not to make it a hanger
door, *especially* when it gets flung open at the point where the shit
has hit the fan and we're in the midst of switching over to manual
processing...

So, in my case, I'ld like it if PG couldn't do anything to generate
any user-initiated WAL unless there is a sync slave connected. Yes, I
understand that leads to hard-fail, and yes, I understand I'm in the
minority, maybe almost singular in that desire.

a.

--
Aidan Van Dyk                                             Create like a god,
aidan(at)highrise(dot)ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

On Fri, Jan 21, 2011 at 12:23 PM, Aidan Van Dyk <aidan(at)highrise(dot)ca> wrote:
> On Fri, Jan 21, 2011 at 11:59 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
>> We all think our own proposed options are the only reasonable thing, but
>> that helps us not at all in moving forwards. I've put much time into
>> delivering options many other people want, so there is a range of
>> function. I think we should hear from Aidan first before we decide to
>> remove that aspect.
>
> Since invited, I'll describe what I *want* do to do.  I understand I
> may not get it ;-)
>
> When no sync slave is connected, yes, I want to stop things hard.  I
> don't mind read-only queries working, but what I want to avoid (if
> possible) is having the master do lots of inserts/updates/deletes for
> clients, fsyncing them all to disk (so on some strange event causing
> recovery they'll be considered commit) and just delay the commit
> return until it has a valid sync slave connected and caught up again.
> And *I*'ld prefer if client transactions get errors right away rather
> than begin to hang if a sync slave is not connected.
>
> Even with single server, there's the window where stuff could be
> "committed" but the client not notified yet.  And that leads to
> transactions which need to be verified.  And with sync rep, that
> window get's a little larger.  But I'ld prefer not to make it a hanger
> door, *especially* when it gets flung open at the point where the shit
> has hit the fan and we're in the midst of switching over to manual
> processing...
>
> So, in my case, I'ld like it if PG couldn't do anything to generate
> any user-initiated WAL unless there is a sync slave connected.  Yes, I
> understand that leads to hard-fail, and yes, I understand I'm in the
> minority, maybe almost singular in that desire.

What you're proposing is to fail things earlier than absolutely
necessary (when they try to XLOG, rather than at commit) but still
later than what I think Simon is proposing (not even letting them log
in).

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jan 21, 2011 at 12:23 PM, Aidan Van Dyk <aidan(at)highrise(dot)ca> wrote:
>> When no sync slave is connected, yes, I want to stop things hard.

> What you're proposing is to fail things earlier than absolutely
> necessary (when they try to XLOG, rather than at commit) but still
> later than what I think Simon is proposing (not even letting them log
> in).

I can't see a reason to disallow login, because read-only transactions
can still run in such a situation --- and, indeed, might be fairly
essential if you need to inspect the database state on the way to fixing
the replication problem. (Of course, we've already had the discussion
about it being a terrible idea to configure replication from inside the
database, but that doesn't mean there might not be views or status you
would wish to look at.)

regards, tom lane

On Fri, Jan 21, 2011 at 1:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jan 21, 2011 at 12:23 PM, Aidan Van Dyk <aidan(at)highrise(dot)ca> wrote:
>>> When no sync slave is connected, yes, I want to stop things hard.
>
>> What you're proposing is to fail things earlier than absolutely
>> necessary (when they try to XLOG, rather than at commit) but still
>> later than what I think Simon is proposing (not even letting them log
>> in).
>
> I can't see a reason to disallow login, because read-only transactions
> can still run in such a situation --- and, indeed, might be fairly
> essential if you need to inspect the database state on the way to fixing
> the replication problem.  (Of course, we've already had the discussion
> about it being a terrible idea to configure replication from inside the
> database, but that doesn't mean there might not be views or status you
> would wish to look at.)

And just disallowing new logins is probably not even enough, because
it allows current logged in clients "forward progress", leading
towards an eventual hang (with now committed data on the master).

Again, I'm trying to stop "forward progress" as soon as possible when
a sync slave isn't replicating. And I'ld like clients to fail with
errors sooner (hopefully they get to the commit point) rather than
accumulate the WAL synced to the master and just wait at the commit.

So I think that's a more complete picture of my quick "not do anything
with no synchronous slave replicating" that I think was what led to
the no-login approach.

a.

--
Aidan Van Dyk                                             Create like a god,
aidan(at)highrise(dot)ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

On Fri, Jan 21, 2011 at 1:09 PM, Aidan Van Dyk <aidan(at)highrise(dot)ca> wrote:
> On Fri, Jan 21, 2011 at 1:03 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Fri, Jan 21, 2011 at 12:23 PM, Aidan Van Dyk <aidan(at)highrise(dot)ca> wrote:
>>>> When no sync slave is connected, yes, I want to stop things hard.
>>
>>> What you're proposing is to fail things earlier than absolutely
>>> necessary (when they try to XLOG, rather than at commit) but still
>>> later than what I think Simon is proposing (not even letting them log
>>> in).
>>
>> I can't see a reason to disallow login, because read-only transactions
>> can still run in such a situation --- and, indeed, might be fairly
>> essential if you need to inspect the database state on the way to fixing
>> the replication problem.  (Of course, we've already had the discussion
>> about it being a terrible idea to configure replication from inside the
>> database, but that doesn't mean there might not be views or status you
>> would wish to look at.)
>
> And just disallowing new logins is probably not even enough, because
> it allows current logged in clients "forward progress", leading
> towards an eventual hang (with now committed data on the master).
>
> Again, I'm trying to stop "forward progress" as soon as possible when
> a sync slave isn't replicating.  And I'ld like clients to fail with
> errors sooner (hopefully they get to the commit point) rather than
> accumulate the WAL synced to the master and just wait at the commit.
>
> So I think that's a more complete picture of my quick "not do anything
> with no synchronous slave replicating" that I think was what led to
> the no-login approach.

Well, stopping all WAL activity with an error sounds *more* reasonable
than refusing all logins, but I'm not personally sold on it. For
example, a brief network disruption on the connection between master
and standby would cause the master to grind to a halt... and then
almost immediately resume operations. More generally, if you have
short-running transactions, there's not much difference between
wait-at-commit and wait-at-WAL, and if you have long-running
transactions, then wait-at-WAL might be gumming up the works more than
necessary.

One idea might be to wait both before and after commit. If
allow_standalone_primary is off, and a commit is attempted, we check
whether there's a slave connected, and if not, wait for one to
connect. Then, we write and sync the commit WAL record. Next, we
wait for the WAL to be ack'd. Of course, the standby might disappear
between the first check and the second, but it would greatly reduce
the possibility of the master being ahead of the standby after a
crash, which might be useful for some people.

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

On Fri, Jan 21, 2011 at 1:32 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>> Again, I'm trying to stop "forward progress" as soon as possible when
>> a sync slave isn't replicating.  And I'ld like clients to fail with
>> errors sooner (hopefully they get to the commit point) rather than
>> accumulate the WAL synced to the master and just wait at the commit.

> Well, stopping all WAL activity with an error sounds *more* reasonable
> than refusing all logins, but I'm not personally sold on it.  For
> example, a brief network disruption on the connection between master
> and standby would cause the master to grind to a halt... and then
> almost immediately resume operations.

Yup. And I'm OK with that. In my case, it would be much better to
have a few quick failures, which can complete automatically a few
seconds later then to have a big buildup of transactions to re-verify
by hand upon starting manual processing.

But again, I'll stress that I'm talking about whe the master has no
sync slave connected. a "brief netowrk disruption" between the
master/slave isn't likely going to disconnect the slave. TCP is
pretty good at handling those. If the master thinks it has a sync
slave connected, I'm fine with it continuing to queue WAL for it even
if it's lagging noticeably.

>  More generally, if you have
> short-running transactions, there's not much difference between
> wait-at-commit and wait-at-WAL, and if you have long-running
> transactions, then wait-at-WAL might be gumming up the works more than
> necessary.

Again, when there is not sync slave *connected*, I don't want to wait
*at all*. I want to fail ASAP. If there is a sync slave, and it's
just slow, I don't really care where it waits.

From my experience, if the slave is not connected (i.e TCP connection
has been disconnected), then we're in something like:

1) Proper slave shutdown: pilot error here stopping it if the master requires it
2) Master start, slave not connected yet: I'm fine with getting
errors here... We *hope* a slave will be here soon, but...
3) network has seperated master/slave: TCP means it's been like this
for a long time already...
4) Slave hardware/os low-level hang/crash: TCP means it's been like
this for a while already before master's os tears down the connection
5) Slave has crashed (or rebooted) and slave OS has closed/rejected
our TCP connection

In all of these, I'ld love for my master not to be generating WAL and
letting clients think they are making progress. And I'm hoping that
for #3 & 4 above, PG will have keepalive type traffic that will
prevent me from queing WAL for normal TCP connection time values.

> One idea might be to wait both before and after commit.  If
> allow_standalone_primary is off, and a commit is attempted, we check
> whether there's a slave connected, and if not, wait for one to
> connect.  Then, we write and sync the commit WAL record.  Next, we
> wait for the WAL to be ack'd.  Of course, the standby might disappear
> between the first check and the second, but it would greatly reduce
> the possibility of the master being ahead of the standby after a
> crash, which might be useful for some people.

Ya, but that becomes much more expensive. Instead of it just being a
"write WAL, fsync WAL, send WAL, wait for slave", it becomes "write
WAL, fsync WAL, send WAL, wait for slave fsync, write WAL, fsync WAL,
send WAL, wait for slave fsync". And it's expense is all the time,
rather than just when the "no slave no go" situations arise.

And it doesn't reduce the transactions I need to verify by hand
either, because that waiting/error still only happens at the COMMIT
statement from the client.

--
Aidan Van Dyk                                             Create like a god,
aidan(at)highrise(dot)ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

On Fri, Jan 21, 2011 at 1:59 PM, Aidan Van Dyk <aidan(at)highrise(dot)ca> wrote:
> Yup.  And I'm OK with that.  In my case, it would be much better to
> have a few quick failures, which can complete automatically a few
> seconds later then to have a big buildup of transactions to re-verify
> by hand upon starting manual processing.

Why would you need to do that?

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

On Fri, 2011-01-21 at 13:32 -0500, Robert Haas wrote:

> One idea might be to wait both before and after commit. If
> allow_standalone_primary is off, and a commit is attempted, we check
> whether there's a slave connected, and if not, wait for one to
> connect. Then, we write and sync the commit WAL record. Next, we
> wait for the WAL to be ack'd. Of course, the standby might disappear
> between the first check and the second, but it would greatly reduce
> the possibility of the master being ahead of the standby after a
> crash, which might be useful for some people.

I like this idea.

I think it would be too invasive to make a check before we insert each
WAL record, as Aidan suggests. Even if we did that, you aren't protected
when a standby goes down because you'll still have written half a
transaction and still be waiting.

So I propose that

if (!allow_standalone_primary)
ConfirmSyncRepAvailable();

before PreCommit_Notify(). That puts transaction into a wait state that
lasts until a sync rep standby is available. Note that it is before the
actual commit, so if we decide we need to we can cancel those
transactions and have them properly abort.

I won't add that code yet, in case better ideas emerge.

There is no support for preventing connections at startup, so I will
remove that completely, now.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Sat, Jan 22, 2011 at 8:31 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Fri, 2011-01-21 at 13:32 -0500, Robert Haas wrote:
>
>> One idea might be to wait both before and after commit.  If
>> allow_standalone_primary is off, and a commit is attempted, we check
>> whether there's a slave connected, and if not, wait for one to
>> connect.  Then, we write and sync the commit WAL record.  Next, we
>> wait for the WAL to be ack'd.  Of course, the standby might disappear
>> between the first check and the second, but it would greatly reduce
>> the possibility of the master being ahead of the standby after a
>> crash, which might be useful for some people.
>
> I like this idea.
>
> I think it would be too invasive to make a check before we insert each
> WAL record, as Aidan suggests. Even if we did that, you aren't protected
> when a standby goes down because you'll still have written half a
> transaction and still be waiting.
>
> So I propose that
>
> if (!allow_standalone_primary)
>    ConfirmSyncRepAvailable();
>
> before PreCommit_Notify(). That puts transaction into a wait state that
> lasts until a sync rep standby is available. Note that it is before the
> actual commit, so if we decide we need to we can cancel those
> transactions and have them properly abort.
>
> I won't add that code yet, in case better ideas emerge.
>
> There is no support for preventing connections at startup, so I will
> remove that completely, now.

Time's running short - do you have an updated patch?

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

On Sun, Jan 30, 2011 at 11:44 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Time's running short - do you have an updated patch?

This patch hasn't been updated in more than three weeks. I assume
this should now be marked Returned with Feedback, and we'll revisit
synchronous replication for 9.2?

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

Robert Haas wrote:
> On Sun, Jan 30, 2011 at 11:44 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Time's running short - do you have an updated patch?
>
> This patch hasn't been updated in more than three weeks. I assume
> this should now be marked Returned with Feedback, and we'll revisit
> synchronous replication for 9.2?

Seems it is time for someone else to take the patch and complete it?
Who can do that?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Mon, 2011-02-07 at 09:55 -0500, Robert Haas wrote:
> On Sun, Jan 30, 2011 at 11:44 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Time's running short - do you have an updated patch?
>
> This patch hasn't been updated in more than three weeks. I assume
> this should now be marked Returned with Feedback, and we'll revisit
> synchronous replication for 9.2?

Hi Robert,

I have time to complete that in next two weeks, but you are right I
haven't had it in last few weeks.

Cheers

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Feb 7, 2011 at 11:33 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2011-02-07 at 09:55 -0500, Robert Haas wrote:
>> On Sun, Jan 30, 2011 at 11:44 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > Time's running short - do you have an updated patch?
>>
>> This patch hasn't been updated in more than three weeks.  I assume
>> this should now be marked Returned with Feedback, and we'll revisit
>> synchronous replication for 9.2?
>
> I have time to complete that in next two weeks, but you are right I
> haven't had it in last few weeks.

Well, the current CommitFest ends in one week, and we need to leave
time for someone (Heikki, most likely) to review, so there's really
only a couple of days left.

Bruce's suggestion of having someone else pick it up seems like it
might work. The obvious candidates are probably Heikki Linnakangas,
Tom Lane, Fujii Masao, and (if you squint a little) me. I am not
clear that any of those people have the necessary time available
immediately, however.

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

On Mon, Feb 7, 2011 at 12:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Feb 7, 2011 at 11:33 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Mon, 2011-02-07 at 09:55 -0500, Robert Haas wrote:
>>> On Sun, Jan 30, 2011 at 11:44 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> > Time's running short - do you have an updated patch?
>>>
>>> This patch hasn't been updated in more than three weeks.  I assume
>>> this should now be marked Returned with Feedback, and we'll revisit
>>> synchronous replication for 9.2?
>>
>> I have time to complete that in next two weeks, but you are right I
>> haven't had it in last few weeks.
>
> Well, the current CommitFest ends in one week, and we need to leave
> time for someone (Heikki, most likely) to review, so there's really
> only a couple of days left.
>
> Bruce's suggestion of having someone else pick it up seems like it
> might work.  The obvious candidates are probably Heikki Linnakangas,
> Tom Lane, Fujii Masao, and (if you squint a little) me.  I am not
> clear that any of those people have the necessary time available
> immediately, however.

I just spoke to my manager at EnterpriseDB and he cleared my schedule
for the next two days to work on this. So I'll go hack on this now.
I haven't read the patch yet so I don't know for sure how quite I'll
be able to get up to speed on it, so if someone who is more familiar
with this code wants to grab the baton away from me, feel free.
Otherwise, I'll see what I can do with it.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> ... Well, the current CommitFest ends in one week, ...

Really? I thought the idea for the last CF of a development cycle was
that it kept going till we'd dealt with everything. Arbitrarily
rejecting stuff we haven't dealt with doesn't seem fair.

regards, tom lane

On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> ... Well, the current CommitFest ends in one week, ...
>
> Really?  I thought the idea for the last CF of a development cycle was
> that it kept going till we'd dealt with everything.  Arbitrarily
> rejecting stuff we haven't dealt with doesn't seem fair.

Uh, we did that with 8.4 and it was a disaster. The CommitFest lasted
*five months*. We've been doing schedule-based CommitFests ever since
and it's worked much better. I agree it's unfair to reject things
without looking at them, and I'd like to avoid that if at all
possible, but punting things because they need more work than can be
done in the time available is another thing entirely. I do NOT want
to still be working on the items for this CommitFest in June - that's
about when I'd like to be releasing beta3.

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

On Mon, 2011-02-07 at 12:39 -0500, Robert Haas wrote:

> I just spoke to my manager at EnterpriseDB and he cleared my schedule
> for the next two days to work on this. So I'll go hack on this now.
> I haven't read the patch yet so I don't know for sure how quite I'll
> be able to get up to speed on it, so if someone who is more familiar
> with this code wants to grab the baton away from me, feel free.
> Otherwise, I'll see what I can do with it.

Presumably you have a reason for declaring war? I'm sorry for that, I
really am.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Feb 7, 2011 at 12:59 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2011-02-07 at 12:39 -0500, Robert Haas wrote:
>
>> I just spoke to my manager at EnterpriseDB and he cleared my schedule
>> for the next two days to work on this.  So I'll go hack on this now.
>> I haven't read the patch yet so I don't know for sure how quite I'll
>> be able to get up to speed on it, so if someone who is more familiar
>> with this code wants to grab the baton away from me, feel free.
>> Otherwise, I'll see what I can do with it.
>
> Presumably you have a reason for declaring war? I'm sorry for that, I
> really am.

What the hell are you talking about?

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

On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Here's the latest patch for sync rep.

Here is a rebased version of this patch which applies to head of the
master branch. I haven't tested it yet beyond making sure that it
compiles and passes the regression tests -- but this fixes the bitrot.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> done in the time available is another thing entirely. I do NOT want
> to still be working on the items for this CommitFest in June - that's
> about when I'd like to be releasing beta3.

Except that's not how we work here. You want to change that with
respect to the release management process and schedule (or lack
thereof). Tradition and current practice say you need to reach
consensus to be able to bypass compromising.

Good luck with that.

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

On Mon, 2011-02-07 at 17:59 +0000, Simon Riggs wrote:
> On Mon, 2011-02-07 at 12:39 -0500, Robert Haas wrote:
>
> > I just spoke to my manager at EnterpriseDB and he cleared my schedule
> > for the next two days to work on this. So I'll go hack on this now.
> > I haven't read the patch yet so I don't know for sure how quite I'll
> > be able to get up to speed on it, so if someone who is more familiar
> > with this code wants to grab the baton away from me, feel free.
> > Otherwise, I'll see what I can do with it.
>
> Presumably you have a reason for declaring war? I'm sorry for that, I
> really am.

Simon,

My impression was that Robert had received a release from current
responsibilities to help you with your patch, not that he was declaring
war or some such thing. I believe we all want SyncRep to be successful.

Sincerely,

JD

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579
Consulting, Training, Support, Custom Development, Engineering
http://twitter.com/cmdpromptinc | http://identi.ca/commandprompt

On Mon, Feb 7, 2011 at 2:09 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> done in the time available is another thing entirely.  I do NOT want
>> to still be working on the items for this CommitFest in June - that's
>> about when I'd like to be releasing beta3.
>
> Except that's not how we work here.  You want to change that with
> respect to the release management process and schedule (or lack
> thereof).  Tradition and current practice say you need to reach
> consensus to be able to bypass compromising.
>
> Good luck with that.

I'm not trying to bypass compromising, and I don't know what makes you
think otherwise. I am trying to ensure that the CommitFest wraps up
in a timely fashion, which is something we have done consistently for
every CommitFest in the 9.0 and 9.1 cycles to date, including the last
CommitFest of the 9.0 cycle. It is not somehow a deviation from past
community practice to boot patches that can't be finished up in the
time available during the CommitFest. That has been routine practice
for a long time.

I have worked very hard on this CommitFest, both to line up patch
reviewers and to review myself. I want to make sure that every patch
gets a good, thorough review before the CommitFest is over. I think
there is general consensus that this is important and that we will
lose contributors if we don't do it. However, I don't want to prolong
the CommitFest indefinitely in the face of patches that the authors
are not actively working on or can't finish in the time available, or
where there is no consensus that the proposed change is what we want.
I believe that this, too, is a generally accepted principle in our
community, not something I just made up.

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

>> I just spoke to my manager at EnterpriseDB and he cleared my schedule
>> for the next two days to work on this. So I'll go hack on this now.
>> I haven't read the patch yet so I don't know for sure how quite I'll
>> be able to get up to speed on it, so if someone who is more familiar
>> with this code wants to grab the baton away from me, feel free.
>> Otherwise, I'll see what I can do with it.
>
> Presumably you have a reason for declaring war? I'm sorry for that, I
> really am.

How is clearing out his whole schedule to help review & fix the patch
declaring war? You have an odd attitude towards assistance, Simon.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Mon, Feb 7, 2011 at 6:55 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> ... Well, the current CommitFest ends in one week, ...
>>
>> Really?  I thought the idea for the last CF of a development cycle was
>> that it kept going till we'd dealt with everything.  Arbitrarily
>> rejecting stuff we haven't dealt with doesn't seem fair.
>
> Uh, we did that with 8.4 and it was a disaster.  The CommitFest lasted
> *five months*. We've been doing schedule-based CommitFests ever since
> and it's worked much better.

Rejecting stuff because we haven't gotten round to dealing with it in
such a short period of time is a damn good way to limit the number of
contributions we get. I don't believe we've agreed at any point that
the last commitfest should be the same time length as the others (when
we originally came up with the commitfest idea, it certainly wasn't
expected), and deciding that without giving people advanced notice is
a really good way to piss them off and encourage them to go work on
other things.

If we're going to put a time limit on this - and I think we should -
we should publish a date ASAP, that gives everyone a fair chance to
finish their work - say, 4 weeks.

Then, if we want to make the last commitfest the same length as the
others next year, we can make that decision and document those plans.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 7 February 2011 18:20, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Here's the latest patch for sync rep.
>
> Here is a rebased version of this patch which applies to head of the
> master branch.  I haven't tested it yet beyond making sure that it
> compiles and passes the regression tests -- but this fixes the bitrot.

"When the primary is started with allow_standalone_primary enabled,
the primary will not allow connections until a standby connects that
also has synchronous_replication enabled. This is a convenience to
ensure that we don't allow connections before write transactions will
return successfully."

Shouldn't this be if allow_standalone_primary is disabled?

Also spotted some indentation, spelling and grammatical issues, which
I've applied to the patch if it's of interest (attached).

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

On Mon, Feb 7, 2011 at 2:56 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Mon, Feb 7, 2011 at 6:55 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> ... Well, the current CommitFest ends in one week, ...
>>>
>>> Really?  I thought the idea for the last CF of a development cycle was
>>> that it kept going till we'd dealt with everything.  Arbitrarily
>>> rejecting stuff we haven't dealt with doesn't seem fair.
>>
>> Uh, we did that with 8.4 and it was a disaster.  The CommitFest lasted
>> *five months*. We've been doing schedule-based CommitFests ever since
>> and it's worked much better.
>
> Rejecting stuff because we haven't gotten round to dealing with it in
> such a short period of time is a damn good way to limit the number of
> contributions we get. I don't believe we've agreed at any point that
> the last commitfest should be the same time length as the others

News to me.

http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Development_Plan

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

On Mon, Feb 7, 2011 at 8:59 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Feb 7, 2011 at 2:56 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> On Mon, Feb 7, 2011 at 6:55 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>> ... Well, the current CommitFest ends in one week, ...
>>>>
>>>> Really?  I thought the idea for the last CF of a development cycle was
>>>> that it kept going till we'd dealt with everything.  Arbitrarily
>>>> rejecting stuff we haven't dealt with doesn't seem fair.
>>>
>>> Uh, we did that with 8.4 and it was a disaster.  The CommitFest lasted
>>> *five months*. We've been doing schedule-based CommitFests ever since
>>> and it's worked much better.
>>
>> Rejecting stuff because we haven't gotten round to dealing with it in
>> such a short period of time is a damn good way to limit the number of
>> contributions we get. I don't believe we've agreed at any point that
>> the last commitfest should be the same time length as the others
>
> News to me.
>
> http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Development_Plan

Yes, and? It doesn't say beta 1 at the after a month of the last
commitfest, which is the milestone which marks the end of development.
It says alpha 4, and possibly more alphas. It's pretty clear that it
is expected that development and polishing will continue past the 20th
February.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>> ... Well, the current CommitFest ends in one week, ...
>>>>
>>>> Really? I thought the idea for the last CF of a development
>>>> cycle was that it kept going till we'd dealt with everything.
>>>> Arbitrarily rejecting stuff we haven't dealt with doesn't seem
>>>> fair.
>>>
>>> Uh, we did that with 8.4 and it was a disaster. The CommitFest
>>> lasted *five months*. We've been doing schedule-based
>>> CommitFests ever since and it's worked much better.
>>
>> Rejecting stuff because we haven't gotten round to dealing with
>> it in such a short period of time is a damn good way to limit the
>> number of contributions we get. I don't believe we've agreed at
>> any point that the last commitfest should be the same time length
>> as the others
>
> News to me.
>
> http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Development_Plan

I believe that with tighter management of the process, it should be
possible to reduce the average delay between someone writing a
feature and that feature appearing in a production release by about
two months without compromising quality. Getting hypothetical for a
moment, delaying release of 50 features for two months to allow
release of one feature ten months earlier is likely to frustrate a
lot more people than having the train leave the station on time and
putting that one feature into the next release.

My impression was that Robert is trying to find a way to help get
Simon's patch into this release without holding everything up for
it. In my book, that's not a declaration of war; it's community
spirit.

-Kevin

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm not trying to bypass compromising, and I don't know what makes you
> think otherwise. I am trying to ensure that the CommitFest wraps up

Well, I'm too tired to allow myself posting such comments, sorry to have
left the previous mail through. More than one commit fest saw its time
frame extended for 1 or 2 weeks already, I think, all I'm saying is that
this one will certainly not be an exception, and that's for the best.

Be sure I appreciate the efforts you're putting into the mix!

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

On 2/7/11 11:41 AM, Robert Haas wrote:
> However, I don't want to prolong
> the CommitFest indefinitely in the face of patches that the authors
> are not actively working on or can't finish in the time available, or
> where there is no consensus that the proposed change is what we want.
> I believe that this, too, is a generally accepted principle in our
> community, not something I just made up.

+1.

I, for one, would vote against extending beta if Sync Rep isn't ready
yet. There's plenty of other "big features" in 9.1, and Sync Rep will
benefit from having additional development time given the number of
major spec points we only cleared up a few weeks ago.

I think the majority of our users would prefer a 9.1 in May to one that
has Sync Rep and is delivered in September. If they had a choice.

Speaking of which, time to do some reviewing ...

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Mon, Feb 7, 2011 at 3:06 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>> Rejecting stuff because we haven't gotten round to dealing with it in
>>> such a short period of time is a damn good way to limit the number of
>>> contributions we get. I don't believe we've agreed at any point that
>>> the last commitfest should be the same time length as the others
>>
>> News to me.
>>
>> http://wiki.postgresql.org/wiki/PostgreSQL_9.1_Development_Plan
>
> Yes, and? It doesn't say beta 1 at the after a month of the last
> commitfest, which is the milestone which marks the end of development.
> It says alpha 4, and possibly more alphas. It's pretty clear that it
> is expected that development and polishing will continue past the 20th
> February.

You're moving the bar. It DOES say that the CommitFest will end on
February 15th. Now, if we want to have a discussion about changing
that, let's have that discussion (perhaps on a thread where the
subject has something to do with the topic), but we DID talk about
this, it WAS agreed, and it's been sitting there on the wiki for
something like 8 months. Obviously, there will continue to be
polishing after the CommitFest is over, but that's not the same thing
as saying we're going to lengthen the CommitFest itself.

I think we need to step back a few paces here and talk about what
we're trying to accomplish by making some change to the proposed and
agreed CommitFest schedule. If there's a concern that some patches
haven't been thoroughly reviewed at this point, then I think that's a
fair concern, and let's talk about which ones they are and see what we
can do about it. I don't believe that's the case, and it's certainly
not the case for sync rep, which was submitted in an unpolished state
by Simon's own admission, reviewed and discussed, then sat for three
weeks without an update. So perhaps the concern is that sync rep is a
make or break for this release. OK, then fine, let's talk about
whether it's worth slipping the release for that feature. I have no
problem with either of those conversations, and I'm happy to offer my
opinions and listen to the opinions of others, and we can make some
decision.

I think, though, that we need to be explicit about what we're doing,
and why we're doing it. I have been working hard on this CommitFest
for a long time (since approximately a month before it started) at the
cost of development projects I would have liked to have worked on,
because I knew we were going to be overwhelmed with patches. I have
helped as many people as I can with as many patches as I have been
able to. I think that finishing on time (or at least as close to on
time as we can manage) is important to our success as a development
community, just as having good features is. We don't have to agree on
what the best thing to do is, but I would certainly appreciate it if
everyone could at least credit me with acting in good faith.

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

On Mon, 2011-02-07 at 12:24 -0800, Josh Berkus wrote:

> +1.
>
> I, for one, would vote against extending beta if Sync Rep isn't ready
> yet. There's plenty of other "big features" in 9.1, and Sync Rep will
> benefit from having additional development time given the number of
> major spec points we only cleared up a few weeks ago.
>
> I think the majority of our users would prefer a 9.1 in May to one that
> has Sync Rep and is delivered in September. If they had a choice.
>

+1

JD

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579
Consulting, Training, Support, Custom Development, Engineering
http://twitter.com/cmdpromptinc | http://identi.ca/commandprompt

On Mon, Feb 7, 2011 at 9:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> You're moving the bar.  It DOES say that the CommitFest will end on
> February 15th.  Now, if we want to have a discussion about changing
> that, let's have that discussion (perhaps on a thread where the
> subject has something to do with the topic), but we DID talk about
> this, it WAS agreed, and it's been sitting there on the wiki for
> something like 8 months.  Obviously, there will continue to be
> polishing after the CommitFest is over, but that's not the same thing
> as saying we're going to lengthen the CommitFest itself.

I'm not moving the bar - I'm talking practically. Regardless of when
we consider the commitfest itself over, development and commit work of
new features has always continued until beta 1, and that has not
changed as far as I'm aware.

> I think, though, that we need to be explicit about what we're doing,
> and why we're doing it.  I have been working hard on this CommitFest
> for a long time (since approximately a month before it started) at the
> cost of development projects I would have liked to have worked on,
> because I knew we were going to be overwhelmed with patches.  I have
> helped as many people as I can with as many patches as I have been
> able to.  I think that finishing on time (or at least as close to on
> time as we can manage) is important to our success as a development
> community, just as having good features is.  We don't have to agree on
> what the best thing to do is, but I would certainly appreciate it if
> everyone could at least credit me with acting in good faith.

Oh, I have absolutely no doubt you're working in good faith, and
personally I thank you for the hard work you've put in. I just
disagree with your interpretation of the timetable.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Feb 7, 2011 at 3:14 PM, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'm not trying to bypass compromising, and I don't know what makes you
>> think otherwise.  I am trying to ensure that the CommitFest wraps up
>
> Well, I'm too tired to allow myself posting such comments, sorry to have
> left the previous mail through.

Thanks, I understand.

> More than one commit fest saw its time
> frame extended for 1 or 2 weeks already, I think, all I'm saying is that
> this one will certainly not be an exception, and that's for the best.

We've actually done really well. The last CommitFest in 9.0 wrapped
up on 2/17 (two days late), and the others were mostly right on time
as well. The CommitFests for 9.1 ended on: 8/15 (on time), 10/26 (9
days late, but there was no activity on the last two of those days, so
say 7 days late), and 12/21 (six days late). As far as I can tell,
the difference primarily has to do with who manages the CommitFests
and how aggressively they follow up on patches that are dropped. The
last CommitFest we have that really ran late was the final CommitFest
of the 8.4 cycle, and it was that event that led me to accept Josh
Berkus's invitation to be a CF manager for the first 9.0 CommitFest.
Because five month CommitFests with the tree frozen are awful and
sucky for everyone except the people who are getting extra time to
finish their patches, and they aren't really that great for those
people either.

As far as I am concerned, everything from now until we've released a
stable beta with no known issues is time that I can't spend doing
development. So I'd like to minimize that time - not by arbitrarily
throwing patches out the window - but by a combination of postponing
patches that are not done and working my ass off to finish as much as
possible.

> Be sure I appreciate the efforts you're putting into the mix!

Thanks.

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

On Mon, Feb 7, 2011 at 3:34 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> On Mon, Feb 7, 2011 at 9:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> You're moving the bar.  It DOES say that the CommitFest will end on
>> February 15th.  Now, if we want to have a discussion about changing
>> that, let's have that discussion (perhaps on a thread where the
>> subject has something to do with the topic), but we DID talk about
>> this, it WAS agreed, and it's been sitting there on the wiki for
>> something like 8 months.  Obviously, there will continue to be
>> polishing after the CommitFest is over, but that's not the same thing
>> as saying we're going to lengthen the CommitFest itself.
>
> I'm not moving the bar - I'm talking practically. Regardless of when
> we consider the commitfest itself over, development and commit work of
> new features has always continued until beta 1, and that has not
> changed as far as I'm aware.

I don't think that's really true. Go back and read the output of 'git
log REL9_0_BETA1'. It's bug fixes, rearrangements of things that were
committed but turned out to be controversial, documentation work,
release note editing, pgindent crap... sure, it wasn't a totally hard
freeze, but it was pretty solid slush. We did a good job not letting
things drag out, and FWIW I think that was a good decision. I don't
remember too many people being unhappy about their patches getting
punted, either. There were one or two, but generally we punted things
that needed major rework or just weren't getting updated in a timely
fashion, and that, combined with a lot of hard work on Tom's part
among others, worked fine.

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

dpage(at)pgadmin(dot)org (Dave Page) writes:
> On Mon, Feb 7, 2011 at 6:55 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> ... Well, the current CommitFest ends in one week, ...
>>>
>>> Really?  I thought the idea for the last CF of a development cycle was
>>> that it kept going till we'd dealt with everything.  Arbitrarily
>>> rejecting stuff we haven't dealt with doesn't seem fair.
>>
>> Uh, we did that with 8.4 and it was a disaster.  The CommitFest lasted
>> *five months*. We've been doing schedule-based CommitFests ever since
>> and it's worked much better.
>
> Rejecting stuff because we haven't gotten round to dealing with it in
> such a short period of time is a damn good way to limit the number of
> contributions we get. I don't believe we've agreed at any point that
> the last commitfest should be the same time length as the others (when
> we originally came up with the commitfest idea, it certainly wasn't
> expected), and deciding that without giving people advanced notice is
> a really good way to piss them off and encourage them to go work on
> other things.
>
> If we're going to put a time limit on this - and I think we should -
> we should publish a date ASAP, that gives everyone a fair chance to
> finish their work - say, 4 weeks.
>
> Then, if we want to make the last commitfest the same length as the
> others next year, we can make that decision and document those plans.

There *is* a problem that there doesn't seem to be enough time to
readily allow development of larger features without people getting
stuck fighting with the release periods. But that's not the problem
taking place here. It was documented, last May, that the final
CommitFest for 9.1 was to complete 2011-02-15, and there did seem to be
agreement on that.

It sure looks to me like there are going to be a bunch of items that,
based on the recognized policies, need to get deferred to 9.2, and the
prospects for Sync Rep getting into 9.1 don't look notably good to me.

Looking at things statistically, the 9.1 commitfests have had the
following numbers of items:

#1 - 2010-09 - 52, of which 26 were committed
#2 - 2010-11 - 43, of which 23 were committed
#3 - 2011-01 - 98, of which 35 have been committed, and 10 are
considered ready to commit.

It may appear unfair to not offer everyone a "fair chance to finish
their work," but it's not as if the date wasn't published Plenty Long
Ago. and well-publicized.

But deferring the end of the CommitFest would be Not Fair to those that
*did* get their proposed changes ready for the preceding Fests. We
cannot evade unfairness.

It's definitely readily arguable that fairness requires that:

- Items not committable by 2011-02-15 be deferred to the 2011-Next fest

There are around 25 items right now that are sitting with [Waiting
for Author] and [Returned with Feedback] statuses. They largely seem
like pretty fair game for "next fest."

- Large items that weren't included in the 2010-11 fest be considered
problematic to try to integrate into 9.1

There sure seem to be some large items in the 2011-01 fest, which I
thought wasn't supposed to be the case.

We shouldn't just impose policy for the sake of imposing policy, but I
do recall Really Long CommitFests being pretty disastrous. And there's
*SO* much outstanding in this particular fest that it's getting past
time for doing some substantial triage so that reviewer attentions may
be directed towards the items most likely to be acceptable for 9.1.

I hate to think that 9.1 won't include Simon's SR material, but that may
have to be.
--
http://www3.sympatico.ca/cbbrowne/slony.html
"It's a pretty rare beginner who isn't clueless. If beginners weren't
clueless, the infamous Unix learning cliff wouldn't be a problem."
-- david parsons

On Mon, Feb 7, 2011 at 9:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Feb 7, 2011 at 3:34 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>> On Mon, Feb 7, 2011 at 9:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> You're moving the bar.  It DOES say that the CommitFest will end on
>>> February 15th.  Now, if we want to have a discussion about changing
>>> that, let's have that discussion (perhaps on a thread where the
>>> subject has something to do with the topic), but we DID talk about
>>> this, it WAS agreed, and it's been sitting there on the wiki for
>>> something like 8 months.  Obviously, there will continue to be
>>> polishing after the CommitFest is over, but that's not the same thing
>>> as saying we're going to lengthen the CommitFest itself.
>>
>> I'm not moving the bar - I'm talking practically. Regardless of when
>> we consider the commitfest itself over, development and commit work of
>> new features has always continued until beta 1, and that has not
>> changed as far as I'm aware.
>
> I don't think that's really true.  Go back and read the output of 'git
> log REL9_0_BETA1'.  It's bug fixes, rearrangements of things that were
> committed but turned out to be controversial, documentation work,
> release note editing, pgindent crap...  sure, it wasn't a totally hard
> freeze, but it was pretty solid slush.  We did a good job not letting
> things drag out, and FWIW I think that was a good decision.  I don't
> remember too many people being unhappy about their patches getting
> punted, either.  There were one or two, but generally we punted things
> that needed major rework or just weren't getting updated in a timely
> fashion, and that, combined with a lot of hard work on Tom's part
> among others, worked fine.

I guess we disagree on what we consider to be "development" then. Just
looking back to April, I see various committers whacking things around
that look to me like the fine tuning and completion of earlier
patches.

Oh - and just so we're clear... I too want us to get the release out
promptly, I'm just concerned that we don't blindside developers.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, 2011-02-07 at 15:25 -0500, Robert Haas wrote:

> I would certainly appreciate it if
> everyone could at least credit me with acting in good faith.

I think you are, if that helps.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, 2011-02-07 at 11:50 -0800, Josh Berkus wrote:
> >> I just spoke to my manager at EnterpriseDB and he cleared my schedule
> >> for the next two days to work on this. So I'll go hack on this now.
> >> I haven't read the patch yet so I don't know for sure how quite I'll
> >> be able to get up to speed on it, so if someone who is more familiar
> >> with this code wants to grab the baton away from me, feel free.
> >> Otherwise, I'll see what I can do with it.
> >
> > Presumably you have a reason for declaring war? I'm sorry for that, I
> > really am.
>
> How is clearing out his whole schedule to help review & fix the patch
> declaring war? You have an odd attitude towards assistance, Simon.

It seems likely that Robert had not read my reply where I said I had
time to work on this project before posting. In that case, I withdraw my
comments and apologise.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Feb 7, 2011 at 5:16 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2011-02-07 at 11:50 -0800, Josh Berkus wrote:
>> >> I just spoke to my manager at EnterpriseDB and he cleared my schedule
>> >> for the next two days to work on this.  So I'll go hack on this now.
>> >> I haven't read the patch yet so I don't know for sure how quite I'll
>> >> be able to get up to speed on it, so if someone who is more familiar
>> >> with this code wants to grab the baton away from me, feel free.
>> >> Otherwise, I'll see what I can do with it.
>> >
>> > Presumably you have a reason for declaring war? I'm sorry for that, I
>> > really am.
>>
>> How is clearing out his whole schedule to help review & fix the patch
>> declaring war?  You have an odd attitude towards assistance, Simon.
>
> It seems likely that Robert had not read my reply where I said I had
> time to work on this project before posting. In that case, I withdraw my
> comments and apologise.

I did read it, but I still don't think saying I'm going to work on a
patch that's been stalled for weeks - or really months - constitutes
any sort of declaration of war, peace, or anything else. I have just
as much of a right to work on a given feature as you or anyone else
does. Typically, when I work on patches and help get them committed,
the response is "thanks". I'm not so sure what's different in this
case.

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

On Mon, Feb 7, 2011 at 1:20 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Here's the latest patch for sync rep.
>
> Here is a rebased version of this patch which applies to head of the
> master branch.  I haven't tested it yet beyond making sure that it
> compiles and passes the regression tests -- but this fixes the bitrot.

As I mentioned yesterday that I would do, I spent some time working on
this. I think that there are somewhere between three and six
independently useful features in this patch, plus a few random changes
to the documentation that I'm not sure whether want or not (e.g.
replacing master by primary in a few places, or the other way around).

One problem with the core synchronous replication technology is that
walreceiver cannot both receive WAL and write WAL at the same time.
It switches back and forth between reading WAL from the network socket
and flushing it to disk. The impact of that is somewhat mitigated in
the current patch because it only implements the "fsync" level of
replication, and chances are that the network read time is small
compared to the fsync time. But it would certainly suck for the
"receive" level we've talked about having in the past, because after
receiving each batch of WAL, the WAL receiver wouldn't be able to send
any more acknowledgments until the fsync completed, and that's bound
to be slow. I'm not really sure how bad it will be in "fsync" mode;
it may be tolerable, but as Simon noted in a comment, in the long run
it'd certainly be nicer to have the WAL writer process running during
recovery.

As a general comment on the quality of the code, I think that the
overall logic is probably sound, but there are an awful lot of
debugging leftovers and inconsistencies between various parts of the
patch. For example, when I initially tested it, *asynchronous*
replication kept breaking between the master and the standby, and I
couldn't figure out why. I finally realized that there was a ten
second pause that had been inserting into the WAL receiver loop as a
debugging tool which was allowing the standby to get far enough behind
that the master was able to recycle WAL segments the standby still
needed. Under ordinary circumstances, I would say that a patch like
this was not mature enough to submit for review, let alone commit.
For that reason, I am pretty doubtful about the chances of getting
this finished for 9.1 without some substantial prolongation of the
schedule.

That having been said, there is at least one part of this patch which
looks to be in pretty good shape and seems independently useful
regardless of what happens to the rest of it, and that is the code
that sends replies from the standby back to the primary. This allows
pg_stat_replication to display the write/flush/apply log positions on
the standby next to the sent position on the primary, which as far as
I am concerned is pure gold. Simon had this set up to happen only
when synchronous replication or XID feedback in use, but I think
people are going to want it even with plain old asynchronous
replication, because it provides a FAR easier way to monitor standby
lag than anything we have today. I've extracted this portion of the
patch, cleaned it up a bit, written docs, and attached it here.

I wasn't too sure how to control the timing of the replies. It's
worth noting that you have to send them pretty frequently for the
distinction between xlog written and xlog flushed to have any value.
What I've done here is made it so that every time we read all
available data on the socket, we send a reply. After flushing, we
send another reply. And then just for the heck of it we send a reply
at least every 10 seconds (configurable), which causes the
last-known-apply position to eventually get updated on the master.
This means the apply position can lag reality by a bit. Simon's
version adds a latch, so that the startup process can poke the WAL
receiver to send a reply when the apply position moves. But this is
substantially more complex and I'm not sure it's worth it. If we were
implementing the "apply" level of synchronized replication, we'd
clearly need that for performance not to stink. But since the patch
is only implementing "fsync" anyway, it doesn't seem necessary for
now.

The only real complaint I can imagine about offering this
functionality all the time is that it uses extra bandwidth. I'm
inclined to think that the ability to shut it off completely is
sufficient answer to that complaint.

<dons asbestos underwear>

Comments?

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

On Tue, Feb 8, 2011 at 19:53, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Feb 7, 2011 at 1:20 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Here's the latest patch for sync rep.
>>
>> Here is a rebased version of this patch which applies to head of the
>> master branch.  I haven't tested it yet beyond making sure that it
>> compiles and passes the regression tests -- but this fixes the bitrot.
>
> As I mentioned yesterday that I would do, I spent some time working on
> this.  I think that there are somewhere between three and six
> independently useful features in this patch, plus a few random changes
> to the documentation that I'm not sure whether want or not (e.g.
> replacing master by primary in a few places, or the other way around).
>
> One problem with the core synchronous replication technology is that
> walreceiver cannot both receive WAL and write WAL at the same time.
> It switches back and forth between reading WAL from the network socket
> and flushing it to disk.  The impact of that is somewhat mitigated in
> the current patch because it only implements the "fsync" level of
> replication, and chances are that the network read time is small
> compared to the fsync time.  But it would certainly suck for the
> "receive" level we've talked about having in the past, because after
> receiving each batch of WAL, the WAL receiver wouldn't be able to send
> any more acknowledgments until the fsync completed, and that's bound
> to be slow.  I'm not really sure how bad it will be in "fsync" mode;
> it may be tolerable, but as Simon noted in a comment, in the long run
> it'd certainly be nicer to have the WAL writer process running during
> recovery.
>
> As a general comment on the quality of the code, I think that the
> overall logic is probably sound, but there are an awful lot of
> debugging leftovers and inconsistencies between various parts of the
> patch.  For example, when I initially tested it, *asynchronous*
> replication kept breaking between the master and the standby, and I
> couldn't figure out why.  I finally realized that there was a ten
> second pause that had been inserting into the WAL receiver loop as a
> debugging tool which was allowing the standby to get far enough behind
> that the master was able to recycle WAL segments the standby still
> needed.  Under ordinary circumstances, I would say that a patch like
> this was not mature enough to submit for review, let alone commit.
> For that reason, I am pretty doubtful about the chances of getting
> this finished for 9.1 without some substantial prolongation of the
> schedule.
>
> That having been said, there is at least one part of this patch which
> looks to be in pretty good shape and seems independently useful
> regardless of what happens to the rest of it, and that is the code
> that sends replies from the standby back to the primary.  This allows
> pg_stat_replication to display the write/flush/apply log positions on
> the standby next to the sent position on the primary, which as far as
> I am concerned is pure gold.  Simon had this set up to happen only
> when synchronous replication or XID feedback in use, but I think
> people are going to want it even with plain old asynchronous
> replication, because it provides a FAR easier way to monitor standby
> lag than anything we have today.  I've extracted this portion of the
> patch, cleaned it up a bit, written docs, and attached it here.

+1. I haven't actually looked at the patch, but having this ability
would be *great*.

I also agree with the general idea of trying to break it into smaller
parts - even if they only provide small parts each on it's own. That
also makes it easier to get an overview of exactly how much is left,
to see where to focus.

> The only real complaint I can imagine about offering this
> functionality all the time is that it uses extra bandwidth.  I'm
> inclined to think that the ability to shut it off completely is
> sufficient answer to that complaint.

Yes, agreed.

I would usually not worry about the bandwidth, really, I'd be more
worried about potentially increasing latency somewhere.

> <dons asbestos underwear>

The ones with little rocketships on them?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Feb 8, 2011 at 2:34 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> I would usually not worry about the bandwidth, really, I'd be more
> worried about potentially increasing latency somewhere.

The time to read and write the socket doesn't seem like it should be
significant, unless the network buffers fill up.... or I'm missing
something.

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

On Tue, Feb 8, 2011 at 2:34 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> I also agree with the general idea of trying to break it into smaller
> parts - even if they only provide small parts each on it's own. That
> also makes it easier to get an overview of exactly how much is left,
> to see where to focus.

And on that note, here's the rest of the patch back, rebased over what
I posted ~90 minutes ago.

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

On Tue, 2011-02-08 at 13:53 -0500, Robert Haas wrote:

> That having been said, there is at least one part of this patch which
> looks to be in pretty good shape and seems independently useful
> regardless of what happens to the rest of it, and that is the code
> that sends replies from the standby back to the primary. This allows
> pg_stat_replication to display the write/flush/apply log positions on
> the standby next to the sent position on the primary, which as far as
> I am concerned is pure gold. Simon had this set up to happen only
> when synchronous replication or XID feedback in use, but I think
> people are going to want it even with plain old asynchronous
> replication, because it provides a FAR easier way to monitor standby
> lag than anything we have today. I've extracted this portion of the
> patch, cleaned it up a bit, written docs, and attached it here.

Score! +1

JD

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579
Consulting, Training, Support, Custom Development, Engineering
http://twitter.com/cmdpromptinc | http://identi.ca/commandprompt

On Wed, Feb 9, 2011 at 3:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> That having been said, there is at least one part of this patch which
> looks to be in pretty good shape and seems independently useful
> regardless of what happens to the rest of it, and that is the code
> that sends replies from the standby back to the primary.  This allows
> pg_stat_replication to display the write/flush/apply log positions on
> the standby next to the sent position on the primary, which as far as
> I am concerned is pure gold.  Simon had this set up to happen only
> when synchronous replication or XID feedback in use, but I think
> people are going to want it even with plain old asynchronous
> replication, because it provides a FAR easier way to monitor standby
> lag than anything we have today.  I've extracted this portion of the
> patch, cleaned it up a bit, written docs, and attached it here.

What about also sending back the timestamp of the last applied
transaction? That's more user-friendly than the apply location
when we calculate the lag of replication, I think.

Regards,

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

On mån, 2011-02-07 at 12:55 -0500, Robert Haas wrote:
> On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> ... Well, the current CommitFest ends in one week, ...
> >
> > Really? I thought the idea for the last CF of a development cycle was
> > that it kept going till we'd dealt with everything. Arbitrarily
> > rejecting stuff we haven't dealt with doesn't seem fair.
>
> Uh, we did that with 8.4 and it was a disaster. The CommitFest lasted
> *five months*. We've been doing schedule-based CommitFests ever since
> and it's worked much better.

The previous three commit fests contained about 50 patches each and
lasted one month each. The current commit fest contains about 100
patches, so it shouldn't be surprising that it will take about 2 months
to get through it.

Moreover, under the current process, it is apparent that reviewing is
the bottleneck. More code gets written than gets reviewed. By
insisting on the current schedule, we would just push the growing review
backlog ahead of ourselves. The solution (at least short-term, while
maintaining the process) has to be to increase the resources (in
practice: time) dedicated to reviewing relative to coding.

On 02/09/2011 07:53 AM, Peter Eisentraut wrote:
> On mån, 2011-02-07 at 12:55 -0500, Robert Haas wrote:
>> On Mon, Feb 7, 2011 at 12:43 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>>>> ... Well, the current CommitFest ends in one week, ...
>>> Really? I thought the idea for the last CF of a development cycle was
>>> that it kept going till we'd dealt with everything. Arbitrarily
>>> rejecting stuff we haven't dealt with doesn't seem fair.
>> Uh, we did that with 8.4 and it was a disaster. The CommitFest lasted
>> *five months*. We've been doing schedule-based CommitFests ever since
>> and it's worked much better.
> The previous three commit fests contained about 50 patches each and
> lasted one month each. The current commit fest contains about 100
> patches, so it shouldn't be surprising that it will take about 2 months
> to get through it.
>
> Moreover, under the current process, it is apparent that reviewing is
> the bottleneck. More code gets written than gets reviewed. By
> insisting on the current schedule, we would just push the growing review
> backlog ahead of ourselves. The solution (at least short-term, while
> maintaining the process) has to be to increase the resources (in
> practice: time) dedicated to reviewing relative to coding.

Personally I think it's not unreasonable to extend the final commitfest
of the release some. It doesn't need to be a huge amount longer,
certainly not five months, but a couple of weeks to a month might be fair.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 02/09/2011 07:53 AM, Peter Eisentraut wrote:
>> The previous three commit fests contained about 50 patches each and
>> lasted one month each. The current commit fest contains about 100
>> patches, so it shouldn't be surprising that it will take about 2 months
>> to get through it.

> Personally I think it's not unreasonable to extend the final commitfest
> of the release some. It doesn't need to be a huge amount longer,
> certainly not five months, but a couple of weeks to a month might be fair.

Yeah. IIRC, in our first cycle using the CF process, we expected the
last CF to take longer than others. I am not sure where the idea came
from that we'd be able to finish this one in a month.

I do accept the fact that we mustn't let it drag on indefinitely.
But two months instead of one isn't indefinite, and it seems more
realistic given the amount of work to be done.

regards, tom lane

On Wed, Feb 9, 2011 at 9:42 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 02/09/2011 07:53 AM, Peter Eisentraut wrote:
>>> The previous three commit fests contained about 50 patches each and
>>> lasted one month each.  The current commit fest contains about 100
>>> patches, so it shouldn't be surprising that it will take about 2 months
>>> to get through it.
>
>> Personally I think it's not unreasonable to extend the final commitfest
>> of the release some. It doesn't need to be a huge amount longer,
>> certainly not five months, but a couple of weeks to a month might be fair.
>
> Yeah.  IIRC, in our first cycle using the CF process, we expected the
> last CF to take longer than others.  I am not sure where the idea came
> from that we'd be able to finish this one in a month.

It came from the fact that we did it last time.

> I do accept the fact that we mustn't let it drag on indefinitely.
> But two months instead of one isn't indefinite, and it seems more
> realistic given the amount of work to be done.

The work will expand to fill the time available.

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

On Wed, Feb 9, 2011 at 7:53 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Moreover, under the current process, it is apparent that reviewing is
> the bottleneck.  More code gets written than gets reviewed.  By
> insisting on the current schedule, we would just push the growing review
> backlog ahead of ourselves.  The solution (at least short-term, while
> maintaining the process) has to be to increase the resources (in
> practice: time) dedicated to reviewing relative to coding.

Yep. People who submit patches must also review patches if they want
their own stuff reviewed.

It sounds to me like what's being proposed is that I should spend
another month working on other people's patches, while they work on
their own patches. I can't get excited about that. The situation
with reviewing has gotten totally out of hand. I review and commit
more patches as part of each CommitFest than anyone except Tom, and I
think there have been some CommitFests where I did more patches than
he did (though he still wins by a mile if you factor in patch
complexity). But on the flip side, I can't always get a reviewer for
my own patches, or sometimes I get a perfunctory review that someone
spent ten minutes on. Huh?

So I heartily approve of the suggestion that we need to devote more
energy to reviewing, if it means "more reviewing by the people who are
not me". And allow me to suggest that that energy get put in NOW,
rather than a month from now. Most of the patches that still need
review are not that complicated. At least half of them could probably
be meaningfully reviewed in an hour or two. Then the author could
post an update tomorrow. Then the reviewer could spend another 30
minutes and mark them ready for committer. Next!

There are certainly some patches in this CommitFest that need more
attention than that, and that probably need the attention of a senior
community member. Jeff's range types patch and Alvaro's key lock
patch are two of those. And I would be willing to do that, except
that I'm already listed as a reviewer for FOURTEEN PATCHES this
CommitFest, plus I committed some others that someone else reviewed
and am also functioning as CommitFest manager. The problem isn't so
much the amount of calendar time that's required to get through 100
patches as the many people either submit half-baked code and assume
that they or someone else will fix it later, or else they submit code
but don't do an amount of review work equal to the amount of review
work they generate.

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

On Feb 9, 2011, at 9:20 AM, Robert Haas wrote:

> There are certainly some patches in this CommitFest that need more
> attention than that, and that probably need the attention of a senior
> community member. Jeff's range types patch and Alvaro's key lock
> patch are two of those. And I would be willing to do that, except
> that I'm already listed as a reviewer for FOURTEEN PATCHES this
> CommitFest, plus I committed some others that someone else reviewed
> and am also functioning as CommitFest manager. The problem isn't so
> much the amount of calendar time that's required to get through 100
> patches as the many people either submit half-baked code and assume
> that they or someone else will fix it later, or else they submit code
> but don't do an amount of review work equal to the amount of review
> work they generate.

Frankly, I think you should surrender some of those 14 and cajole some other folks to take on more.

Best,

David

On Wed, Feb 9, 2011 at 1:09 PM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> Frankly, I think you should surrender some of those 14 and cajole some other folks to take on more.

Happily... only trouble is, I suck at cajoling. Even my begging is
distinctly sub-par.

Pleeeeeeeease?

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

On Feb 9, 2011, at 10:29 AM, Robert Haas wrote:

>> Frankly, I think you should surrender some of those 14 and cajole some other folks to take on more.
>
> Happily... only trouble is, I suck at cajoling. Even my begging is
> distinctly sub-par.
>
> Pleeeeeeeease?

Try this:

“Listen up, bitches! I'm tired of Tom and me having to do all the work. All of you who submitted patches need to review some other patches! If you haven't submitted a review for someone else's patch by commitfest end, your patches will be marked "returned."”

Then maybe cuff Jeff or Alvaro or someone, to show you mean business.

HTH,

David

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Feb 9, 2011 at 1:09 PM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> > Frankly, I think you should surrender some of those 14 and cajole some other folks to take on more.
>
> Happily... only trouble is, I suck at cajoling. Even my begging is
> distinctly sub-par.
>
> Pleeeeeeeease?

Erm, I've been through the commitfest app a couple of different times,
but have ignored things which are marked 'Needs Reivew' when there's a
reviewer listed...

If there are patches where you're marked as the reviewer but you don't
have time to review them or want help, take your name off as a reviewer
for them and/or speak up and explicitly ask for help. I'm not going to
start reviewing something if I think someone else is already working on
it..

Thanks,

Stephen

On Wed, Feb 9, 2011 at 1:32 PM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> On Feb 9, 2011, at 10:29 AM, Robert Haas wrote:
>>> Frankly, I think you should surrender some of those 14 and cajole some other folks to take on more.
>>
>> Happily...  only trouble is, I suck at cajoling.  Even my begging is
>> distinctly sub-par.
>>
>> Pleeeeeeeease?
>
> Try this:
>
> “Listen up, bitches! I'm tired of Tom and me having to do all the work. All of you who submitted patches need to review some other patches! If you haven't submitted a review for someone else's patch by commitfest end, your patches will be marked "returned."”
>
> Then maybe cuff Jeff or Alvaro or someone, to show you mean business.

That tends not to get a lot of community support, and it isn't my
intention anyway. We actually do not need to impose a draconian rule;
we just need everyone to put in a little extra effort to get us over
the hump.

But speaking of that, I just so happen to notice you haven't signed up
to review any patches this CF. How about grabbing one or two?

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

On Feb 9, 2011, at 10:56 AM, Robert Haas wrote:

>> “Listen up, bitches! I'm tired of Tom and me having to do all the work. All of you who submitted patches need to review some other patches! If you haven't submitted a review for someone else's patch by commitfest end, your patches will be marked "returned."”
>>
>> Then maybe cuff Jeff or Alvaro or someone, to show you mean business.
>
> That tends not to get a lot of community support, and it isn't my
> intention anyway. We actually do not need to impose a draconian rule;
> we just need everyone to put in a little extra effort to get us over
> the hump.

Agreed. Let me remove my tongue from my cheek.

> But speaking of that, I just so happen to notice you haven't signed up
> to review any patches this CF. How about grabbing one or tw

ha ha! Alas, I'm completely overcommitted at this point. Been having a hard time making time for PGXN. I've been tracking the extension stuff closely, though, as you can imagine.

Looking at the patches without reviewers anyway, frankly none look like the sorts of things I have the expertise to test in any but the most superficial way. Are there more that should have the reviewer removed? If there were one I could give a couple of hours to and speak with some knowledge, I could fix up some time next week.

Best,

David

On Wed, Feb 9, 2011 at 2:01 PM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> ha ha! Alas, I'm completely overcommitted at this point. Been having a hard time making time for PGXN. I've been tracking the extension stuff closely, though, as you can imagine.

It's a common problem, and of course none of us are in a position to
dictate how other people spend their time. But the issue on the table
is whether we want PostgreSQL 9.1 to be released in 2011. If yes,
then without making any statements about what any particular person
has to or must do, we collectively need to step it up a notch or two.

> Looking at the patches without reviewers anyway, frankly none look like the sorts of things I have the expertise to test in any but the most superficial way. Are there more that should have the reviewer removed? If there were one I could give a couple of hours to and speak with some knowledge, I could fix up some time next week.

I just sent a note on some that seem like they could use more looking
at, but there may be other ones too. Now is not the time to hold back
because you think someone else might be working on it. Most of the
time, the fact that a patch has a reviewer means that they either
intended to or actually did review it at some point in time, but not
that they are necessarily working on it right this minute, and
certainly not that other input isn't welcome. This is especially true
towards the end of the CommitFest or when the thread hasn't had
anything new posted to it for several days.

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

On Wed, Feb 9, 2011 at 5:25 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Feb 8, 2011 at 2:34 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> I also agree with the general idea of trying to break it into smaller
>> parts - even if they only provide small parts each on it's own. That
>> also makes it easier to get an overview of exactly how much is left,
>> to see where to focus.
>
> And on that note, here's the rest of the patch back, rebased over what
> I posted ~90 minutes ago.

Though I haven't read the patch enough yet, I have one review comment.

While walsender uses the non-blocking I/O function (i.e.,
pq_getbyte_if_available)
for the receive, it uses the blocking one (i.e., pq_flush, etc) for the send.
So, sync_rep_timeout_server would not work well when the walsender
gets blocked in sending WAL. This is one the problems which I struggled
with when I created the SyncRep patch before. I think that we need to
introduce the non-blocking send function for the replication timeout.

Regards,

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

On 08.02.2011 20:53, Robert Haas wrote:
> That having been said, there is at least one part of this patch which
> looks to be in pretty good shape and seems independently useful
> regardless of what happens to the rest of it, and that is the code
> that sends replies from the standby back to the primary. This allows
> pg_stat_replication to display the write/flush/apply log positions on
> the standby next to the sent position on the primary, which as far as
> I am concerned is pure gold. Simon had this set up to happen only
> when synchronous replication or XID feedback in use, but I think
> people are going to want it even with plain old asynchronous
> replication, because it provides a FAR easier way to monitor standby
> lag than anything we have today. I've extracted this portion of the
> patch, cleaned it up a bit, written docs, and attached it here.

Thanks!

> I wasn't too sure how to control the timing of the replies. It's
> worth noting that you have to send them pretty frequently for the
> distinction between xlog written and xlog flushed to have any value.
> What I've done here is made it so that every time we read all
> available data on the socket, we send a reply. After flushing, we
> send another reply. And then just for the heck of it we send a reply
> at least every 10 seconds (configurable), which causes the
> last-known-apply position to eventually get updated on the master.
> This means the apply position can lag reality by a bit.

Seems reasonable. As the patch stands, however, the standby doesn't send
any status updates if its busy receiving, writing, and flushing the
incoming WAL. That would happen if you have a fast network, and slow
disk, and the standby is catching up, e.g after restoring a base backup.

I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it
also sends a status update every time the WAL is flushed. If the
walreceiver is busy receiving and flushing, that would happen once per
WAL segment, which seems sensible.

The comment above StandbyReplyMessage said that its message type is 'r'.
However, no message type was actually sent for the replies. A message
type byte seems like a good idea, for the sake of extensibility, so I
made the code match that comment. I also added documentation of this new
message type in the manual section about the streaming replication protocol.

I committed the patch with those changes, and some minor comment tweaks
and other kibitzing.

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

On Tue, Feb 8, 2011 at 3:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Feb 8, 2011 at 2:34 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> I also agree with the general idea of trying to break it into smaller
>> parts - even if they only provide small parts each on it's own. That
>> also makes it easier to get an overview of exactly how much is left,
>> to see where to focus.
>
> And on that note, here's the rest of the patch back, rebased over what
> I posted ~90 minutes ago.

Another rebase.

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

On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> I committed the patch with those changes, and some minor comment tweaks and
> other kibitzing.

+ * 'd' means a standby reply wrapped in a COPY BOTH packet.
+ */

Typo: s/COPY BOTH/CopyData

+ msgtype = pq_getmsgbyte(&input_message);
+ if (msgtype != 'r')
+ ereport(COMMERROR,
+ (errcode(ERRCODE_PROTOCOL_VIOLATION),
+ errmsg("unexpected message type %c", msgtype)));

I think that proc_exit(0) needs to be called in error case.

+ static StringInfoData input_message;
+ StandbyReplyMessage reply;
+ char msgtype;
+
+ initStringInfo(&input_message);

Doesn't the repeat of initStringInfo() cause the memory leak?

@@ -518,6 +584,7 @@ WalSndLoop(void)
{
if (!XLogSend(output_message, &caughtup))
break;
+ ProcessRepliesIfAny();

Why is ProcessRepliesIfAny() required there?

We added new columns "write_location", "flush_location" and
"apply_location". So, for the sake of consistency, the column
name "sent_location" should be changed to "send_location"?

Regards,.

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

On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
> sends a status update every time the WAL is flushed. If the walreceiver is
> busy receiving and flushing, that would happen once per WAL segment, which
> seems sensible.

This change can make the callback function "WalRcvDie()" call ereport(ERROR)
via XLogWalRcvFlush(). This looks unsafe.

Regards,

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

On Mon, Feb 14, 2011 at 2:08 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> I committed the patch with those changes, and some minor comment tweaks and
>> other kibitzing.

I have another comment:

The description of wal_receiver_status_interval is in "18.5.4.
Streaming Replication".
But I think that it should be moved to "18.5.5. Standby Servers" since
it's a parameter
to control the behavior of the standby server rather than that of the master.

Regards,

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

On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> Here's the latest patch for sync rep.
>

I was looking at this code and found something in SyncRepWaitOnQueue
we declare a timeout variable that is a long and another that is a
boolean (this last one in the else part of the "if
(!IsOnSyncRepQueue())"), and then use the boolean one as if it were
the long one

+ else
+ {
+ bool release = false;
+ bool timeout = false;
+
+ SpinLockAcquire(&queue->qlock);
+
+ /*
+ * Check the LSN on our queue and if its moved far enough then
+ * remove us from the queue. First time through this is
+ * unlikely to be far enough, yet is possible. Next time we are
+ * woken we should be more lucky.
+ */
+ if (XLByteLE(XactCommitLSN, queue->lsn))
+ release = true;
+ else if (timeout > 0 &&
+ TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
+ now,
+ timeout))
+ {
+ release = true;
+ timeout = true;
+ }

the other two things are on postgresql.conf.sample:
- we have two replication_timeout_client, obviously one of them should
be replication_timeout_server
- synchronous_replication_feedback is off by default, but docs says otherwise

i also have been testing this, until now the only issue i have found
is that if i set allow_standalone_primary to off and there isn't a
standby connected i need to stop the server with -m immediate which is
at least surprising

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

On Tue, 2011-02-15 at 01:45 -0500, Jaime Casanova wrote:
> On Sat, Jan 15, 2011 at 4:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >
> > Here's the latest patch for sync rep.
> >
>
> I was looking at this code and found something in SyncRepWaitOnQueue
> we declare a timeout variable that is a long and another that is a
> boolean (this last one in the else part of the "if
> (!IsOnSyncRepQueue())"), and then use the boolean one as if it were
> the long one

OK, thanks.

> + else
> + {
> + bool release = false;
> + bool timeout = false;
> +
> + SpinLockAcquire(&queue->qlock);
> +
> + /*
> + * Check the LSN on our queue and if its moved far enough then
> + * remove us from the queue. First time through this is
> + * unlikely to be far enough, yet is possible. Next time we are
> + * woken we should be more lucky.
> + */
> + if (XLByteLE(XactCommitLSN, queue->lsn))
> + release = true;
> + else if (timeout > 0 &&
> + TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
> + now,
> + timeout))
> + {
> + release = true;
> + timeout = true;
> + }
>
> the other two things are on postgresql.conf.sample:
> - we have two replication_timeout_client, obviously one of them should
> be replication_timeout_server
> - synchronous_replication_feedback is off by default, but docs says otherwise
>
> i also have been testing this, until now the only issue i have found
> is that if i set allow_standalone_primary to off and there isn't a
> standby connected i need to stop the server with -m immediate which is
> at least surprising

I think that code is being ripped out, so will check again later.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Feb 14, 2011 at 12:08 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> I committed the patch with those changes, and some minor comment tweaks and
>> other kibitzing.
>
> +            * 'd' means a standby reply wrapped in a COPY BOTH packet.
> +            */
>
> Typo: s/COPY BOTH/CopyData

Fixed.

> +   msgtype = pq_getmsgbyte(&input_message);
> +   if (msgtype != 'r')
> +       ereport(COMMERROR,
> +               (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +                errmsg("unexpected message type %c", msgtype)));
>
> I think that proc_exit(0) needs to be called in error case.

Fixed.

> +   static StringInfoData input_message;
> +   StandbyReplyMessage reply;
> +   char msgtype;
> +
> +   initStringInfo(&input_message);
>
> Doesn't the repeat of initStringInfo() cause the memory leak?

Yeah. Fixed, I hope.

> @@ -518,6 +584,7 @@ WalSndLoop(void)
>        {
>            if (!XLogSend(output_message, &caughtup))
>                break;
> +           ProcessRepliesIfAny();
>
> Why is ProcessRepliesIfAny() required there?

I'm not sure if that's 100% necessary, but it seems harmless enough.

> We added new columns "write_location", "flush_location" and
> "apply_location". So, for the sake of consistency, the column
> name "sent_location" should be changed to "send_location"?

I was thinking about stream_location or streaming_location, per
discussion on the other thread.

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

On Tue, Feb 15, 2011 at 1:11 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Mon, Feb 14, 2011 at 2:08 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> I committed the patch with those changes, and some minor comment tweaks and
>>> other kibitzing.
>
> I have another comment:
>
> The description of wal_receiver_status_interval is in "18.5.4.
> Streaming Replication".
> But I think that it should be moved to "18.5.5. Standby Servers" since
> it's a parameter
> to control the behavior of the standby server rather than that of the master.

Fixed.

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

On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>> sends a status update every time the WAL is flushed. If the walreceiver is
>> busy receiving and flushing, that would happen once per WAL segment, which
>> seems sensible.
>
> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
> via XLogWalRcvFlush(). This looks unsafe.

Good catch. Is the cleanest solution to pass a boolean parameter to
XLogWalRcvFlush() indicating whether we're in the midst of dying?

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

On Wed, Feb 16, 2011 at 2:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>>> sends a status update every time the WAL is flushed. If the walreceiver is
>>> busy receiving and flushing, that would happen once per WAL segment, which
>>> seems sensible.
>>
>> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
>> via XLogWalRcvFlush(). This looks unsafe.
>
> Good catch.  Is the cleanest solution to pass a boolean parameter to
> XLogWalRcvFlush() indicating whether we're in the midst of dying?

Agreed if the comment about why such a boolean parameter is
required is added.

Regards,

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

On Tue, Feb 15, 2011 at 10:13 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Feb 16, 2011 at 2:08 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>>>> sends a status update every time the WAL is flushed. If the walreceiver is
>>>> busy receiving and flushing, that would happen once per WAL segment, which
>>>> seems sensible.
>>>
>>> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
>>> via XLogWalRcvFlush(). This looks unsafe.
>>
>> Good catch.  Is the cleanest solution to pass a boolean parameter to
>> XLogWalRcvFlush() indicating whether we're in the midst of dying?
>
> Agreed if the comment about why such a boolean parameter is
> required is added.

OK, done.

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

On Tue, 2011-02-15 at 12:08 -0500, Robert Haas wrote:
> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
> > <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> >> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
> >> sends a status update every time the WAL is flushed. If the walreceiver is
> >> busy receiving and flushing, that would happen once per WAL segment, which
> >> seems sensible.
> >
> > This change can make the callback function "WalRcvDie()" call ereport(ERROR)
> > via XLogWalRcvFlush(). This looks unsafe.
>
> Good catch. Is the cleanest solution to pass a boolean parameter to
> XLogWalRcvFlush() indicating whether we're in the midst of dying?

Surely if you do this then sync rep will fail to respond correctly if
WalReceiver dies.

Why is it OK to write to disk, but not OK to reply?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On 16.02.2011 17:36, Simon Riggs wrote:
> On Tue, 2011-02-15 at 12:08 -0500, Robert Haas wrote:
>> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao<masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>>>> sends a status update every time the WAL is flushed. If the walreceiver is
>>>> busy receiving and flushing, that would happen once per WAL segment, which
>>>> seems sensible.
>>>
>>> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
>>> via XLogWalRcvFlush(). This looks unsafe.
>>
>> Good catch. Is the cleanest solution to pass a boolean parameter to
>> XLogWalRcvFlush() indicating whether we're in the midst of dying?
>
> Surely if you do this then sync rep will fail to respond correctly if
> WalReceiver dies.
>
> Why is it OK to write to disk, but not OK to reply?

Because the connection might be dead. A broken connection is a likely
cause of walreceiver death.

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

On Wed, 2011-02-16 at 17:40 +0200, Heikki Linnakangas wrote:
> On 16.02.2011 17:36, Simon Riggs wrote:
> > On Tue, 2011-02-15 at 12:08 -0500, Robert Haas wrote:
> >> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao<masao(dot)fujii(at)gmail(dot)com> wrote:
> >>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
> >>> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> >>>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
> >>>> sends a status update every time the WAL is flushed. If the walreceiver is
> >>>> busy receiving and flushing, that would happen once per WAL segment, which
> >>>> seems sensible.
> >>>
> >>> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
> >>> via XLogWalRcvFlush(). This looks unsafe.
> >>
> >> Good catch. Is the cleanest solution to pass a boolean parameter to
> >> XLogWalRcvFlush() indicating whether we're in the midst of dying?
> >
> > Surely if you do this then sync rep will fail to respond correctly if
> > WalReceiver dies.
> >
> > Why is it OK to write to disk, but not OK to reply?
>
> Because the connection might be dead. A broken connection is a likely
> cause of walreceiver death.

Would it not be possible to check that?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Feb 16, 2011 at 11:32 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, 2011-02-16 at 17:40 +0200, Heikki Linnakangas wrote:
>> On 16.02.2011 17:36, Simon Riggs wrote:
>> > On Tue, 2011-02-15 at 12:08 -0500, Robert Haas wrote:
>> >> On Mon, Feb 14, 2011 at 12:25 AM, Fujii Masao<masao(dot)fujii(at)gmail(dot)com>  wrote:
>> >>> On Fri, Feb 11, 2011 at 4:06 AM, Heikki Linnakangas
>> >>> <heikki(dot)linnakangas(at)enterprisedb(dot)com>  wrote:
>> >>>> I added a XLogWalRcvSendReply() call into XLogWalRcvFlush() so that it also
>> >>>> sends a status update every time the WAL is flushed. If the walreceiver is
>> >>>> busy receiving and flushing, that would happen once per WAL segment, which
>> >>>> seems sensible.
>> >>>
>> >>> This change can make the callback function "WalRcvDie()" call ereport(ERROR)
>> >>> via XLogWalRcvFlush(). This looks unsafe.
>> >>
>> >> Good catch.  Is the cleanest solution to pass a boolean parameter to
>> >> XLogWalRcvFlush() indicating whether we're in the midst of dying?
>> >
>> > Surely if you do this then sync rep will fail to respond correctly if
>> > WalReceiver dies.
>> >
>> > Why is it OK to write to disk, but not OK to reply?
>>
>> Because the connection might be dead. A broken connection is a likely
>> cause of walreceiver death.
>
> Would it not be possible to check that?

I'm not actually sure that it matters that much whether we do or not.
ISTM that the WAL receiver is normally going to exit the main loop (in
WalReceiverMain) right here:

/* Process any requests or signals received recently */
ProcessWalRcvInterrupts();

But to get to that point, we either have to be making our first pass
through the loop (in which case nothing interesting has happened yet)
or we have to have just completed an iteration through the loop (in
which case we just sent a reply). I think that the only thing that
can have changed since the last reply is the replay position, which
this version of the sync rep patch doesn't care about anyway. Even if
it did, I'm not sure it'd be worth complicating the die path to
squeeze in one final reply.

Actually, on further reflection, I'm not even sure why we bother with
the fsync. It seems like a useful safeguard but I'm not seeing how we
can get to that point without having fsync'd everything anyway. Am I
missing something?

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

On 16.02.2011 19:29, Robert Haas wrote:
> Actually, on further reflection, I'm not even sure why we bother with
> the fsync. It seems like a useful safeguard but I'm not seeing how we
> can get to that point without having fsync'd everything anyway. Am I
> missing something?

WalRcvDie() is called on error. For example, if the connection dies
unexpectedly during walrcv_receive().

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

On Wed, Feb 16, 2011 at 12:34 PM, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> On 16.02.2011 19:29, Robert Haas wrote:
>>
>> Actually, on further reflection, I'm not even sure why we bother with
>> the fsync.  It seems like a useful safeguard but I'm not seeing how we
>> can get to that point without having fsync'd everything anyway.  Am I
>> missing something?
>
> WalRcvDie() is called on error. For example, if the connection dies
> unexpectedly during walrcv_receive().

Ah, OK.

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

On Fri, 2011-01-21 at 14:45 +0200, Heikki Linnakangas wrote:

> * The UI differs from what was agreed on here:
> http://archives.postgresql.org/message-id/4D1DCF5A.7070808@enterprisedb.com.

Patch to add server_name parameter, plus mechanism to send info from
standby to master. While doing that, refactor into 3 message types, not
just 1. This addresses Fujii's comment that we may not wish to send
feedback as often as other replies, but doesn't actually alter yet when
the feedback is sent (nor will I do that anytime soon).

Complete but rough hack, for comments, but nothing surprising.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, 2011-02-18 at 00:48 +0000, Simon Riggs wrote:

> Complete but rough hack, for comments, but nothing surprising.

This is an implicit requirement from our earlier agreed API, so its
blocking further work on Sync Rep.

I'm looking to commit this in about 3-4 hours unless I get comments.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services