Re: [BUG] Checkpointer on hot standby runs without looking checkpoint_segments

On Tue, Apr 24, 2012 at 3:47 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
>> >  - xlog.c: Make StandbyMode shared.
>> >
>> >  - checkpointer.c: Use IsStandbyMode() to check if postmaster is
>> >   under standby mode.
>>
>> IsStandbyMode() looks overkill to me. The standby mode flag is forcibly
>> turned off at the end of recovery, but its change doesn't need to be shared
>> to the checkpointer process, IOW, the shared flag doesn't need to change
>> since startup like XLogCtl->archiveCleanupCommand, I think. So we can
>> simplify the code to share the flag to the checkpointer. See the attached
>> patch (though not tested yet).
>
> Hmm. I understood that the aim of the spinlock and volatil'ize of
> the pointer in reading shared memory is to secure the memory
> consistency on SMPs with weak memory consistency and to make
> compiler help from over-optimization for non-volatile pointer
> respectively.
>
> You removed both of them in the patch.
>
> If we are allowed to be tolerant of the temporary lack of
> coherence in shared memory there, the spinlock could be removed.
> But the possibility to read garbage by using XLogCtl itself to
> access standbyMode does not seem to be tolerable. What do you
> think about that?

I'm not sure if we really need to worry about that for such shared variable
that doesn't change since it's been initialized at the start of recovery.
Anyway, if we really need to worry about that, we need to protect the
shared variable RecoveryTargetTLI and archiveCleanupCommand with
the spinlock because they are in the same situation as standbyMode.

Regards,

--
Fujii Masao