On Tue, Nov 21, 2017 at 3:11 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Mon, Nov 20, 2017 at 4:12 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> I agree with your approach. It makes sense to me.
>>
>> Attached updated patch. Please review it.
>
> Thanks for updating the patch! The patch basically looks good to me.
I am not seeing problems either. The start and stop logic of base
backups is what I would expect they should.
> + /*
> + * Clean up session-level lock. To avoid calling CHECK_FOR_INTERRUPTS by
> + * LWLockReleaseClearVar before changing the backup state we change it
> + * while holding the WAL insert lock.
> + */
>
> I think that you should comment *why* we need to avoid calling
> CHECK_FOR_INTERRUPTS before changing the backup state, here.
You could just add "as this allows to keep backup counters kept in
shared memory consistent with the state of the session starting or
stopping a backup.".
--
Michael