Initial review of xslt with no limits patch

Started by Mike Fowlerover 15 years ago64 messageshackers
Jump to latest
#1Mike Fowler
mike@mlfowler.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?

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

#2Pavel Stehule
pavel.stehule@gmail.com
In reply to: Mike Fowler (#1)
Re: Initial review of xslt with no limits patch

Hello

2010/8/2 Mike Fowler <mike@mlfowler.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

Attachments:

xslt.patchtext/x-patch; charset=US-ASCII; name=xslt.patchDownload+46-43
#3Mike Fowler
mike@mlfowler.com
In reply to: Pavel Stehule (#2)
Re: Initial review of xslt with no limits patch

Hi Pavel,

On 02/08/10 09:21, Pavel Stehule wrote:

Hello

2010/8/2 Mike Fowler<mike@mlfowler.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&quot;
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

#4Pavel Stehule
pavel.stehule@gmail.com
In reply to: Mike Fowler (#3)
Re: Initial review of xslt with no limits patch

2010/8/6 Mike Fowler <mike@mlfowler.com>:

Hi Pavel,

On 02/08/10 09:21, Pavel Stehule wrote:

Hello

2010/8/2 Mike Fowler<mike@mlfowler.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&quot;
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

Show quoted text

Regards,

--
Mike Fowler
Registered Linux user: 379787

#5Andrew Dunstan
andrew@dunslane.net
In reply to: Mike Fowler (#3)
Re: Initial review of xslt with no limits patch

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&quot;
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

#6Pavel Stehule
pavel.stehule@gmail.com
In reply to: Andrew Dunstan (#5)
Re: Initial review of xslt with no limits patch

2010/8/6 Andrew Dunstan <andrew@dunslane.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&quot;
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

Show quoted text

cheers

andrew

#7David Fetter
david@fetter.org
In reply to: Pavel Stehule (#6)
Re: Initial review of xslt with no limits patch

On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:

2010/8/6 Andrew Dunstan <andrew@dunslane.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&quot;
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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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

#8Pavel Stehule
pavel.stehule@gmail.com
In reply to: David Fetter (#7)
Re: Initial review of xslt with no limits patch

2010/8/6 David Fetter <david@fetter.org>:

On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:

2010/8/6 Andrew Dunstan <andrew@dunslane.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&quot;
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

Show quoted text

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.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

#9Andrew Dunstan
andrew@dunslane.net
In reply to: Pavel Stehule (#8)
Re: Initial review of xslt with no limits patch

On 08/06/2010 02:29 AM, Pavel Stehule wrote:

2010/8/6 David Fetter<david@fetter.org>:

On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:

2010/8/6 Andrew Dunstan<andrew@dunslane.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&quot;
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

#10Mike Fowler
mike@mlfowler.com
In reply to: Andrew Dunstan (#9)
Re: Initial review of xslt with no limits patch

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@fetter.org>:

On Fri, Aug 06, 2010 at 05:57:37AM +0200, Pavel Stehule wrote:

2010/8/6 Andrew Dunstan<andrew@dunslane.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&quot;
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

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Mike Fowler (#3)
Re: Initial review of xslt with no limits patch

Mike Fowler <mike@mlfowler.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

#12Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#11)
Re: Initial review of xslt with no limits patch

Hello

attached updated patch with regression test

2010/8/6 Tom Lane <tgl@sss.pgh.pa.us>:

Mike Fowler <mike@mlfowler.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

Show quoted text

                       regards, tom lane

Attachments:

xslt.difftext/x-patch; charset=US-ASCII; name=xslt.diffDownload+164-43
#13Andrew Dunstan
andrew@dunslane.net
In reply to: Tom Lane (#11)
Re: Initial review of xslt with no limits patch

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

#14Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andrew Dunstan (#13)
Re: Initial review of xslt with no limits patch

Andrew Dunstan <andrew@dunslane.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

#15Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#14)
Re: Initial review of xslt with no limits patch

2010/8/6 Tom Lane <tgl@sss.pgh.pa.us>:

Andrew Dunstan <andrew@dunslane.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

Show quoted text

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

#16Robert Haas
robertmhaas@gmail.com
In reply to: Pavel Stehule (#15)
Re: Initial review of xslt with no limits patch

On Fri, Aug 6, 2010 at 1:46 PM, Pavel Stehule <pavel.stehule@gmail.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

#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Pavel Stehule (#15)
Re: Initial review of xslt with no limits patch

Pavel Stehule <pavel.stehule@gmail.com> writes:

2010/8/6 Tom Lane <tgl@sss.pgh.pa.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

#18Pavel Stehule
pavel.stehule@gmail.com
In reply to: Robert Haas (#16)
Re: Initial review of xslt with no limits patch

2010/8/6 Robert Haas <robertmhaas@gmail.com>:

On Fri, Aug 6, 2010 at 1:46 PM, Pavel Stehule <pavel.stehule@gmail.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

Show quoted text

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

#19Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#16)
Re: Initial review of xslt with no limits patch

Robert Haas <robertmhaas@gmail.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

#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Pavel Stehule (#18)
Re: Initial review of xslt with no limits patch

Pavel Stehule <pavel.stehule@gmail.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

#21Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#20)
#22David E. Wheeler
david@kineticode.com
In reply to: Tom Lane (#19)
#23Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#14)
#24Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#21)
#25Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#22)
#26Pavel Stehule
pavel.stehule@gmail.com
In reply to: Peter Eisentraut (#24)
#27Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#25)
#28David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#25)
#29Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#28)
#30David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#29)
#31Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#23)
#32Tom Lane
tgl@sss.pgh.pa.us
In reply to: David E. Wheeler (#30)
#33David Fetter
david@fetter.org
In reply to: Peter Eisentraut (#24)
#34Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#32)
#35David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#34)
#36Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#35)
#37Tom Lane
tgl@sss.pgh.pa.us
In reply to: David E. Wheeler (#35)
#38David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#36)
#39Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#37)
#40David E. Wheeler
david@kineticode.com
In reply to: Tom Lane (#37)
#41David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#39)
#42Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#38)
#43Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#41)
#44David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#43)
#45Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#44)
#46David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#45)
#47Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#46)
#48Mike Fowler
mike@mlfowler.com
In reply to: Pavel Stehule (#12)
#49David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#47)
#50Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#49)
#51Kevin Grittner
Kevin.Grittner@wicourts.gov
In reply to: Pavel Stehule (#50)
#52David E. Wheeler
david@kineticode.com
In reply to: Pavel Stehule (#50)
#53Pavel Stehule
pavel.stehule@gmail.com
In reply to: Kevin Grittner (#51)
#54Pavel Stehule
pavel.stehule@gmail.com
In reply to: David E. Wheeler (#52)
#55Robert Haas
robertmhaas@gmail.com
In reply to: Mike Fowler (#48)
#56Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#55)
#57Mike Fowler
mike@mlfowler.com
In reply to: Tom Lane (#56)
#58Tom Lane
tgl@sss.pgh.pa.us
In reply to: Mike Fowler (#57)
#59Robert Haas
robertmhaas@gmail.com
In reply to: Tom Lane (#58)
#60Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#59)
#61Tom Lane
tgl@sss.pgh.pa.us
In reply to: Mike Fowler (#48)
#62Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#61)
#63Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#62)
#64Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#63)