Subject: Re: Sync Rep for 2011CF1
From: Simon Riggs <simon@2ndQuadrant.com>
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: pgsql-hackers@postgresql.org
In-Reply-To: <4D397FE4.2020709@enterprisedb.com>
References: <1295127631.3282.100.camel@ebony>
	<4D397FE4.2020709@enterprisedb.com>
Content-Type: text/plain; charset="UTF-8"
Date: Fri, 21 Jan 2011 13:24:30 +0000
Message-ID: <1295616270.1803.10799.camel@ebony>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit

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