xmlconcat as variadic function

Peter Eisentraut wrote:

Here is a patch to reimplement the xmlconcat functionality as a variadic
function instead of a hardcoded special expression type. I haven't
found any variadic function in the set of built-in functions so far, so
I figured I would ask for feedback before we go down this route.

I can't comment on the patch, but it would be nice to have a built-in
variadic function, just to have an example and some testing of the feature.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, Nov 17, 2008 at 6:22 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

Here is a patch to reimplement the xmlconcat functionality as a variadic
function instead of a hardcoded special expression type. I haven't found
any variadic function in the set of built-in functions so far, so I figured
I would ask for feedback before we go down this route.

One small side effect is that variadic functions have stricter casting
rules...you can't pass string literals without a cast. This is not
necessarily a bad thing, but the invocations are not quite compatible.

merlin

Peter Eisentraut <peter_e@gmx.net> writes:

Here is a patch to reimplement the xmlconcat functionality as a variadic
function instead of a hardcoded special expression type.

What's the point of this? I suppose making xmlconcat not a keyword is
some small advantage, but having it behave subtly differently from the
other xmlfoo functions isn't really all that nice. And the change in
this error message is not for the better:

*** 55,63 ****
(1 row)

SELECT xmlconcat(1, 2);
! ERROR:  argument of XMLCONCAT must be type xml, not type integer
LINE 1: SELECT xmlconcat(1, 2);
!                          ^
SELECT xmlconcat('bad', '<syntax');
ERROR:  invalid XML content
LINE 1: SELECT xmlconcat('bad', '<syntax');
--- 55,64 ----
(1 row)

SELECT xmlconcat(1, 2);
! ERROR: function xmlconcat(integer, integer) does not exist
LINE 1: SELECT xmlconcat(1, 2);
! ^
! HINT: No function matches the given name and argument types. You might need to add explicit type casts.
SELECT xmlconcat('bad', '<syntax');
ERROR: invalid XML content
LINE 1: SELECT xmlconcat('bad', '<syntax');

On the whole I think we should just leave it alone.

regards, tom lane

Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Here is a patch to reimplement the xmlconcat functionality as a variadic
function instead of a hardcoded special expression type.

What's the point of this? I suppose making xmlconcat not a keyword is
some small advantage, but having it behave subtly differently from the
other xmlfoo functions isn't really all that nice.

Different from what and how? We already have a mix of XML functions
implemented in user space and some as built-in expressions. And they
all have different signatures and purposes, so I don't see a lot of need
to align anything here.

If variadic functions had been available in 8.3, we would probably have
implemented xmlconcat like this in the first place.

And the change in
this error message is not for the better:

*** 55,63 ****
(1 row)

SELECT xmlconcat(1, 2);
! ERROR:  argument of XMLCONCAT must be type xml, not type integer
LINE 1: SELECT xmlconcat(1, 2);
!                          ^
SELECT xmlconcat('bad', '<syntax');
ERROR:  invalid XML content
LINE 1: SELECT xmlconcat('bad', '<syntax');
--- 55,64 ----
(1 row)

SELECT xmlconcat(1, 2);
! ERROR: function xmlconcat(integer, integer) does not exist
LINE 1: SELECT xmlconcat(1, 2);
! ^
! HINT: No function matches the given name and argument types. You might need to add explicit type casts.
SELECT xmlconcat('bad', '<syntax');
ERROR: invalid XML content
LINE 1: SELECT xmlconcat('bad', '<syntax');

If you think this is a problem, then it can be fixed. I never
particularly liked this second error message in general. A much better
hint that would address this case and many others might be:

ERROR: function foo(type, type) does not exist
HINT: There is foo(type, type) and foo(type, type) instead.

There are a number of other distinguishable cases that call for better
hints, e.g., no foo() exists at all. I think in my experience, "you
need to add explicit type casts" has almost never been the actual
solution when this message comes up.

Peter Eisentraut <peter_e@gmx.net> writes:

Tom Lane wrote:

What's the point of this? I suppose making xmlconcat not a keyword is
some small advantage, but having it behave subtly differently from the
other xmlfoo functions isn't really all that nice.

Different from what and how? We already have a mix of XML functions
implemented in user space and some as built-in expressions.

