On 21.11.2013 17:08, Merlin Moncure wrote:
> On Thu, Nov 21, 2013 at 9:02 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> On 2013-11-21 16:25:02 +0200, Heikki Linnakangas wrote:
>>> Hmm. All callers of RecoveryInProgress() must be prepared to handle the case
>>> that RecoveryInProgress() returns true, but the system is no longer in
>>> recovery. No matter what locking we do in RecoveryInProgress(), the startup
>>> process might finish recovery just after RecoveryInProgress() has returned.
>>
>> True.
>>
>>> What about the attached? It reads the shared variable without a lock or
>>> barrier. If it returns 'true', but the system in fact just exited recovery,
>>> that's OK. As explained above, all the callers must tolerate that anyway.
>>> But if it returns 'false', then it performs a full memory barrier, which
>>> should ensure that it sees any other shared variables as it is after the
>>> startup process cleared SharedRecoveryInProgress (notably,
>>> XLogCtl->ThisTimeLineID).
>>
>> I'd argue that we should also remove the spinlock in StartupXLOG and
>> replace it with a write barrier. Obviously not for performance reasons,
>> but because somebody might add more code to run under that spinlock.
>>
>> Looks good otherwise, although a read memory barrier ought to suffice.
>
> This code is in a very hot code path. Are we *sure* that the read
> barrier is fast enough that we don't want to provide an alternate
> function that only returns the local flag? I don't know enough about
> them to say either way.
In my patch, I put the barrier inside the if (!LocalRecoveryInProgress)
block. That codepath can only execute once in a backend, so performance
is not an issue there. Does that look sane to you?
- Heikki
