Re: Initial review of xslt with no limits patch

Hi Pavel,

Currently your patch isn't applying to head, from the looks of things a
function signature has changed. Can you update your patch please?

Also, having had a read through the patch itself I note that there are
no tests and no changes to documentation. Shouldn't the documentation
advertise that the there are no limits on the numbers of parameters? A
couple of tests will also help me to test your patch,

Below is the results of running patch:

patch -p0 < ../nolimits.diff
patching file ./contrib/xml2/xslt_proc.c
Hunk #1 FAILED at 42.
Hunk #2 succeeded at 57 (offset -2 lines).
Hunk #3 succeeded at 69 (offset -2 lines).
Hunk #4 succeeded at 142 (offset -4 lines).
Hunk #5 succeeded at 179 (offset -4 lines).
Hunk #6 succeeded at 192 with fuzz 1 (offset -4 lines).
1 out of 6 hunks FAILED -- saving rejects to file
./contrib/xml2/xslt_proc.c.rej

The rejects were:

*** ./contrib/xml2/xslt_proc.c.orig 2010-03-03 20:10:22.000000000 +0100
--- ./contrib/xml2/xslt_proc.c 2010-05-03 15:07:17.010918303 +0200
***************
*** 42,50 ****
extern void pgxml_parser_init(void);

/* local defs */
! static void parse_params(const char **params, text *paramstr);

! #define MAXPARAMS 20 /* must be even, see
parse_params() */

#endif /* USE_LIBXSLT */

--- 42,51 ----
extern void pgxml_parser_init(void);

/* local defs */
! const char **parse_params(text *paramstr);

! #define INIT_PARAMS 20 /* must be even, see
parse_params() */
! #define EXTEND_PARAMS 20 /* must be even, see
parse_params() */

#endif /* USE_LIBXSLT */

Regards,
--
Mike Fowler
Registered Linux user: 379787

Hello

2010/8/2 Mike Fowler <mike(at)mlfowler(dot)com>:
> Hi Pavel,
>
> Currently your patch isn't applying to head, from the looks of things a
> function signature has changed. Can you update your patch please?
>

yes - see attachment

> Also, having had a read through the patch itself I note that there are no
> tests and no changes to documentation. Shouldn't the documentation advertise
> that the there are no limits on the numbers of parameters? A couple of tests
> will also help me to test your patch,
>

the limit of 10 parameters was not documented. With this patch, there
are not limit - or limit comes from libxml2.

Test are a problem - for me - I don't understand to xslt too much - so
I am not able to write xslt template with more than 10 params.

I looked there and I the params are not tested now - so it is
necessary to write a new set of regress tests. But I am not able to do
it :(.

> Below is the results of running patch:
>
> patch -p0 < ../nolimits.diff
> patching file ./contrib/xml2/xslt_proc.c
> Hunk #1 FAILED at 42.
> Hunk #2 succeeded at 57 (offset -2 lines).
> Hunk #3 succeeded at 69 (offset -2 lines).
> Hunk #4 succeeded at 142 (offset -4 lines).
> Hunk #5 succeeded at 179 (offset -4 lines).
> Hunk #6 succeeded at 192 with fuzz 1 (offset -4 lines).
> 1 out of 6 hunks FAILED -- saving rejects to file
> ./contrib/xml2/xslt_proc.c.rej
>
> The rejects were:
>
>
> *** ./contrib/xml2/xslt_proc.c.orig     2010-03-03 20:10:22.000000000 +0100
> --- ./contrib/xml2/xslt_proc.c  2010-05-03 15:07:17.010918303 +0200
> ***************
> *** 42,50 ****
>  extern void pgxml_parser_init(void);
>
>  /* local defs */
> ! static void parse_params(const char **params, text *paramstr);
>
> ! #define MAXPARAMS 20                  /* must be even, see parse_params()
> */
>
>  #endif /* USE_LIBXSLT */
>
> --- 42,51 ----
>  extern void pgxml_parser_init(void);
>
>  /* local defs */
> ! const char **parse_params(text *paramstr);
>
> ! #define INIT_PARAMS 20                        /* must be even, see
> parse_params() */
> ! #define EXTEND_PARAMS 20              /* must be even, see parse_params()
> */
>
>  #endif /* USE_LIBXSLT */
>
>
> Regards,
> --
> Mike Fowler
> Registered Linux user: 379787
>

Regards

Pavel Stehule

Hi Pavel,

On 02/08/10 09:21, Pavel Stehule wrote:
> Hello
>
> 2010/8/2 Mike Fowler<mike(at)mlfowler(dot)com>:
>> Hi Pavel,
>>
>> Currently your patch isn't applying to head, from the looks of things a
>> function signature has changed. Can you update your patch please?
>>
>
> yes - see attachment
>

Thanks, the new patch applies cleanly. However I've been attempting to
run the parameterised XSLT this evening but to no avail. Reverting your
code I've discovered that it does not work in the old version either.

Given the complete lack of documentation (not your fault) it's always
possible that I'm doing something wrong. Given the query below, you
should get the XML that follows, and indeed in oXygen (a standalone XML
tool) you do:

SELECT
xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
$$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
version="1.0">
<xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
<xsl:strip-space elements="*"/>
<xsl:param name="n1"/>
<xsl:param name="n2"/>
<xsl:param name="n3"/>
<xsl:param name="n4"/>
<xsl:param name="n5" select="'me'"/>
<xsl:template match="*">
<xsl:element name="samples">
<xsl:element name="sample">
<xsl:value-of select="$n1"/>
</xsl:element>
<xsl:element name="sample">
<xsl:value-of select="$n2"/>
</xsl:element>
<xsl:element name="sample">
<xsl:value-of select="$n3"/>
</xsl:element>
<xsl:element name="sample">
<xsl:value-of select="$n4"/>
</xsl:element>
<xsl:element name="sample">
<xsl:value-of select="$n5"/>
</xsl:element>
</xsl:element>
</xsl:template>
</xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)

<samples>
<sample>v1</sample>
<sample>v2</sample>
<sample>v3</sample>
<sample>v4</sample>
<sample>v5</sample>
</samples>

Sadly I get the following in both versions:

<samples>
<sample/>
<sample/>
<sample/>
<sample/>
<sample/>
</samples>

Unless you can see anything I'm doing wrong I suggest we mark this patch
either 'Returned with feedback' or 'Rejected'. Since contrib/xml2 is
deprecated, perhaps a better way forward is to pull XSLT handling into
core and fix both the apparent inability to handle parameters as well as
the limited number of parameters.

Regards,

--
Mike Fowler
Registered Linux user: 379787

2010/8/6 Mike Fowler <mike(at)mlfowler(dot)com>:
> Hi Pavel,
>
> On 02/08/10 09:21, Pavel Stehule wrote:
>>
>> Hello
>>
>> 2010/8/2 Mike Fowler<mike(at)mlfowler(dot)com>:
>>>
>>> Hi Pavel,
>>>
>>> Currently your patch isn't applying to head, from the looks of things a
>>> function signature has changed. Can you update your patch please?
>>>
>>
>> yes - see attachment
>>
>
> Thanks, the new patch applies cleanly. However I've been attempting to run
> the parameterised XSLT this evening but to no avail. Reverting your code
> I've discovered that it does not work in the old version either.
>
> Given the complete lack of documentation (not your fault) it's always
> possible that I'm doing something wrong. Given the query below, you should
> get the XML that follows, and indeed in oXygen (a standalone XML tool) you
> do:
>
> SELECT
> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> version="1.0">
>   <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
>   <xsl:strip-space elements="*"/>
>   <xsl:param name="n1"/>
>   <xsl:param name="n2"/>
>   <xsl:param name="n3"/>
>   <xsl:param name="n4"/>
>   <xsl:param name="n5" select="'me'"/>
>   <xsl:template match="*">
>     <xsl:element name="samples">
>       <xsl:element name="sample">
>         <xsl:value-of select="$n1"/>
>       </xsl:element>
>       <xsl:element name="sample">
>         <xsl:value-of select="$n2"/>
>       </xsl:element>
>       <xsl:element name="sample">
>         <xsl:value-of select="$n3"/>
>       </xsl:element>
>       <xsl:element name="sample">
>         <xsl:value-of select="$n4"/>
>       </xsl:element>
>       <xsl:element name="sample">
>         <xsl:value-of select="$n5"/>
>       </xsl:element>
>     </xsl:element>
>   </xsl:template>
> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>
> <samples>
>  <sample>v1</sample>
>  <sample>v2</sample>
>  <sample>v3</sample>
>  <sample>v4</sample>
>  <sample>v5</sample>
> </samples>
>
> Sadly I get the following in both versions:
>
> <samples>
>  <sample/>
>  <sample/>
>  <sample/>
>  <sample/>
>  <sample/>
> </samples>
>
>
> Unless you can see anything I'm doing wrong I suggest we mark this patch
> either 'Returned with feedback' or 'Rejected'. Since contrib/xml2 is
> deprecated, perhaps a better way forward is to pull XSLT handling into core
> and fix both the apparent inability to handle parameters as well as the
> limited number of parameters.

there is some wrong, but I am not able to sey what now. But this patch
is very simply. I'll fix it today.

Pavel

>
> Regards,
>
> --
> Mike Fowler
> Registered Linux user: 379787
>

On 08/05/2010 06:56 PM, Mike Fowler wrote:
>
> SELECT
> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> version="1.0">
> <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
>
[snip]
> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>
>

