On Fri, Sep 27, 2013 at 7:56 AM, Merlin Moncure <mmonc...@gmail.com> wrote: > On Thu, Sep 26, 2013 at 10:14 PM, Merlin Moncure <mmonc...@gmail.com> wrote: >> On Thu, Sep 26, 2013 at 6:08 PM, Andres Freund <and...@2ndquadrant.com> >> wrote: >>> On 2013-08-27 12:17:55 -0500, Merlin Moncure wrote: >>>> On Tue, Aug 27, 2013 at 10:55 AM, Andres Freund <and...@2ndquadrant.com> >>>> wrote: >>>> > On 2013-08-27 09:57:38 -0500, Merlin Moncure wrote: >>>> >> + bool >>>> >> + RecoveryMightBeInProgress(void) >>>> >> + { >>>> >> + /* >>>> >> + * We check shared state each time only until we leave recovery >>>> >> mode. We >>>> >> + * can't re-enter recovery, so there's no need to keep checking >>>> >> after the >>>> >> + * shared variable has once been seen false. >>>> >> + */ >>>> >> + if (!LocalRecoveryInProgress) >>>> >> + return false; >>>> >> + else >>>> >> + { >>>> >> + /* use volatile pointer to prevent code rearrangement */ >>>> >> + volatile XLogCtlData *xlogctl = XLogCtl; >>>> >> + >>>> >> + /* Intentionally query xlogctl without spinlocking! */ >>>> >> + LocalRecoveryInProgress = >>>> >> xlogctl->SharedRecoveryInProgress; >>>> >> + >>>> >> + return LocalRecoveryInProgress; >>>> >> + } >>>> >> + } >>>> > >>>> > I don't think it's acceptable to *set* LocalRecoveryInProgress >>>> > here. That should only be done in the normal routine. >>>> >>>> quite right -- that was a major error -- you could bypass the >>>> initialization call to the xlog with some bad luck. >>> >>> I've seen this in profiles since, so I'd appreciate pushing this >>> forward. >> >> roger that -- will push ahead when I get into the office... > > attached is new version fixing some comment typos.
Attached is simplified patch that replaces the spinlock with a read barrier based on a suggestion made by Andres offlist. The approach has different performance characteristics -- a barrier call is being issued instead of a non-blocking read. I don't have a performance test case in hand to prove that's better so I'm going with Andre's approach because it's simpler. Aside: can this truly the only caller of pg_read_barrier()? Also, moving to -hackers from -performance. merlin
recovery5.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers