Re: (ITS#8844) LMDB mdb_env_close is unsafe in forked child

[h.b.furuseth@usit.uio.no]

On 14/06/2018 21:04, Howard Chu wrote:
> Hallvard Breien Furuseth wrote:
>> On 14/06/2018 20:34, Howard Chu wrote:
>>> If there are any table entries under that PID there's no problem 
>>> removing them.
>>
>> The problem is that it's arbitrary.  Something is exiting without
>> clearing its pid slots, and 0.001% of the time this code will catch it.
> 
> Sure. But I don't think it's worthwhile to make a special case here to 
> skip this step.

To my eyes the getpid() is a special case already, to someone trying to
understand the code. Though adding a comment could fix that. See below.

>> Better to skip this cleanup, or to always do it - but "always" belongs
>> in env_open().   If a new env is seeing its pid in use, then it can 
>> safely clean it out. Last we talked about that, I liked it and you
>> did not:-)
> 
> And I don't see any particular reason to add this step.

right

>>(...)
>> Parent, pid P:
>> Â  open env A.
>>
>> Child, pid Q:
>> - open and start using env B with the same path.
>> - close env A, while B is still open.
>> Â Â  With the new code, this clears pid Q - i.e. B's pids.
>> Â Â  But lmdb.h already lists that as a "don't do that",
>> Â Â  because it also removes B's lock on the lockfile.
>>
> Yeah, this is still a "don't do that" though, regardless of this 
> particular ITS.

Right.  So in summary, there are two cases when getpid() can be there
to clean out, and in neither case does it make sense to clean it out.

Other than the sense of avoiding one "if () { init two vars; }".
I like that better than a comment about not bothering to, but a comment
is OK too.