I haven't been paying attention to this, so sorry if this has been
discussed before, but it just caught my eye. Why are we passing these
params as a comma-separated list rather than as an array or as a
variadic list of params? This looks rather ugly. What if you want to
have a param that includes a comma?

cheers

andrew

2010/8/6 Andrew Dunstan <andrew(at)dunslane(dot)net>:
>
>
> On 08/05/2010 06:56 PM, Mike Fowler wrote:
>>
>> SELECT
>> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
>> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
>> version="1.0">
>> <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
>>
> [snip]
>>
>> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>>
>>
>
> I haven't been paying attention to this, so sorry if this has been discussed
> before, but it just caught my eye. Why are we passing these params as a
> comma-separated list rather than as an array or as a variadic list of
> params? This looks rather ugly. What if you want to have a param that
> includes a comma?
>

There is probably problem in pairs - label = value. Can be nice, if we
can use a variadic functions for this, but I am afraid, ...

using a variadic function isn't too much nice now

some xslt_process(xmlsrc, 'n1=v1','n2=v2','n3=v3'

The same is true for array. Pg hasn't hash available from SQL level

I am thinking about new kind of functions - with only positionals
arguments. And internal parameter can be a array of used labels.

Regards

Pavel Stehule

> cheers
>
> andrew
>

On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:
> 2010/8/6 Andrew Dunstan <andrew(at)dunslane(dot)net>:
> > On 08/05/2010 06:56 PM, Mike Fowler wrote:
> >> SELECT
> >> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
> >> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> >> version="1.0">
> >> <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
> >>
> > [snip]
> >>
> >> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
> >
> > I haven't been paying attention to this, so sorry if this has been discussed
> > before, but it just caught my eye. Why are we passing these params as a
> > comma-separated list rather than as an array or as a variadic list of
> > params? This looks rather ugly. What if you want to have a param that
> > includes a comma?
>
> There is probably problem in pairs - label = value. Can be nice, if we
> can use a variadic functions for this, but I am afraid, ...
>
> using a variadic function isn't too much nice now
>
> some xslt_process(xmlsrc, 'n1=v1','n2=v2','n3=v3'

This sounds like the perfect case for pulling hstore into core code. :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

2010/8/6 David Fetter <david(at)fetter(dot)org>:
> On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:
>> 2010/8/6 Andrew Dunstan <andrew(at)dunslane(dot)net>:
>> > On 08/05/2010 06:56 PM, Mike Fowler wrote:
>> >> SELECT
>> >> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
>> >> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
>> >> version="1.0">
>> >> <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
>> >>
>> > [snip]
>> >>
>> >> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>> >
>> > I haven't been paying attention to this, so sorry if this has been discussed
>> > before, but it just caught my eye. Why are we passing these params as a
>> > comma-separated list rather than as an array or as a variadic list of
>> > params? This looks rather ugly. What if you want to have a param that
>> > includes a comma?
>>
>> There is probably problem in pairs - label = value. Can be nice, if we
>> can use a variadic functions for this, but I am afraid, ...
>>
>> using a variadic function isn't too much nice now
>>
>> some xslt_process(xmlsrc, 'n1=v1','n2=v2','n3=v3'
>
> This sounds like the perfect case for pulling hstore into core code. :)

I afraid so integration of hstore can break and block work on real
hash support. I would to have hash tables in core, but with usual
features and usual syntax - like Perl or PHP

Regards

Pavel

>
> Cheers,
> David.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
> Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com
> iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate
>

On 08/06/2010 02:29 AM, Pavel Stehule wrote:
> 2010/8/6 David Fetter<david(at)fetter(dot)org>:
>> On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:
>>> 2010/8/6 Andrew Dunstan<andrew(at)dunslane(dot)net>:
>>>> On 08/05/2010 06:56 PM, Mike Fowler wrote:
>>>>> SELECT
>>>>> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
>>>>> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
>>>>> version="1.0">
>>>>> <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
>>>>>
>>>> [snip]
>>>>> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>>>> I haven't been paying attention to this, so sorry if this has been discussed
>>>> before, but it just caught my eye. Why are we passing these params as a
>>>> comma-separated list rather than as an array or as a variadic list of
>>>> params? This looks rather ugly. What if you want to have a param that
>>>> includes a comma?
>>> There is probably problem in pairs - label = value. Can be nice, if we
>>> can use a variadic functions for this, but I am afraid, ...
>>>
>>> using a variadic function isn't too much nice now
>>>
>>> some xslt_process(xmlsrc, 'n1=v1','n2=v2','n3=v3'
>> This sounds like the perfect case for pulling hstore into core code. :)
> I afraid so integration of hstore can break and block work on real
> hash support. I would to have hash tables in core, but with usual
> features and usual syntax - like Perl or PHP
>

Can we just keep this discussion within reasonable bounds? The issue is
not hstore or other hashes, but how to do the param list for xslt sanely
given what we have now. A variadic list will be much nicer than what is
currently proposed.

cheers

andrew

On 06/08/10 15:08, Andrew Dunstan wrote:
>
>
> On 08/06/2010 02:29 AM, Pavel Stehule wrote:
>> 2010/8/6 David Fetter<david(at)fetter(dot)org>:
>>> On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:
>>>> 2010/8/6 Andrew Dunstan<andrew(at)dunslane(dot)net>:
>>>>> On 08/05/2010 06:56 PM, Mike Fowler wrote:
>>>>>> SELECT
>>>>>> xslt_process('<employee><name>cim</name><age>30</age><pay>400</pay></employee>'::text,
>>>>>>
>>>>>> $$<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
>>>>>> version="1.0">
>>>>>> <xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
>>>>>>
>>>>> [snip]
>>>>>> </xsl:stylesheet>$$::text, 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>>>>> I haven't been paying attention to this, so sorry if this has been
>>>>> discussed
>>>>> before, but it just caught my eye. Why are we passing these params
>>>>> as a
>>>>> comma-separated list rather than as an array or as a variadic list of
>>>>> params? This looks rather ugly. What if you want to have a param that
>>>>> includes a comma?
>>>> There is probably problem in pairs - label = value. Can be nice, if we
>>>> can use a variadic functions for this, but I am afraid, ...
>>>>
>>>> using a variadic function isn't too much nice now
>>>>
>>>> some xslt_process(xmlsrc, 'n1=v1','n2=v2','n3=v3'
>>> This sounds like the perfect case for pulling hstore into core code. :)
>> I afraid so integration of hstore can break and block work on real
>> hash support. I would to have hash tables in core, but with usual
>> features and usual syntax - like Perl or PHP
>>
>
> Can we just keep this discussion within reasonable bounds? The issue
> is not hstore or other hashes, but how to do the param list for xslt
> sanely given what we have now. A variadic list will be much nicer than
> what is currently proposed.
>
> cheers
>
> andrew

+1

Variadic seems the most sensible to me anyways.

However the more urgent problem is, pending someone spotting an error in
my ways, neither the existing code or the patched code appear able to
evaluate the parameters. Note than in my example I supplied a default
value for the fifth parameter and not even that value is appearing in
the outputted XML.

Regards,

--
Mike Fowler
Registered Linux user: 379787

Mike Fowler <mike(at)mlfowler(dot)com> writes:
> SELECT
> xslt_process( ... , ... ,
> 'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)

produces

> <samples>
> <sample>v1</sample>
> <sample>v2</sample>
> <sample>v3</sample>
> <sample>v4</sample>
> <sample>v5</sample>
> </samples>

> Sadly I get the following in both versions:

> <samples>
> <sample/>
> <sample/>
> <sample/>
> <sample/>
> <sample/>
> </samples>

Some examination of
http://www.xmlsoft.org/XSLT/tutorial/libxslttutorial.html
suggests that the parameter values need to be single-quoted,
and indeed when I change the last part of your example to

'n1=''v1'',n2=''v2'',n3=''v3'',n4=''v4'',n5=''v5'''::text);

I get

xslt_process
-----------------------
<samples> +
<sample>v1</sample>+
<sample>v2</sample>+
<sample>v3</sample>+
<sample>v4</sample>+
<sample>v5</sample>+
</samples> +

(1 row)

So this seems to be a documentation problem more than a code problem.

(It's a bit distressing to notice that the regression tests for the
module fail to exercise 3-parameter xslt_process at all, though.)

regards, tom lane

Hello

attached updated patch with regression test

2010/8/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Mike Fowler <mike(at)mlfowler(dot)com> writes:
>> SELECT
>> xslt_process( ... , ... ,
>>              'n1=v1,n2=v2,n3=v3,n4=v4,n5=v5'::text)
>
> produces
>
>> <samples>
>>    <sample>v1</sample>
>>    <sample>v2</sample>
>>    <sample>v3</sample>
>>    <sample>v4</sample>
>>    <sample>v5</sample>
>> </samples>
>
>> Sadly I get the following in both versions:
>
>> <samples>
>>    <sample/>
>>    <sample/>
>>    <sample/>
>>    <sample/>
>>    <sample/>
>> </samples>
>
> Some examination of
> http://www.xmlsoft.org/XSLT/tutorial/libxslttutorial.html
> suggests that the parameter values need to be single-quoted,
> and indeed when I change the last part of your example to
>
>        'n1=''v1'',n2=''v2'',n3=''v3'',n4=''v4'',n5=''v5'''::text);
>
> I get
>
>     xslt_process
> -----------------------
>  <samples>            +
>   <sample>v1</sample>+
>   <sample>v2</sample>+
>   <sample>v3</sample>+
>   <sample>v4</sample>+
>   <sample>v5</sample>+
>  </samples>           +
>
> (1 row)
>
> So this seems to be a documentation problem more than a code problem.
>
> (It's a bit distressing to notice that the regression tests for the
> module fail to exercise 3-parameter xslt_process at all, though.)
>

??? I don't see it

Regards

Pavel Stehule

>                        regards, tom lane
>

On 08/06/2010 12:15 PM, Tom Lane wrote:
>
> Some examination of
> http://www.xmlsoft.org/XSLT/tutorial/libxslttutorial.html
> suggests that the parameter values need to be single-quoted,
> and indeed when I change the last part of your example to
>
> 'n1=''v1'',n2=''v2'',n3=''v3'',n4=''v4'',n5=''v5'''::text);
>
>

Which would look a whole lot nicer with dollar quoting ;-)

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 08/06/2010 12:15 PM, Tom Lane wrote:
>> Some examination of
>> http://www.xmlsoft.org/XSLT/tutorial/libxslttutorial.html
>> suggests that the parameter values need to be single-quoted,
>> and indeed when I change the last part of your example to
>>
>> 'n1=''v1'',n2=''v2'',n3=''v3'',n4=''v4'',n5=''v5'''::text);