Well, for example pg_catalog.xmlconcat(...) will behave differently if
we make this change.

If there were some useful benefit to be gained by altering the behavior
established by 8.3, then I'd be for it, but this just seems like code
churn.

regards, tom lane

Tom Lane wrote:

Well, for example pg_catalog.xmlconcat(...) will behave differently if
we make this change.

Uh, because there is no pg_catalog.xmlconcat() in 8.3. Why is that
relevant?

Peter Eisentraut <peter_e@gmx.net> writes:

Tom Lane wrote:

Well, for example pg_catalog.xmlconcat(...) will behave differently if
we make this change.

Uh, because there is no pg_catalog.xmlconcat() in 8.3. Why is that
relevant?

My point is it's a user-visible change --- maybe a subtle one, but still
a change that in principle could break some app somewhere --- and no
good reason has been put forward for making it.

regards, tom lane

On Mon, 2008-11-17 at 19:34 -0500, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Tom Lane wrote:

Well, for example pg_catalog.xmlconcat(...) will behave differently if
we make this change.

Uh, because there is no pg_catalog.xmlconcat() in 8.3. Why is that
relevant?

My point is it's a user-visible change --- maybe a subtle one, but still
a change that in principle could break some app somewhere --- and no
good reason has been put forward for making it.

Uhh... what user is going to be calling pg_catalog.xmlconcat() in any
version? Wouldn't it be public.xml... or other namespace?

Joshua D. Drake

regards, tom lane

--

"Joshua D. Drake" <jd@commandprompt.com> writes:

On Mon, 2008-11-17 at 19:34 -0500, Tom Lane wrote:

My point is it's a user-visible change --- maybe a subtle one, but still
a change that in principle could break some app somewhere --- and no
good reason has been put forward for making it.

Uhh... what user is going to be calling pg_catalog.xmlconcat() in any
version?

Anybody wanting to ensure that they got the system version of the
function rather than some other version. In 8.3 that wasn't necessary
because the function had a special production, but with this change
schema-qualification would become *necessary* for anyone wanting to
avoid search path gotchas. So arguably we'd be creating a security hole
that wasn't there in 8.3. Immediately visible breakage would probably
only happen in the other direction, ie trying to run an 8.4 app on 8.3.

I still haven't heard an argument what's the value of changing it, anyway.

regards, tom lane

Tom Lane wrote:

"Joshua D. Drake" <jd@commandprompt.com> writes:

On Mon, 2008-11-17 at 19:34 -0500, Tom Lane wrote:

My point is it's a user-visible change --- maybe a subtle one, but still
a change that in principle could break some app somewhere --- and no
good reason has been put forward for making it.

Uhh... what user is going to be calling pg_catalog.xmlconcat() in any
version?

Anybody wanting to ensure that they got the system version of the
function rather than some other version. In 8.3 that wasn't necessary
because the function had a special production, but with this change
schema-qualification would become *necessary* for anyone wanting to
avoid search path gotchas. So arguably we'd be creating a security hole
that wasn't there in 8.3. Immediately visible breakage would probably
only happen in the other direction, ie trying to run an 8.4 app on 8.3.

I still haven't heard an argument what's the value of changing it, anyway.

Well, if that is the attitude, then there is never going to be any
purpose or incentive to generalized hard-coded hacks into generalized
features. Moreover, what is the point of extensibility if the system
itself cannot use it.

If you think the scenario you describe above is relevant, then perhaps
people shouldn't be allow to override system objects in the first place.
Because this security hole pretty much exits today already: Anyone can
place an object in the public schema, and in reality hardly anyone
schema-qualifies system objects. If you are trying to be sneaky, you
place your own versions of functions added in 8.4 into 8.3, let the
admin upgrade and then let him try out all this great new functionality
of 8.4 based on your "versions". I am not sure what our response to
this is, but creating a singular solution for xmlconcat is probably not
sufficient.

If you think about it, the default search path of

public, pg_catalog

is in Unix terms equivalent to a default path -- for everyone, including
root -- of

~user1/bin:~user2/bin:...:~userN/bin:/usr/bin:/bin

Maybe the relative placements of these things needs to be rethought.