Re: [REVIEW] Re: Fix xpath() to return namespace definitions

On Mon, Dec 15, 2014 at 1:02 PM, Ali Akbar <the.apaan@gmail.com> wrote:
> 2014-12-15 10:19 GMT+07:00 Michael Paquier <michael.paquier@gmail.com>:
>>
>> On Mon, Dec 15, 2014 at 9:05 AM, Ali Akbar <the.apaan@gmail.com> wrote:
>> > Peter, while reviewing the better performing patch myself, now i think
>> > the
>> > patch needs more work to be committed. The structuring of the method
>> > will be
>> > confusing in the long term. I think I'll restructure the patch in the
>> > next
>> > commitfest.
>> > So i propose to break the patch:
>> > 1. We apply the current patch which uses xmlNodeCopy, so that the
>> > long-standing bug will be fixed in postgres.
>> > 2. I'll work with the performance enhancement in the next commitfest.
>> >
>> > Maybe for (2), the current better-performing patch can be viewed as PoC
>> > of
>> > the expected performance.
>>
>> Ali, are you currently working on that? Would you mind re-creating new
>> entries in the commit fest app for the new set of patches that you are
>> planning to do?
>> For now I am switching this patch as returned with feedback.
>> Thanks,
>
>
> What i mean, the last patch (v7 patch) as it is is enough to fix the bug
> (nested xpath namespace problem). I think the performance regression is
> still acceptable (in my case it's ~20%), because the bug is severe.
> Currently, xpath can return invalid xml because the namespace isn't included
> in the output!
>
> What i'll be working is the v4 patch, because it turns out the v4 patch has
> better performance (~10%, but Peter's test shows it isn't the case). But,
> the problem is the v4 patch is organized wrongly, and hacks around the
> libxml's xml node structure (duplicating the namespace on the existing
> structure). I'll work on that, but it will affects the performance benefit.
>
> So what i propose is, we close the longstanding bug in this comitfest with
> the v7 patch. I'll work on improving the performance, without compromising
> good code structure. If the result is good, i'll submit the patch.

OK. Could you then move this patch to the new CF with Needs Review 

with Peter as reviewer then? He seems to be looking at it anyway
seeing the update from 12/11.