> Which would look a whole lot nicer with dollar quoting ;-)

No doubt. But one would assume that constant parameters aren't going
to be the normal use-case, and dollar quoting isn't helpful for
nonconstant text.

I think there are issues here that we need to take a step back and think
about. Right now, thanks to the lack of documentation, we can probably
assume there are approximately zero users of the xslt_process parameter
feature. Once we document it that'll no longer be true. So right now
would be the time to reflect on whether this is a specification we
actually like or believe is usable; it'll be too late to change it
later.

There are two specific points bothering me now that I see how it works:

1. name = value pretty much sucks, especially with the 100% lack of any
quoting convention for either equals or comma. I concur with the
thoughts upthread that turning this into a variadic function would be a
more sensible solution.

2. I'm not sure whether we ought to auto-single-quote the values.
If we don't, how hard is it for users to properly quote nonconstant
parameter values? (Will quote_literal work, or are the quoting rules
different for libxslt?) If we do, are we giving up functionality
someone cares about?

regards, tom lane

2010/8/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> On 08/06/2010 12:15 PM, Tom Lane wrote:
>>> Some examination of
>>> http://www.xmlsoft.org/XSLT/tutorial/libxslttutorial.html
>>> suggests that the parameter values need to be single-quoted,
>>> and indeed when I change the last part of your example to
>>>
>>> 'n1=''v1'',n2=''v2'',n3=''v3'',n4=''v4'',n5=''v5'''::text);
>
>> Which would look a whole lot nicer with dollar quoting ;-)
>
> No doubt.  But one would assume that constant parameters aren't going
> to be the normal use-case, and dollar quoting isn't helpful for
> nonconstant text.
>
> I think there are issues here that we need to take a step back and think
> about.  Right now, thanks to the lack of documentation, we can probably
> assume there are approximately zero users of the xslt_process parameter
> feature.  Once we document it that'll no longer be true.  So right now
> would be the time to reflect on whether this is a specification we
> actually like or believe is usable; it'll be too late to change it
> later.
>

I know about one important user from Czech Republic

> There are two specific points bothering me now that I see how it works:
>
> 1. name = value pretty much sucks, especially with the 100% lack of any
> quoting convention for either equals or comma.  I concur with the
> thoughts upthread that turning this into a variadic function would be a
> more sensible solution.

I'll propose a new kind of functions (only position parameter's
function). My idea is simple - for functions with this mark the mixed
and named notation is blocked. But these functions can have a
parameter names - and these names can be passed to function. So there
is possible have a

xslt_process function with current behave - third argument has not
label, and new variadic version like

xslt_process(..,.., param_name1 = 'v1', param_name2 = 'v2',
param_name3 = 'v3', ...)

an implementation of this functionality can be very simple, and we can
use this for xslt_process in 9.1

Regards

Pavel Stehule

>
> 2. I'm not sure whether we ought to auto-single-quote the values.
> If we don't, how hard is it for users to properly quote nonconstant
> parameter values?  (Will quote_literal work, or are the quoting rules
> different for libxslt?)  If we do, are we giving up functionality
> someone cares about?
>
>                        regards, tom lane
>

On Fri, Aug 6, 2010 at 1:46 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I'll propose a new kind of functions (only position parameter's
> function). My idea is simple - for functions with this mark the mixed
> and named notation is blocked. But these functions can have a
> parameter names - and these names can be passed to function. So there
> is possible have a
>
> xslt_process function with current behave - third argument has not
> label, and new variadic version like
>
> xslt_process(..,.., param_name1 = 'v1', param_name2 = 'v2',
> param_name3 = 'v3', ...)
>
> an implementation of this functionality can be very simple, and we can
> use this for xslt_process in 9.1

Why wouldn't we just pass two text arrays to this function and be done
with it? Custom syntax is all well and good when you're writing these
calls by hand, but it's not hard to imagine someone wanting to pass in
values stored somewhere else.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2010/8/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> I think there are issues here that we need to take a step back and think
>> about. Right now, thanks to the lack of documentation, we can probably
>> assume there are approximately zero users of the xslt_process parameter
>> feature. Once we document it that'll no longer be true. So right now
>> would be the time to reflect on whether this is a specification we
>> actually like or believe is usable; it'll be too late to change it
>> later.

> I know about one important user from Czech Republic

Well, if there actually is anybody who's figured it out, we could easily
have a backwards-compatible mode. Provide one variadic function that
acts as follows:
even number of variadic array elements -> they're names/values
one variadic array element -> parse it the old way
otherwise -> error

I wouldn't even bother with fixing the MAXPARAMS limitation for the
"old way" code, just make it work exactly as before.

> I'll propose a new kind of functions (only position parameter's
> function). My idea is simple - for functions with this mark the mixed
> and named notation is blocked.

We don't need random new function behaviors for this. Anyway your
proposal doesn't work at all for non-constant parameter names.

regards, tom lane

2010/8/6 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Aug 6, 2010 at 1:46 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I'll propose a new kind of functions (only position parameter's
>> function). My idea is simple - for functions with this mark the mixed
>> and named notation is blocked. But these functions can have a
>> parameter names - and these names can be passed to function. So there
>> is possible have a
>>
>> xslt_process function with current behave - third argument has not
>> label, and new variadic version like
>>
>> xslt_process(..,.., param_name1 = 'v1', param_name2 = 'v2',
>> param_name3 = 'v3', ...)
>>
>> an implementation of this functionality can be very simple, and we can
>> use this for xslt_process in 9.1
>
> Why wouldn't we just pass two text arrays to this function and be done
> with it?  Custom syntax is all well and good when you're writing these
> calls by hand, but it's not hard to imagine someone wanting to pass in
> values stored somewhere else.

yes, it is possible too. And maybe is better then current
xslt_process. But it isn't too much readable and robust. You have to
calculate position of name and position of value. This is same in
other languages. You can use a dynamic parameters in PHP or Perl via
two arrays, but nobody use it. People use a hash syntax (param1=>val,
param2=>val). This proposal isn't only for xslt_process function. Why
hstore has a custom parser? It can use only a pair of arrays too.

For Tom: proposed syntax can be used generally - everywhere when you
are working with collection. It can be used for hash (hstore)
constructor for example. For me is more readable code like

select hstore(name := 'Tomas', surname := 'Novak')

than

select hstore('name=\'Tomas\', surname=\'Novak\'')

The main advance of this feature is simplicity of custom functions.
Its must not have a custom parser. So possible an using is hstore,
xslt_process

Regards

Pavel Stehule

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Why wouldn't we just pass two text arrays to this function and be done
> with it?

That would work too, although I think it might be a bit harder to use
than one alternating-name-and-value array, at least in some scenarios.
You'd have to be careful that you got the values in the same order in
both arrays, which'd be easy to botch.

There might be other use-cases where two separate arrays are easier
to use, but I'm not seeing one offhand.

regards, tom lane

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> For Tom: proposed syntax can be used generally - everywhere when you
> are working with collection. It can be used for hash (hstore)
> constructor for example. For me is more readable code like

> select hstore(name := 'Tomas', surname := 'Novak')

You've tried to sell us on that before, with few takers. This proposed
use-case impresses me even less than the previous ones, because callers
of xslt_process seem quite likely to need to work with non-constant
parameter names.

In any case, given what we have at the moment for function overload
resolution rules, I think it's a fundamentally bad idea to introduce
a "wild card" function type that would necessarily conflict with
practically every other possible function declaration. So regardless
of what use-cases you propose, I'm going to vote against that.

regards, tom lane

2010/8/6 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> For Tom: proposed syntax can be used generally - everywhere when you
>> are working with collection. It can be used for hash (hstore)
>> constructor for example. For me is more readable code like
>
>> select hstore(name := 'Tomas', surname := 'Novak')
>
> You've tried to sell us on that before, with few takers.  This proposed
> use-case impresses me even less than the previous ones, because callers
> of xslt_process seem quite likely to need to work with non-constant
> parameter names.
>
> In any case, given what we have at the moment for function overload
> resolution rules, I think it's a fundamentally bad idea to introduce
> a "wild card" function type that would necessarily conflict with
> practically every other possible function declaration.  So regardless
> of what use-cases you propose, I'm going to vote against that.

It must not be a function. Just I missing any tool that helps with
complex structured data. This proposed kind functions has one
advantage - there isn't necessary any change in parser. Yes, I can use
a pair of arrays, I can use a one array with seq name, value, I can
use a custom parser. But nothing from these offers a comfort or
readability for example a Perl's hash tables.

