I had the possibility to perform tests on 9.5, and can confirm the
memory leak I was seeing is solved with the patch (and that's great :) )
Regards
Marc
On 18/04/2016 17:53, Julien Rouhaud wrote:
> On 18/04/2016 16:33, Tom Lane wrote:
>> I poked at this over the weekend, and got more unhappy the more I poked.
>> Aside from the memory leakage issue, there are multiple coding-rule
>> violations besides the one you noted about scope of the critical sections.
>> One example is that in the page-split path for an inner page, we modify
>> the contents of childbuf long before getting into the critical section.
>> The number of out-of-date comments was depressingly large as well.
>>
>> I ended up deciding that the most reasonable fix was along the lines of
>> my first alternative above. In the attached draft patches, the
>> "placeToPage" method is split into two methods, "beginPlaceToPage" and
>> "execPlaceToPage", where the second method is what's supposed to happen
>> inside the critical section for a non-page-splitting update. Nothing
>> that's done inside beginPlaceToPage is critical.
>>
>> I've tested this to the extent of verifying that it passes make
>> check-world and stops the memory leak in your test case, but it could use
>> more eyeballs on it.
>>
> Thanks! I also started working on it but it was very far from being
> complete (and already much more ugly...).
>
> I couldn't find any problem in the patch.
>
> I wonder if asserting being in a critical section would be valuable in
> the *execPlaceToPage functions, or that (leaf->walinfolen > 0) in
> dataExecPlaceToPageLeaf(), since it's filled far from this function.
>
>> Attached are draft patches against HEAD and 9.5 (they're the same except
>> for BufferGetPage calls). I think we'd also want to back-patch to 9.4,
>> but that will take considerable additional work because of the XLOG API
>> rewrite that happened in 9.5. I also noted that some of the coding-rule
>> violations seem to be new in 9.5, so the problems may be less severe in
>> 9.4 --- the memory leak definitely exists there, though.
>>
