Re: Online enabling of checksums

On Thu, Apr 5, 2018 at 7:30 PM, Magnus Hagander <magnus@hagander.net> wrote:

> On Thu, Apr 5, 2018 at 5:08 PM, Andrey Borodin <x4mmm@yandex-team.ru>
> wrote:
>
>>
>>
>> > 5 апр. 2018 г., в 19:58, Magnus Hagander <magnus@hagander.net>
>> написал(а):
>> >
>> >
>> >
>> > On Thu, Apr 5, 2018 at 4:55 PM, Andrey Borodin <x4mmm@yandex-team.ru>
>> wrote:
>> >
>> >
>> > > 5 апр. 2018 г., в 14:33, Tomas Vondra <tomas.vondra@2ndquadrant.com>
>> написал(а):
>> > >
>> > > This patch version seems fine to me. I'm inclined to mark it RFC.
>> > +1
>> > The patch works fine for me. I've tried different combinations of
>> backend cancelation and the only suspicious thing I found is that you can
>> start multiple workers by cancelling launcher and not cancelling worker. Is
>> it problematic behavior? If we run pg_enable_data_checksums() it checks for
>> existing launcher for a reason, m.b. it should check for worker too?
>> >
>> > I don't think it's a problem in itself -- it will cause pointless work,
>> but not actually cause any poroblems I think (whereas duplicate launchers
>> could cause interesting things to happen).
>> >
>> > How did you actually cancel the launcher to end up in this situation?
>> select pg_enable_data_checksums(10000,1);
>> select pg_sleep(0.1);
>> select pg_cancel_backend(pid),backend_type from pg_stat_activity where
>> backend_type ~ 'checksumhelper launcher' ;
>> select pg_enable_data_checksums(10000,1);
>> select pg_sleep(0.1);
>> select pg_cancel_backend(pid),backend_type from pg_stat_activity where
>> backend_type ~ 'checksumhelper launcher' ;
>> select pg_enable_data_checksums(10000,1);
>> select pg_sleep(0.1);
>> select pg_cancel_backend(pid),backend_type from pg_stat_activity where
>> backend_type ~ 'checksumhelper launcher' ;
>>
>> select pid,backend_type from pg_stat_activity where backend_type
>> ~'checks';
>>   pid  |     backend_type
>> -------+-----------------------
>>  98587 | checksumhelper worker
>>  98589 | checksumhelper worker
>>  98591 | checksumhelper worker
>> (3 rows)
>>
>> There is a way to shoot yourself in a leg then by calling
>> pg_disable_data_checksums(), but this is extremely stupid for a user
>>
>
>
> Ah, didn't consider query cancel. I'm not sure how much  we should
> actually care about it, but it's easy enough to trap that signal and just
> do a clean shutdown on it, so I've done that.
>
> PFA a patch that does that, and also rebased over the datallowconn patch
> just landed (which also removes some docs).
>
>

I have now pushed this latest version with some minor text adjustments and
a catversion bump.

Thanks for all the reviews!


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ <http://www.hagander.net/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>