so if we have a integrated hash tables, then we can to write some like

CREATE FUNCTION xslt_process(..,.., text{})

select xslt_process(..,.., { par1 => val1, par2 => val3, .. } )

any simple method can significantly help for us, who write a lot of
complex stored procedures. It can be a big help. I am only
dissatisfied because it is "Perlism" - maybe I don't understand SQL
well, but my personal opinion about the most natural syntax for this
situation is some like SQL/XML - xmlattributes or named notation. SQL
isn't too much consistent too - it uses two semantically similar
syntax

foo(name=>value, ..) versus foo(value AS name, ..)

Next my idea was mix of named parameters and marked variadic parameter:

CREATE FUNCTION xslt_process(..,.., VARIADIC LABELED text[])

and then we can call SELECT xslt_process(..,.., par1 := var, ..)

This doesn't introduce a heavy new syntax - just use old in some
specific context. It is only my feeling, so this way is more SQL
natural than using a some hash data type. Maybe you don't think, so
stored procedures must not to have a this functionality. Everywhere
where you can to wrap a c library for SQL you have to meet this task -
often you have to pass a set of flags, collection of values. With
default parameter values situation is better then before, but its
still not ideal - for example SOAP interface or dblink to Oracle

SELECT exec(db, 'SELECT * FROM foo where c = :name', name => 'Pavel')

so this is my motivation - the possibility to write generic custom
functions. Sure - this is "redundant" to existing functionality. I can
write:

SELECT exec(db, 'SELECT * FROM ...', ARRAY['name'], ARRAY['Pavel']) --
Robert'syntax
or your syntax:
SELECT exec(db, 'SELECT * FROM ...', ARRAY['name','Pavel']) or

like current xml2 syntax:
SELECT exec(db, 'SELECT * FROM ...', 'name="Pavel")

But these versions are "too simple" if you understand me. It doesn't
do life of SQL stored procedure's programmer simper.

Regards

Pavel

p.s. sorry for offtopic

p.s. for me isn't important notation. Just I would to like more things
with custom functions withou parser modification.

>
>                        regards, tom lane
>

On Aug 6, 2010, at 11:13 AM, Tom Lane wrote:

> That would work too, although I think it might be a bit harder to use
> than one alternating-name-and-value array, at least in some scenarios.
> You'd have to be careful that you got the values in the same order in
> both arrays, which'd be easy to botch.
>
> There might be other use-cases where two separate arrays are easier
> to use, but I'm not seeing one offhand.

Stuff like this makes me wish PostgreSQL had an ordered pair data type. Then you'd just have a function with `variadic ordered pair` as the signature.

I don't suppose anyone has implemented a data type like this…

Best,

David

On fre, 2010-08-06 at 13:01 -0400, Tom Lane wrote:
> 2. I'm not sure whether we ought to auto-single-quote the values.
> If we don't, how hard is it for users to properly quote nonconstant
> parameter values? (Will quote_literal work, or are the quoting rules
> different for libxslt?) If we do, are we giving up functionality
> someone cares about?

Not every parameter is a string.

Compare xsltproc:

--param PARAMNAME PARAMVALUE
Pass a parameter of name PARAMNAME and value PARAMVALUE to the
stylesheet. You may pass multiple name/value pairs up to a
maximum of 32. If the value being passed is a string, you can use
--stringparam instead, to avoid additional quote characters
that appear in string expressions. Note: the XPath expression
must be UTF-8 encoded.

--stringparam PARAMNAME PARAMVALUE
Pass a parameter of name PARAMNAME and value PARAMVALUE where
PARAMVALUE is a string rather than a node identifier. Note:
The string must be UTF-8 encoded.

On fre, 2010-08-06 at 21:31 +0200, Pavel Stehule wrote:
> It must not be a function. Just I missing any tool that helps with
> complex structured data. This proposed kind functions has one
> advantage - there isn't necessary any change in parser. Yes, I can use
> a pair of arrays, I can use a one array with seq name, value, I can
> use a custom parser. But nothing from these offers a comfort or
> readability for example a Perl's hash tables.

Maybe you should just use PL/XSLT. :-)

2010/8/6 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 6, 2010, at 11:13 AM, Tom Lane wrote:
>
>> That would work too, although I think it might be a bit harder to use
>> than one alternating-name-and-value array, at least in some scenarios.
>> You'd have to be careful that you got the values in the same order in
>> both arrays, which'd be easy to botch.
>>
>> There might be other use-cases where two separate arrays are easier
>> to use, but I'm not seeing one offhand.
>
> Stuff like this makes me wish PostgreSQL had an ordered pair data type. Then you'd just have a function with `variadic ordered pair` as the signature.
>

yes it is one a possibility and probably best. The nice of this
variant can be two forms like current variadic does - foo(.., a :=
10, b := 10) or foo(.., variadic ARRAY[(a,10),(b,10)])

> I don't suppose anyone has implemented a data type like this…
>
> Best,
>
> David
>
>

2010/8/6 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> On fre, 2010-08-06 at 21:31 +0200, Pavel Stehule wrote:
>> It must not be a function. Just I missing any tool that helps with
>> complex structured data. This proposed kind functions has one
>> advantage - there isn't necessary any change in parser. Yes, I can use
>> a pair of arrays, I can use a one array with seq name, value, I can
>> use a custom parser. But nothing from these offers a comfort or
>> readability for example a Perl's hash tables.
>
> Maybe you should just use PL/XSLT. :-)
>
:)

> yes it is one a possibility and probably best. The nice of this
> variant can be two forms like current variadic does -  foo(.., a :=
> 10, b := 10) or foo(.., variadic ARRAY[(a,10),(b,10)])
>
>
pardon foo(..., VARIADIC ARRAY[('a', 10), ('b' 10)])

regards

Pavel

On Aug 6, 2010, at 1:49 PM, Pavel Stehule wrote:

> yes it is one a possibility and probably best. The nice of this
> variant can be two forms like current variadic does - foo(.., a :=
> 10, b := 10) or foo(.., variadic ARRAY[(a,10),(b,10)])

I started fiddling and got as far as this:

CREATE TYPE pair AS ( key text, val text );

CREATE OR REPLACE FUNCTION pair(anyelement, anyelement) RETURNS pair
LANGUAGE SQL AS $$
SELECT ROW($1, $2)::pair;
$$;

CREATE OR REPLACE FUNCTION pair(text, text) RETURNS pair
LANGUAGE SQL AS $$
SELECT ROW($1, $2)::pair;
$$;

CREATE OPERATOR ~> (
LEFTARG = anyelement,
RIGHTARG = anyelement,
PROCEDURE = pair
);

CREATE OPERATOR ~> (
LEFTARG = text,
RIGHTARG = text,
PROCEDURE = pair
);

CREATE OR REPLACE FUNCTION foo(variadic pair[]) RETURNS SETOF text
LANGUAGE SQL AS $$
-- SELECT unnest($1)::text
SELECT $1[1].key
UNION SELECT $1[1].val
UNION SELECT $1[2].key
UNION SELECT $1[2].val;
$$;

SELECT foo('this' ~> 'that', 1 ~> 4);

Not bad, I think. I kind of like it. It reminds me how much I hate the % hstore construction operator, though (the new name for =>).

Best,

David

2010/8/6 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 6, 2010, at 1:49 PM, Pavel Stehule wrote:
>
>> yes it is one a possibility and probably best. The nice of this
>> variant can be two forms like current variadic does -  foo(.., a :=
>> 10, b := 10) or foo(.., variadic ARRAY[(a,10),(b,10)])
>
> I started fiddling and got as far as this:
>
>
> CREATE TYPE pair AS ( key text, val text );
>
> CREATE OR REPLACE FUNCTION pair(anyelement, anyelement) RETURNS pair
> LANGUAGE SQL AS $$
>    SELECT ROW($1, $2)::pair;
> $$;
>
> CREATE OR REPLACE FUNCTION pair(text, text) RETURNS pair
> LANGUAGE SQL AS $$
>    SELECT ROW($1, $2)::pair;
> $$;
>
> CREATE OPERATOR ~> (
>        LEFTARG = anyelement,
>        RIGHTARG = anyelement,
>        PROCEDURE = pair
> );
>
> CREATE OPERATOR ~> (
>        LEFTARG = text,
>        RIGHTARG = text,
>        PROCEDURE = pair
> );
>
> CREATE OR REPLACE FUNCTION foo(variadic pair[]) RETURNS SETOF text
> LANGUAGE SQL AS $$
> --    SELECT unnest($1)::text
>    SELECT $1[1].key
>    UNION  SELECT $1[1].val
>    UNION  SELECT $1[2].key
>    UNION  SELECT $1[2].val;
> $$;
>
> SELECT foo('this' ~> 'that', 1 ~> 4);
>
> Not bad, I think. I kind of like it. It reminds me how much I hate the % hstore construction operator, though (the new name for =>).

so there is only small step to proposed feature

SELECT foo(this := 'that', "1" := 4)

there is only one difference (but you cannot implement it now)
* notation for key is same like for sql identifier - why: I would to
clearly identify key and value. When I use a custom operator - like
you did, it depends on implementation what is key, what is value. When
you use a SQL identifier's notation for key, you can't to do a error

Regards

Pavel

>
> Best,
>
> David
>
>
>

On Aug 6, 2010, at 2:12 PM, Pavel Stehule wrote:

