Re: xpath improvement V2 with xml.c

Hi all,

I've combined the review suggestions of Jan Urbański, Scott Bailey, and
others.
This was a lot harder, then I had foreseen; and I took my time to do it
the right way (hope you agree!).

BTW. really appreciate your efforts, so far, to enlighten me on nuub
errors/mistakes in the previous version.

Additional improvement (hence the V2):
two extra functions: xpath_value_text and xpath_value_strict
Both are quite general: xpath_value_text maps everything to text, except
nodeset. xpath_value_strict has to be told exactly what to expect.
xpath_value_text(text xpath, xml doc [, namespace]) returns text
xpath_value_strict(anyelement typexample, text xpath, xml doc [,
namespace]) returns anyelement
See the doc for further explanation.
I chose this approach, as opposed to xpath_value_string/number/boolean
for a couple of reasons:
- We want postgresql functions, with postgresql types. The functions
should fit postgresql usage and hide libxml parlance.
- Functions in pg_catalog should be destined for broad use, not just
satisfy the adhoc desire of the implementer.
- Function synopsis should be adequate to withstand libxml extension or
improvements (libxml3?)
- Construction of XPath expressions for value retrieval is not trivial.
A precise calling syntax, hopefully, focuses the user to select the
right expressions.
- Loose implementation with "autocasting" opens the door for unwanted
injection possibilities.

Lastly, when in need of a xpath_value_string (et al), it can easily be
added by the user through:
CREATE FUNCTION xpath_value_string(text, xml) RETURNS text AS $$
SELECT xpath_value_strict('a'::text,$1,$2);
$$ LANGUAGE SQL;

Points fixed:
- source code indentation. (manually though, can't get pgindent to work
properly)
- Doc entries with some examples
- test/regress entries
- Detailed directions from Jan Urbanski on ereport and PG_TRY/CATCH.

Here's a shortlist of subjects you should definetely review (aka I'me
not certain these are up to standards)
- calling style in xpath_value_strict with typexample. I really like it,
but I haven't seen such an approach elsewhere in the API.
- tone and style in doc. I'me not native English speaker and had to
learn DocBook along the way.
- insertion in catalog/pg_proc.h. Just took some free oid numbers, and
fiddled the other parameters.
- Is ERRCODE_DATA_EXCEPTION (in xml.c) acceptable as error code for a
type mismatch? Could not find anything better.

kind regards, Arie Bikker

And here is the patch.

And a patch for the code implementing this

Arie Bikker wrote:
> Hi all,
>
> I've combined the review suggestions of Jan Urbański, Scott Bailey, and
> others.
> This was a lot harder, then I had foreseen; and I took my time to do it
> the right way (hope you agree!).

Hi,

I see the patch has been marked as "Returned with Feedback" on the 6th
of February, I assume on grounds of prolonged silence about it. I
confess it was partly my fault, because soon after posting the review I
suddenly had to focus on other things.

I won't be able to review the new version in the next few days, and this
commitfest is closing anyway... However I would hate to see that patch
just disappear, as I think it's useful and you obviously invested some
work in it.

At this stage I would suggest moving it to the first 9.1 commitfest,
since it's a nice feature, but not one we should burden the committers
with this late in the development cycle.

Arie, care to add that last version of the patch to the 2010-Next
commitfest?

Cheers,
Jan

Jan Urbański wrote:
> Arie Bikker wrote:
>> Hi all,
>>
>> I've combined the review suggestions of Jan Urbański, Scott Bailey, and
>> others.
>> This was a lot harder, then I had foreseen; and I took my time to do it
>> the right way (hope you agree!).
>
> Hi,
>
> I see the patch has been marked as "Returned with Feedback" on the 6th
> of February, I assume on grounds of prolonged silence about it. I
> confess it was partly my fault, because soon after posting the review I
> suddenly had to focus on other things.
>
> I won't be able to review the new version in the next few days, and this
> commitfest is closing anyway... However I would hate to see that patch
> just disappear, as I think it's useful and you obviously invested some
> work in it.
>
> At this stage I would suggest moving it to the first 9.1 commitfest,
> since it's a nice feature, but not one we should burden the committers
> with this late in the development cycle.
>
> Arie, care to add that last version of the patch to the 2010-Next
> commitfest?
>
> Cheers,
> Jan

I would agree w/ Jan about just adding to 9.1 except that it fixes
several known bugs.

Scott