Re: dropdb --force

Hi

so 2. 11. 2019 v 17:18 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> pá 25. 10. 2019 v 4:55 odesílatel Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> napsal:
>
>> On Thu, Oct 24, 2019 at 8:22 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >
>> > čt 24. 10. 2019 v 11:10 odesílatel Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> napsal:
>> >>
>> >> While making some changes in the patch, I noticed that in
>> >> TerminateOtherDBBackends, there is a race condition where after we
>> >> release the ProcArrayLock, the backend process to which we decided to
>> >> send a signal exits by itself and the same pid can be assigned to
>> >> another backend which is connected to some other database. This leads
>> >> to terminating a wrong backend. I think we have some similar race
>> >> condition at few other places like in pg_terminate_backend(),
>> >> ProcSleep() and CountOtherDBBackends(). I think here the risk is a
>> >> bit more because there could be a long list of pids.
>> >>
>> >> One idea could be that we write a new function similar to IsBackendPid
>> >> which takes dbid and ensures that pid belongs to that database and use
>> >> that before sending kill signal, but still it will not be completely
>> >> safe. But, I think it can be closer to cases like we already have in
>> >> code.
>> >>
>> >> Another possible idea could be to use the SendProcSignal mechanism
>> >> similar to how we have used it in CancelDBBackends() to allow the
>> >> required backends to exit by themselves. This might be safer.
>> >>
>> >> I am not sure if we can entirely eliminate this race condition and
>> >> whether it is a good idea to accept such a race condition even though
>> >> it exists in other parts of code. What do you think?
>> >>
>> >> BTW, I have added/revised some comments in the code and done few other
>> >> cosmetic changes, the result of which is attached.
>> >
>> >
>> > Tomorrow I'll check variants that you mentioned.
>> >
>> > We sure so there are not any new connect to removed database, because
>> we hold lock there.
>> >
>>
>> Right.
>>
>> > So check if oid db is same should be enough.
>> >
>>
>> We can do this before sending a kill signal but is it enough? Because
>> as soon as we release ProcArrayLock anytime the other process can exit
>> and a new process can use its pid. I think this is more of a
>> theoretical risk and might not be easy to hit, but still, we can't
>> ignore it.
>>
>
> yes, there is a theoretical risk probably - the released pid should near
> current fresh pid from range 0 .. pid_max.
>
> Probably the best solutions is enhancing SendProcSignal and using it here
> and fix CountOtherDBBackends.
>
> Alternative solution can be killing in block 50 processes and recheck.
> I'll try to write both and you can decide for one
>

I am sending patch where kill was replaced by SendProcSignal. I tested it
on pg_bench with 400 connections, and it works without problems.

Regards

Pavel

>
> Pavel
>
>
>> --
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>