>> SELECT foo('this' ~> 'that', 1 ~> 4);
>>
>> Not bad, I think. I kind of like it. It reminds me how much I hate the % hstore construction operator, though (the new name for =>).
>
> so there is only small step to proposed feature
>
> SELECT foo(this := 'that', "1" := 4)
>
> there is only one difference (but you cannot implement it now)
> * notation for key is same like for sql identifier - why: I would to
> clearly identify key and value. When I use a custom operator - like
> you did, it depends on implementation what is key, what is value. When
> you use a SQL identifier's notation for key, you can't to do a error

Sorry, not following you here…

David

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On fre, 2010-08-06 at 13:01 -0400, Tom Lane wrote:
>> 2. I'm not sure whether we ought to auto-single-quote the values.
>> If we don't, how hard is it for users to properly quote nonconstant
>> parameter values? (Will quote_literal work, or are the quoting rules
>> different for libxslt?) If we do, are we giving up functionality
>> someone cares about?

> Not every parameter is a string.

So I gather, but what else is there, and do we actually want to expose
the other alternatives in xslt_process()?

If we don't auto-quote, we need to provide some sort of quote_xslt()
function that will apply the appropriate quoting/escaping to deal
with an arbitrary string value.

regards, tom lane

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Aug 6, 2010, at 2:12 PM, Pavel Stehule wrote:
>> so there is only small step to proposed feature
>> SELECT foo(this := 'that', "1" := 4)

> Sorry, not following you here

Pavel doesn't understand "no" ;-)

regards, tom lane

On Fri, Aug 06, 2010 at 11:48:58PM +0300, Peter Eisentraut wrote:
> On fre, 2010-08-06 at 21:31 +0200, Pavel Stehule wrote:
> > It must not be a function. Just I missing any tool that helps with
> > complex structured data. This proposed kind functions has one
> > advantage - there isn't necessary any change in parser. Yes, I can
> > use a pair of arrays, I can use a one array with seq name, value,
> > I can use a custom parser. But nothing from these offers a comfort
> > or readability for example a Perl's hash tables.
>
> Maybe you should just use PL/XSLT. :-)

When's that going into the tree?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

2010/8/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> On Aug 6, 2010, at 2:12 PM, Pavel Stehule wrote:
>>> so there is only small step to proposed feature
>>> SELECT foo(this := 'that', "1" := 4)
>
>> Sorry, not following you here

I would to difference a key and value in notation.

>
> Pavel doesn't understand "no" ;-)
>

you are don't writing a stored procedures like me - so maybe you are
doesn't understand a my motivation. :). I have to try it. You are
rejected almost of all my proposals - named parameters, variadic
functions, enhancing of RAISE STATEMENT - and now its in core. But it
was a battle :). Try to write a XML-RPC support for PostgreSQL, and
try to thinking on programmer comfort, please. I am sure so our
support for stored procedures or external procedures are not complete
- it is limited by BISON possibilities, and because BISON isn't
extensible parser, I am searching other ways. If I can enhance a
syntax from external module, I don't talk.

Regards

Pavel

Regards

Pavel

>                        regards, tom lane
>

On Aug 6, 2010, at 8:49 PM, Pavel Stehule wrote:

>> Sorry, not following you here
>
> I would to difference a key and value in notation.

That's exactly what my solution does. The array solution doesn't. Whether it's appropriate to use a custom composite type, however, is an open question.

>> Pavel doesn't understand "no" ;-)
>
> you are don't writing a stored procedures like me - so maybe you are
> doesn't understand a my motivation. :). I have to try it. You are
> rejected almost of all my proposals - named parameters, variadic
> functions, enhancing of RAISE STATEMENT - and now its in core. But it
> was a battle :).

This is how most stuff gets in: you fight Tom to exhaustion. It's a slog, but usually the resulting implementation is better than it would otherwise have been.

> Try to write a XML-RPC support for PostgreSQL, and
> try to thinking on programmer comfort, please. I am sure so our
> support for stored procedures or external procedures are not complete
> - it is limited by BISON possibilities, and because BISON isn't
> extensible parser, I am searching other ways. If I can enhance a
> syntax from external module, I don't talk.

I think that some sort of variadic pairs would be useful for this. But since there is no core "ordered pair" data type, I don't think you're going to get too far.

Best,

David

2010/8/7 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 6, 2010, at 8:49 PM, Pavel Stehule wrote:
>
>>> Sorry, not following you here
>>
>> I would to difference a key and value in notation.
>
> That's exactly what my solution does. The array solution doesn't. Whether it's appropriate to use a custom composite type, however, is an open question.

no it doesn't - in your design there are no different notation for key
and for value. Next this design block a '->'. Because it's based on
polymorphic operator. But it can be a one variant - where you would to
put together expr with expr. And you can't do more from user space
now. But if you have a build in operator for (sqlidentifier, any) with
early processing - like "AS" in xml_attributies, we can do it. The
using of this operator can be limited only on function parameter
context.

>
>>> Pavel doesn't understand "no" ;-)
>>
>> you are don't writing a stored procedures like me - so maybe you are
>> doesn't understand a my motivation. :). I have to try it. You are
>> rejected almost of all my proposals - named parameters, variadic
>> functions, enhancing of RAISE STATEMENT - and now its in core. But it
>> was a battle :).
>
> This is how most stuff gets in: you fight Tom to exhaustion. It's a slog, but usually the resulting implementation is better than it would otherwise have been.
>
>> Try to write a XML-RPC support for PostgreSQL, and
>> try to thinking on programmer comfort, please. I am sure so our
>> support for stored procedures or external procedures are not complete
>> - it is limited by BISON possibilities, and because BISON isn't
>> extensible parser, I am searching other ways. If I can enhance a
>> syntax from external module, I don't talk.
>
> I think that some sort of variadic pairs would be useful for this. But since there is no core "ordered pair" data type, I don't think you're going to get too far.

Postgres has a array of rows (Inside C or plperlu can be transofmed to
real hash simply). It just miss a user friendly notation for using it.

Regards

Pavel
>
> Best,
>
> David
>
>

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> I think that some sort of variadic pairs would be useful for this. But since there is no core "ordered pair" data type, I don't think you're going to get too far.

It's not immediately clear to me what an ordered-pair type would get you
that you don't get with 2-element arrays.

A couple of quick experiments suggest that 2-D arrays might be the thing
to use. They're easy to construct:

regression=# select array[[1,2],[3,4]];
array
---------------
{{1,2},{3,4}}
(1 row)

and you can build them dynamically at need:

regression=# select array[[1,2],[3,4]] || array[5,6];
?column?
---------------------
{{1,2},{3,4},{5,6}}
(1 row)

This is not exactly without precedent, either: our built-in xpath()
function appears to use precisely this approach for its namespace-list
argument.

regards, tom lane

On Aug 6, 2010, at 9:48 PM, Pavel Stehule wrote:

>> That's exactly what my solution does. The array solution doesn't. Whether it's appropriate to use a custom composite type, however, is an open question.
>
> no it doesn't - in your design there are no different notation for key
> and for value. Next this design block a '->'. Because it's based on
> polymorphic operator. But it can be a one variant - where you would to
> put together expr with expr. And you can't do more from user space
> now. But if you have a build in operator for (sqlidentifier, any) with
> early processing - like "AS" in xml_attributies, we can do it. The
> using of this operator can be limited only on function parameter
> context.

I'm sorry, I still don't follow. It creates an ordered pair, with one value being named "key" and the other one "val". And when you use the ~> operator, the lhs is the key and the rhs if the value.

> Postgres has a array of rows (Inside C or plperlu can be transofmed to
> real hash simply). It just miss a user friendly notation for using it.

I think Tom's right, frankly: An array of arrays will be the best solution for your interface. Sure someone could include more than two items in each nested array, or fewer than 2, but if there are more you ignore them, and if there are fewer you treat them as NULLs.

Best,

David

2010/8/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> I think that some sort of variadic pairs would be useful for this. But since there is no core "ordered pair" data type, I don't think you're going to get too far.
>
> It's not immediately clear to me what an ordered-pair type would get you
> that you don't get with 2-element arrays.
>
> A couple of quick experiments suggest that 2-D arrays might be the thing
> to use.  They're easy to construct:
>
> regression=# select array[[1,2],[3,4]];
>     array
> ---------------
>  {{1,2},{3,4}}
> (1 row)
>
> and you can build them dynamically at need:
>
> regression=# select array[[1,2],[3,4]] || array[5,6];
>      ?column?
> ---------------------
>  {{1,2},{3,4},{5,6}}
> (1 row)
>
> This is not exactly without precedent, either: our built-in xpath()
> function appears to use precisely this approach for its namespace-list
> argument.

it's one variant, but isn't perfect

a) it expects so key and value are literals
b) it expects so all values has same types - what is usually what we
need, but not necessary
c) isn't too readable - I am sorry so I am repeating - it is same
reason, why people will prefer a VARIADIC function before function
with array - but I can accept, so this is my view of world

Regards

Pavel Stehule

>
>                        regards, tom lane
>

On Aug 6, 2010, at 9:59 PM, Tom Lane wrote:

> It's not immediately clear to me what an ordered-pair type would get you
> that you don't get with 2-element arrays.

Just syntactic sugar, really. And control over how many items you have (a bounded pair rather than an unlimited element array).

