Re: Sync Rep and shutdown Re: Sync Rep v19

On Fri, Mar 18, 2011 at 1:15 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, 2011-03-17 at 09:33 -0400, Robert Haas wrote:
>> Thanks for the review!
>
> Lets have a look here...
>
> You've added a test inside the lock to see if there is a standby, which
> I took out for performance reasons. Maybe there's another way, I know
> that code is fiddly.
>
> You've also added back in the lock acquisition at wakeup with very
> little justification, which was a major performance hit.
>
> Together that's about a >20% hit in performance in Yeb's tests. I think
> you should spend a little time thinking how to retune that.

Ouch.  Do you have a link that describes his testing methodology?  I
will look at it.

> I see handling added for ProcDiePending and QueryCancelPending directly
> into syncrep.c without any comments in postgres.c to indicate that you
> bypass ProcessInterrupts() in some cases. That looks pretty hokey to me.

I can add some comments.  Unfortunately, it's not feasible to call
ProcessInterrupts() directly from this point in the code - it causes a
database panic.

> SyncRepUpdateSyncStandbysDefined() is added into walwriter, which means
> waiters won't be released if we do a sighup during a fast shutdown,
> since the walwriter gets killed as soon as that starts. I'm thinking
> bgwriter should handle that now.

Hmm.  I was thinking that doing it in WAL writer would make it more
responsive, but since this is a fairly unlikely scenario, it's
probably not worth complicating the shutdown sequence to do it the way
I did.  I'll move it to bgwriter.

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