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.
>
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