> A couple of quick experiments suggest that 2-D arrays might be the thing
> to use. They're easy to construct:
>
> regression=# select array[[1,2],[3,4]];
> array
> ---------------
> {{1,2},{3,4}}
> (1 row)
>
> and you can build them dynamically at need:
>
> regression=# select array[[1,2],[3,4]] || array[5,6];
> ?column?
> ---------------------
> {{1,2},{3,4},{5,6}}
> (1 row)
>
> This is not exactly without precedent, either: our built-in xpath()
> function appears to use precisely this approach for its namespace-list
> argument.

Agreed.

Best,

David

On Aug 6, 2010, at 10:15 PM, Pavel Stehule wrote:

>> This is not exactly without precedent, either: our built-in xpath()
>> function appears to use precisely this approach for its namespace-list
>> argument.
>
> it's one variant, but isn't perfect
>
> a) it expects so key and value are literals

Huh? You can select into an array:

try=# create table foo(k text, v text);
CREATE TABLE
try=# insert into foo values ('foo', 'bar'), ('baz', 'yow');
INSERT 0 2
try=# select ARRAY[k,v] FROM foo;
array
-----------
{foo,bar}
{baz,yow}
(2 rows)

try=# select ARRAY(SELECT ARRAY[k,v] FROM foo);
ERROR: could not find array type for datatype text[]

Okay, so nested arrays are harder.

> b) it expects so all values has same types - what is usually what we
> need, but not necessary

The XML interface converts them all to text anyway, no?

> c) isn't too readable - I am sorry so I am repeating - it is same
> reason, why people will prefer a VARIADIC function before function
> with array - but I can accept, so this is my view of world

I agree that it's not as sugary as pairs would be. But I admit to having no problem with

SELECT foo(ARRAY[ ['foo', 'bar'], ['baz', 'yow']]);

But maybe I'm biased, since there's a lot of that sort of syntax in pgTAP.

Best,

David

2010/8/7 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 6, 2010, at 9:48 PM, Pavel Stehule wrote:
>
>>> That's exactly what my solution does. The array solution doesn't. Whether it's appropriate to use a custom composite type, however, is an open question.
>>
>> no it doesn't - in your design there are no different notation for key
>> and for value. Next this design block  a '->'. Because it's based on
>> polymorphic operator. But it can be a one variant - where you would to
>> put together expr with expr. And you can't do more from user space
>> now. But if you have a build in operator for (sqlidentifier, any) with
>> early processing - like "AS" in xml_attributies, we can do it. The
>> using of this operator can be limited only on function parameter
>> context.
>
> I'm sorry, I still don't follow. It creates an ordered pair, with one value being named "key" and the other one "val". And when you use the ~> operator, the lhs is the key and the rhs if the value.
>
>> Postgres has a array of rows (Inside C or plperlu can be transofmed to
>> real hash simply). It just miss a user friendly notation for using it.
>
> I think Tom's right, frankly: An array of arrays will be the best solution for your interface. Sure someone could include more than two items in each nested array, or fewer than 2, but if there are more you ignore them, and if there are fewer you treat them as NULLs.

it can be a just plain text - it isn't about functionality, it is
about readability, verbosity and stored procedures developer's comfort
- and some consistency.

Pavel

>
> Best,
>
> David
>
>
>

2010/8/7 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 6, 2010, at 10:15 PM, Pavel Stehule wrote:
>
>>> This is not exactly without precedent, either: our built-in xpath()
>>> function appears to use precisely this approach for its namespace-list
>>> argument.
>>
>> it's one variant, but isn't perfect
>>
>> a) it expects so key and value are literals
>
> Huh? You can select into an array:

and pg doesn't handle 2D arrays well - can't to use ARRAY(subselect)
constructor for 2D arrays

>
> try=# create table foo(k text, v text);
> CREATE TABLE
> try=# insert into foo values ('foo', 'bar'), ('baz', 'yow');
> INSERT 0 2
> try=# select ARRAY[k,v] FROM foo;
>   array
> -----------
>  {foo,bar}
>  {baz,yow}
> (2 rows)
>
> try=# select ARRAY(SELECT ARRAY[k,v] FROM foo);
> ERROR:  could not find array type for datatype text[]

try SELECT ARRAY(SELECT row(k,v) FROM foo)

>
> Okay, so nested arrays are harder.
>
>> b) it expects so all values has same types - what is usually what we
>> need, but not necessary
>
> The XML interface converts them all to text anyway, no?
>

sure, but it isn't relevant here - the problem is buildin output
functions for datatypes. For example - true is different formated in
PostgresSQL and different formated in xml or JSON. Date values are
differently formated in JSON and XML. So if you would to correctly
format some date type value and if your interface is only text - then
you have to cast value back to binary and format it again. More - if
you have a information about original data type, you can use a corect
format. So if you use a only text parameters, then you lost a
significant information (when some parameter are not text). For
example, if I have only text interface for some hypothetical JSON API,
then I am not able to show a boolean value correctly - because it
doesn't use a quoting - and it is string and isn't number.

There is some other issue - PLpgSQL can't to work well with untyped
collections. But it isn't problem for C custom functions, and there
are not any reason why we can't to support polymorphic collections
(+/- because these collection cannot be accessed from PLpgSQL
directly).

>> c) isn't too readable - I am sorry so I am repeating - it is same
>> reason, why people will prefer a VARIADIC function before function
>> with array - but I can accept, so this is my view of world
>
> I agree that it's not as sugary as pairs would be. But I admit to having no problem with
>
>  SELECT foo(ARRAY[ ['foo', 'bar'], ['baz', 'yow']]);
>
> But maybe I'm biased, since there's a lot of that sort of syntax in pgTAP.
>

Yes, when you are a author of code, you know what you are wrote. But
when you have do some review? Then an reviewer have to look on
definition of foo, and he has to verify, if you are use a parameters
well. For two params I don't see on first view what system you used -
[[key,key],[value,value]] or [[key,value],[key, value]]. More you have
to use a nested data structure - what is less readable then variadic
parameters. And - in pg - you are lost information about original data
types.

Regards

Pavel

> Best,
>
> David
>
>
>

On Aug 6, 2010, at 10:49 PM, Pavel Stehule wrote:

>> Huh? You can select into an array:
>
> and pg doesn't handle 2D arrays well - can't to use ARRAY(subselect)
> constructor for 2D arrays

Right.

>> try=# select ARRAY(SELECT ARRAY[k,v] FROM foo);
>> ERROR: could not find array type for datatype text[]
>
> try SELECT ARRAY(SELECT row(k,v) FROM foo)

Yeah, but those aren't nested arrays., They're…well, they're ordered pairs. ;-P

> sure, but it isn't relevant here - the problem is buildin output
> functions for datatypes. For example - true is different formated in
> PostgresSQL and different formated in xml or JSON. Date values are
> differently formated in JSON and XML. So if you would to correctly
> format some date type value and if your interface is only text - then
> you have to cast value back to binary and format it again. More - if
> you have a information about original data type, you can use a corect
> format. So if you use a only text parameters, then you lost a
> significant information (when some parameter are not text). For
> example, if I have only text interface for some hypothetical JSON API,
> then I am not able to show a boolean value correctly - because it
> doesn't use a quoting - and it is string and isn't number.

Point. FWIW, though, this is already an issue for non-SQL functions. PL/Perl, for example, gets all arguments cast to text, AFAICT:

try=# create or replace function try(bool) returns text language plperl AS 'shift';
CREATE FUNCTION
Time: 121.403 ms
try=# select try(true);
try
-----
t
(1 row)

I wish this wasn't so.

> There is some other issue - PLpgSQL can't to work well with untyped
> collections. But it isn't problem for C custom functions, and there
> are not any reason why we can't to support polymorphic collections
> (+/- because these collection cannot be accessed from PLpgSQL
> directly).

I completely agree with you here. I'd love to be able to support RECORD arguments to non-C functions.

>> I agree that it's not as sugary as pairs would be. But I admit to having no problem with
>>
>> SELECT foo(ARRAY[ ['foo', 'bar'], ['baz', 'yow']]);
>>
>> But maybe I'm biased, since there's a lot of that sort of syntax in pgTAP..
>>
>
> Yes, when you are a author of code, you know what you are wrote. But
> when you have do some review? Then an reviewer have to look on
> definition of foo, and he has to verify, if you are use a parameters
> well. For two params I don't see on first view what system you used -
> [[key,key],[value,value]] or [[key,value],[key, value]]. More you have
> to use a nested data structure - what is less readable then variadic
> parameters. And - in pg - you are lost information about original data
> types.

Valid points. I agree that it would be nicer to use RECORDs:

SELECT foo( row('foo', 1), row('bar', true));

Certainly much clearer. But given that we've gone round and round on allowing non-C functions to use ROWs and gotten nowhere, I don't know that we'll get any further now. But can you not create a C function that allows a signature of VARIADIC RECORD?

Best,

David

2010/8/7 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 6, 2010, at 10:49 PM, Pavel Stehule wrote:
>
>>> Huh? You can select into an array:
>>
>> and pg doesn't handle 2D arrays well - can't to use ARRAY(subselect)
>> constructor for 2D arrays
>
> Right.
>
>>> try=# select ARRAY(SELECT ARRAY[k,v] FROM foo);
>>> ERROR:  could not find array type for datatype text[]
>>
>> try SELECT ARRAY(SELECT row(k,v) FROM foo)
>
> Yeah, but those aren't nested arrays., They're…well, they're ordered pairs. ;-P
>
>> sure, but it isn't relevant here - the problem is buildin output
>> functions for datatypes. For example - true is different formated in
>> PostgresSQL and different formated in xml or JSON. Date values are
>> differently formated in JSON and XML. So if you would to correctly
>> format some date type value and if your interface is only text - then
>> you have to cast value back to binary and format it again. More - if
>> you have a information about original data type, you can use a corect
>> format. So if you use a only text parameters, then you lost a
>> significant information (when some parameter are not text). For
>> example, if I have only text interface for some hypothetical JSON API,
>> then I am not able to show a boolean value correctly - because it
>> doesn't use a quoting - and it is string and isn't number.
>
> Point. FWIW, though, this is already an issue for non-SQL functions. PL/Perl, for example, gets all arguments cast to text, AFAICT:
>
> try=# create or replace function try(bool) returns text language plperl AS 'shift';
> CREATE FUNCTION
> Time: 121.403 ms
> try=# select try(true);
>  try
> -----
>  t
> (1 row)
>
> I wish this wasn't so.
>

It must not be - it depends on PL handler implementation. PostgreSQL
call PL handler with binary values. I am thinking so new Python PL can
do it well.

>> There is some other issue - PLpgSQL can't to work well with untyped
>> collections. But it isn't problem for C custom functions, and there
>> are not any reason why we can't to support polymorphic collections
>> (+/- because these collection cannot be accessed from PLpgSQL
>> directly).
>
> I completely agree with you here. I'd love to be able to support RECORD arguments to non-C functions.
>
>>> I agree that it's not as sugary as pairs would be. But I admit to having no problem with
>>>
>>>  SELECT foo(ARRAY[ ['foo', 'bar'], ['baz', 'yow']]);
>>>
>>> But maybe I'm biased, since there's a lot of that sort of syntax in pgTAP..
>>>
>>
>> Yes, when you are a author of code, you know what you are wrote. But
>> when you have do some review? Then an reviewer have to look on
>> definition of foo, and he has to verify, if you are use a parameters
>> well. For two params I don't see on first view what system you used -
>> [[key,key],[value,value]] or [[key,value],[key, value]]. More you have
>> to use a nested data structure - what is less readable then variadic
>> parameters. And - in pg - you are lost information about original data
>> types.
>
> Valid points. I agree that it would be nicer to use RECORDs:
>
>    SELECT foo( row('foo', 1), row('bar', true));

I am not absolutly satisfied - but it's better, than arrays.

>
> Certainly much clearer. But given that we've gone round and round on allowing non-C functions to use ROWs and gotten nowhere, I don't know that we'll get any further now. But can you not create a C function that allows a signature of VARIADIC RECORD?

you can do a variadic over ROW type. We have not a polymorphic arrays
- so isn't possible to write VARIADIC RECORD now. It could be a nice
if we are to define a own composite types with polymorphic fields.
Then you can do:

CREATE TYPE pair AS (key text, value "any");
CREATE FUNCTION foo(VARIADIC pair[])

other idea is leave arrays - and thinking about key, value collection
as new kind of data types. so maybe

CREATE FUNCTION foo(VARIADIC params COLECTION OF text WITH UNIQUE text KEY)

Regards

Pavel

>
> Best,
>
> David
>
>
>
>
>
>
>
>
>
>
>

On Aug 7, 2010, at 12:24 AM, Pavel Stehule wrote:

>> try=# create or replace function try(bool) returns text language plperl AS 'shift';
>> CREATE FUNCTION
>> Time: 121.403 ms
>> try=# select try(true);
>> try
>> -----
>> t
>> (1 row)
>>
>> I wish this wasn't so.
>>
>
> It must not be - it depends on PL handler implementation. PostgreSQL
> call PL handler with binary values. I am thinking so new Python PL can
> do it well.

I'm thinking an update to PL/Perl would be useful. Frankly, I'd most like to see proper array support. But that's another topic.

>> Valid points. I agree that it would be nicer to use RECORDs:
>>
>> SELECT foo( row('foo', 1), row('bar', true));
>
> I am not absolutly satisfied - but it's better, than arrays.

>> Certainly much clearer. But given that we've gone round and round on allowing non-C functions to use ROWs and gotten nowhere, I don't know that we'll get any further now. But can you not create a C function that allows a signature of VARIADIC RECORD?
>
> you can do a variadic over ROW type. We have not a polymorphic arrays
> - so isn't possible to write VARIADIC RECORD now.

Ah, right. I guess table types can't be cast to RECORD?

> It could be a nice
> if we are to define a own composite types with polymorphic fields.
> Then you can do:
>
> CREATE TYPE pair AS (key text, value "any");
> CREATE FUNCTION foo(VARIADIC pair[])

Yes.

> other idea is leave arrays - and thinking about key, value collection
> as new kind of data types. so maybe
>
> CREATE FUNCTION foo(VARIADIC params COLECTION OF text WITH UNIQUE text KEY)

COLLECTION?

David

2010/8/8 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 7, 2010, at 12:24 AM, Pavel Stehule wrote:
>
>>> try=# create or replace function try(bool) returns text language plperl AS 'shift';
>>> CREATE FUNCTION
>>> Time: 121.403 ms
>>> try=# select try(true);
>>>  try
>>> -----
>>>  t
>>> (1 row)
>>>
>>> I wish this wasn't so.
>>>
>>
>> It must not be - it depends on PL handler implementation. PostgreSQL
>> call PL handler with binary values. I am thinking so new Python PL can
>> do it well.
>
> I'm thinking an update to PL/Perl would be useful. Frankly, I'd most like to see proper array support. But that's another topic.
>
>>> Valid points. I agree that it would be nicer to use RECORDs:
>>>
>>>    SELECT foo( row('foo', 1), row('bar', true));
>>
>> I am not absolutly satisfied - but it's better, than arrays.
>
>>> Certainly much clearer. But given that we've gone round and round on allowing non-C functions to use ROWs and gotten nowhere, I don't know that we'll get any further now. But can you not create a C function that allows a signature of VARIADIC RECORD?
>>
>> you can do a variadic over ROW type. We have not a polymorphic arrays
>> - so isn't possible to write VARIADIC RECORD now.
>
> Ah, right. I guess table types can't be cast to RECORD?
>
>> It could be a nice
>> if we are to define a own composite types with polymorphic fields.
>> Then you can do:
>>
>> CREATE TYPE pair AS (key text, value "any");
>> CREATE FUNCTION foo(VARIADIC pair[])
>
> Yes.
>
>> other idea is leave arrays - and thinking about key, value collection
>> as new kind of data types. so maybe
>>
>> CREATE FUNCTION foo(VARIADIC params COLECTION OF text WITH UNIQUE text KEY)
>
> COLLECTION?

yes, sorry - simply - class where fields can be accessed via specified
index - unique or not unique.

>
> David
>
>

On 06/08/10 17:50, Pavel Stehule wrote:
>
> attached updated patch with regression test
>

>

Bravely ignoring the quotation/varidic/<favourite_scheme_here>
conversations, I've taken a look at the patch as is. Thanks to Tom's
input I can now correctly drive the function. I can also report that
code is now behaving in the expected way.

I have two other observations, more directed at the community than Pavel:

1) XML2 is largely undocumented, giving rise to the problems
encountered. Since the module is deprecated anyways, does it make more
sense to get xslt handling moved into core and get it fully documented?

2) Pavel's regression test exposes a bug in libxslt. The stylesheet
declares 5 parameters, but uses 12. Simplifying, take the stylesheet:

<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
version="1.0">
<xsl:output method="xml" omit-xml-declaration="yes" indent="yes"/>
<xsl:strip-space elements="*"/>
<xsl:param name="n1"/>
<xsl:template match="*">
<xsl:element name="samples">
<xsl:element name="sample">
<xsl:value-of select="$n1"/>
</xsl:element>
<xsl:element name="sample">
<xsl:value-of select="$n2"/>
</xsl:element>
</xsl:element>
</xsl:template>
</xsl:stylesheet>

and run the command:

~/temp$ xsltproc --stringparam n2 "v2" Untitled2.xsl Untitled1.xml
<samples>
<sample/>
<sample>v2</sample>
</samples>

All looks good. However if you run:

~/temp$ xsltproc --stringparam n1 "v1" Untitled2.xsl Untitled1.xml
runtime error: file Untitled2.xsl line 28 element value-of
Variable 'n2' has not been declared.
xmlXPathCompiledEval: evaluation failed
runtime error: file Untitled2.xsl line 28 element value-of
XPath evaluation returned no result.
<samples>
<sample>v1</sample>
<sample/>
</samples>

The xslt_process function ignores these errors and returns cleanly.

To summarize, the bug in libxslt is that it allows the processing of
unnamed parameters - most other parsers would reject this stylesheet.
Secondly, the xslt_process does not return the errors reported when
running without passing the unnamed parameter.

Personally I would like to see this documented and moved to core so that
the whole of xml2 can be dropped. I also think that the errors should be
reported, even if libxslt doesn't behave properly in all scenarios.

Of course there's that whole other issue around how you pass the
parameters in the first place that needs resolving too...

Regards,
--
Mike Fowler
Registered Linux user: 379787

On Aug 7, 2010, at 11:05 PM, Pavel Stehule wrote:

>> COLLECTION?
>
> yes, sorry - simply - class where fields can be accessed via specified
> index - unique or not unique.

Like in Oracle? From: http://download.oracle.com/docs/cd/B19306_01/appdev.102/b14261/collections.htm

> A collection is an ordered group of elements, all of the same type. It is a general concept that encompasses lists, arrays, and other datatypes used in classic programming algorithms. Each element is addressed by a unique subscript.

There are no keys.

Best,

David

2010/8/8 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 7, 2010, at 11:05 PM, Pavel Stehule wrote:
>
>>> COLLECTION?
>>
>> yes, sorry - simply - class where fields can be accessed via specified
>> index - unique or not unique.
>
> Like in Oracle? From: http://download.oracle.com/docs/cd/B19306_01/appdev.102/b14261/collections.htm

yes, can be some like it. But I would to allow a non unique indexes

>
>> A collection is an ordered group of elements, all of the same type. It is a general concept that encompasses lists, arrays, and other datatypes used in classic programming algorithms. Each element is addressed by a unique subscript.
>
> There are no keys.

ok - I didn't use a correct name - so "indexed set" is better.

>
> Best,
>
> David
>
>

On Aug 8, 2010, at 9:10 AM, Pavel Stehule wrote:

>> There are no keys.
>
> ok - I didn't use a correct name - so "indexed set" is better.

Hash?

David

2010/8/8 David E. Wheeler <david(at)kineticode(dot)com>:
> On Aug 8, 2010, at 9:10 AM, Pavel Stehule wrote:
>
>>> There are no keys.
>>
>> ok - I didn't use a correct name - so "indexed set" is better.
>
> Hash?

Just only hash isn't good model, because I sometimes we would prefer a
ordered set

Regards

Pavel

>
> David
>
>

On Sun, Aug 8, 2010 at 11:36 AM, Mike Fowler <mike(at)mlfowler(dot)com> wrote:
> On 06/08/10 17:50, Pavel Stehule wrote:
>> attached updated patch with regression test
>
> Bravely ignoring the quotation/varidic/<favourite_scheme_here>
> conversations,

Thank you!

> I've taken a look at the patch as is.

Excellent.

> Thanks to Tom's input I
> can now correctly drive the function. I can also report that code is now
> behaving in the expected way.
>
> I have two other observations, more directed at the community than Pavel:
>
> 1) XML2 is largely undocumented, giving rise to the problems encountered.
> Since the module is deprecated anyways, does it make more sense to get xslt
> handling moved into core and get it fully documented?

Yes, I think that would be better.

> 2) Pavel's regression test exposes a bug in libxslt. The stylesheet declares
> 5 parameters, but uses 12. Simplifying, take the stylesheet:

I'm not sure whether there's anything we can do about this.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Aug 8, 2010 at 11:36 AM, Mike Fowler <mike(at)mlfowler(dot)com> wrote:
>> 1) XML2 is largely undocumented, giving rise to the problems encountered.
>> Since the module is deprecated anyways, does it make more sense to get xslt
>> handling moved into core and get it fully documented?

> Yes, I think that would be better.

I'm hesitant to consider pulling this into core when there's so little
consensus on how it ought to act. It'd be better to have a solid,
widely used contrib module *first*, rather than imagine that pulling it
into core is somehow a cure for its problems.

>> 2) Pavel's regression test exposes a bug in libxslt. The stylesheet declares
>> 5 parameters, but uses 12. Simplifying, take the stylesheet:

> I'm not sure whether there's anything we can do about this.

We should file a bug report with the libxslt authors, obviously.

regards, tom lane

On 09/08/10 04:07, Tom Lane wrote:
> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>> On Sun, Aug 8, 2010 at 11:36 AM, Mike Fowler<mike(at)mlfowler(dot)com> wrote:
>>> 1) XML2 is largely undocumented, giving rise to the problems encountered.
>>> Since the module is deprecated anyways, does it make more sense to get xslt
>>> handling moved into core and get it fully documented?
>
>> Yes, I think that would be better.
>
> I'm hesitant to consider pulling this into core when there's so little
> consensus on how it ought to act. It'd be better to have a solid,
> widely used contrib module *first*, rather than imagine that pulling it
> into core is somehow a cure for its problems.

Perhaps the first step forward is to pull xslt_process out of xml2 and
create a standalone xslt contrib module. Then at least it can lose the
stigma of being in a deprecated module and perhaps make it more visible
to users.

>
>>> 2) Pavel's regression test exposes a bug in libxslt. The stylesheet declares
>>> 5 parameters, but uses 12. Simplifying, take the stylesheet:
>
>> I'm not sure whether there's anything we can do about this.
>
> We should file a bug report with the libxslt authors, obviously.

Turns out the bug was filed in 2005 (see
https://bugzilla.gnome.org/show_bug.cgi?id=307061). They are currently
taking a fairly loose interpretation of the XSLT spec. However that was
only one aspect of the concern. The other was that no errors were being
reported back in psql when the libxslt is generating errors. Is this
desirable?

Regards,
--
Mike Fowler
Registered Linux user: 379787

Mike Fowler <mike(at)mlfowler(dot)com> writes:
> Turns out the bug was filed in 2005 (see
> https://bugzilla.gnome.org/show_bug.cgi?id=307061). They are currently
> taking a fairly loose interpretation of the XSLT spec. However that was
> only one aspect of the concern. The other was that no errors were being
> reported back in psql when the libxslt is generating errors. Is this
> desirable?

Uh, no; if we're failing to detect an error that the library does
report, that's our bug (and another indication of the immaturity
of this code :-()).

regards, tom lane

On Mon, Aug 9, 2010 at 10:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Mike Fowler <mike(at)mlfowler(dot)com> writes:
>> Turns out the bug was filed in 2005 (see
>> https://bugzilla.gnome.org/show_bug.cgi?id=307061). They are currently
>> taking a fairly loose interpretation of the XSLT spec. However that was
>> only one aspect of the concern. The other was that no errors were being
>> reported back in psql when the libxslt is generating errors. Is this
>> desirable?
>
> Uh, no; if we're failing to detect an error that the library does
> report, that's our bug (and another indication of the immaturity
> of this code :-()).

Right. So, what about Mike's idea of extracting this into a new
contrib module, perhaps contrib/xslt? That might also provide a good
excuse to jettison any details of the existing interfaces that we
happen to find unfortunate.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Right. So, what about Mike's idea of extracting this into a new
> contrib module, perhaps contrib/xslt? That might also provide a good
> excuse to jettison any details of the existing interfaces that we
> happen to find unfortunate.

Seems like mostly make-work to me --- we could just as easily fix the
code where it sits. But if you're excited about it, I won't stand in
the way.

regards, tom lane

Mike Fowler <mike(at)mlfowler(dot)com> writes:
> On 06/08/10 17:50, Pavel Stehule wrote:
>> attached updated patch with regression test

> Bravely ignoring the quotation/varidic/<favourite_scheme_here>
> conversations, I've taken a look at the patch as is. Thanks to Tom's
> input I can now correctly drive the function. I can also report that
> code is now behaving in the expected way.

I've gone ahead and applied this patch, since the subsequent discussion
seemed to be getting *extremely* far afield from the expressed intent
of the patch, and nobody had pointed out a reason not to fix the
number-of-parameters limitation.

I think we have a few TODO items here:

* Invent ... and document ... an API that permits safe assembly of a
parameter list from non-constant (and perhaps untrustworthy) values.

* Fix xslt_process' failure to report (some?) errors detected by libxslt.

* Move the functionality to a less deprecated place.

None of these are within the scope of the current patch though.

regards, tom lane

Tom Lane wrote:
> Mike Fowler <mike(at)mlfowler(dot)com> writes:
> > On 06/08/10 17:50, Pavel Stehule wrote:
> >> attached updated patch with regression test
>
> > Bravely ignoring the quotation/varidic/<favourite_scheme_here>
> > conversations, I've taken a look at the patch as is. Thanks to Tom's
> > input I can now correctly drive the function. I can also report that
> > code is now behaving in the expected way.
>
> I've gone ahead and applied this patch, since the subsequent discussion
> seemed to be getting *extremely* far afield from the expressed intent
> of the patch, and nobody had pointed out a reason not to fix the
> number-of-parameters limitation.
>
> I think we have a few TODO items here:
>
> * Invent ... and document ... an API that permits safe assembly of a
> parameter list from non-constant (and perhaps untrustworthy) values.
>
> * Fix xslt_process' failure to report (some?) errors detected by libxslt.
>
> * Move the functionality to a less deprecated place.
>
> None of these are within the scope of the current patch though.

Should any of these be added to our TODO list under XML?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> I think we have a few TODO items here:
>>
>> * Invent ... and document ... an API that permits safe assembly of a
>> parameter list from non-constant (and perhaps untrustworthy) values.
>>
>> * Fix xslt_process' failure to report (some?) errors detected by libxslt.
>>
>> * Move the functionality to a less deprecated place.
>>
>> None of these are within the scope of the current patch though.

> Should any of these be added to our TODO list under XML?

Yes, all of them, since nothing's been done about any of 'em ...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Tom Lane wrote:
> >> I think we have a few TODO items here:
> >>
> >> * Invent ... and document ... an API that permits safe assembly of a
> >> parameter list from non-constant (and perhaps untrustworthy) values.
> >>
> >> * Fix xslt_process' failure to report (some?) errors detected by libxslt.
> >>
> >> * Move the functionality to a less deprecated place.
> >>
> >> None of these are within the scope of the current patch though.
>
> > Should any of these be added to our TODO list under XML?
>
> Yes, all of them, since nothing's been done about any of 'em ...

OK, TODO items added:

Move XSLT from contrib/xml2 to a more reasonable location

* http://archives.postgresql.org/pgsql-hackers/2010-08/msg00539.php

Report errors returned by the XSLT library

* http://archives.postgresql.org/pgsql-hackers/2010-08/msg00562.php

Improve the XSLT parameter passing API

* http://archives.postgresql.org/pgsql-hackers/2010-08/msg00416.php

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +