xmlconcat as variadic function

Started by Peter Eisentrautabout 17 years ago11 messages
#1Peter Eisentraut
peter_e@gmx.net
1 attachment(s)

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.

Btw., the corresponding ecpg changes appear to have fallen into place
automatically. Nice.

Attachments:

xmlconcat-variadic.difftext/plain; name=xmlconcat-variadic.diff; x-mac-creator=0; x-mac-type=0Download
Index: src/backend/executor/execQual.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/execQual.c,v
retrieving revision 1.237
diff -u -3 -p -c -r1.237 execQual.c
*** src/backend/executor/execQual.c	15 Nov 2008 20:52:35 -0000	1.237
--- src/backend/executor/execQual.c	17 Nov 2008 11:17:09 -0000
*************** ExecEvalXml(XmlExprState *xmlExpr, ExprC
*** 3174,3202 ****
  
  	switch (xexpr->op)
  	{
- 		case IS_XMLCONCAT:
- 			{
- 				List	   *values = NIL;
- 
- 				foreach(arg, xmlExpr->args)
- 				{
- 					ExprState  *e = (ExprState *) lfirst(arg);
- 
- 					value = ExecEvalExpr(e, econtext, &isnull, NULL);
- 					if (!isnull)
- 						values = lappend(values, DatumGetPointer(value));
- 				}
- 
- 				if (list_length(values) > 0)
- 				{
- 					*isNull = false;
- 					return PointerGetDatum(xmlconcat(values));
- 				}
- 				else
- 					return (Datum) 0;
- 			}
- 			break;
- 
  		case IS_XMLFOREST:
  		{
  			StringInfoData buf;
--- 3174,3179 ----
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.637
diff -u -3 -p -c -r2.637 gram.y
*** src/backend/parser/gram.y	13 Nov 2008 11:10:06 -0000	2.637
--- src/backend/parser/gram.y	17 Nov 2008 11:17:09 -0000
*************** static TypeName *TableFuncTypeName(List 
*** 474,480 ****
  
  	WHEN WHERE WHITESPACE_P WITH WITHOUT WORK WRITE
  
! 	XML_P XMLATTRIBUTES XMLCONCAT XMLELEMENT XMLFOREST XMLPARSE
  	XMLPI XMLROOT XMLSERIALIZE
  
  	YEAR_P YES_P
--- 474,480 ----
  
  	WHEN WHERE WHITESPACE_P WITH WITHOUT WORK WRITE
  
! 	XML_P XMLATTRIBUTES XMLELEMENT XMLFOREST XMLPARSE
  	XMLPI XMLROOT XMLSERIALIZE
  
  	YEAR_P YES_P
*************** func_expr:	func_name '(' ')'
*** 8620,8629 ****
  					v->location = @1;
  					$$ = (Node *)v;
  				}
- 			| XMLCONCAT '(' expr_list ')'
- 				{
- 					$$ = makeXmlExpr(IS_XMLCONCAT, NULL, NIL, $3, @1);
- 				}
  			| XMLELEMENT '(' NAME_P ColLabel ')'
  				{
  					$$ = makeXmlExpr(IS_XMLELEMENT, $4, NIL, NIL, @1);
--- 8620,8625 ----
*************** col_name_keyword:
*** 9672,9678 ****
  			| VALUES
  			| VARCHAR
  			| XMLATTRIBUTES
- 			| XMLCONCAT
  			| XMLELEMENT
  			| XMLFOREST
  			| XMLPARSE
--- 9668,9673 ----
Index: src/backend/parser/keywords.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/keywords.c,v
retrieving revision 1.205
diff -u -3 -p -c -r1.205 keywords.c
*** src/backend/parser/keywords.c	27 Oct 2008 09:37:47 -0000	1.205
--- src/backend/parser/keywords.c	17 Nov 2008 11:17:09 -0000
*************** const ScanKeyword ScanKeywords[] = {
*** 414,420 ****
  	{"write", WRITE, UNRESERVED_KEYWORD},
  	{"xml", XML_P, UNRESERVED_KEYWORD},
  	{"xmlattributes", XMLATTRIBUTES, COL_NAME_KEYWORD},
- 	{"xmlconcat", XMLCONCAT, COL_NAME_KEYWORD},
  	{"xmlelement", XMLELEMENT, COL_NAME_KEYWORD},
  	{"xmlforest", XMLFOREST, COL_NAME_KEYWORD},
  	{"xmlparse", XMLPARSE, COL_NAME_KEYWORD},
--- 414,419 ----
Index: src/backend/parser/parse_expr.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.237
diff -u -3 -p -c -r1.237 parse_expr.c
*** src/backend/parser/parse_expr.c	26 Oct 2008 02:46:25 -0000	1.237
--- src/backend/parser/parse_expr.c	17 Nov 2008 11:17:09 -0000
*************** transformXmlExpr(ParseState *pstate, Xml
*** 1706,1715 ****
  		newe = transformExpr(pstate, e);
  		switch (x->op)
  		{
- 			case IS_XMLCONCAT:
- 				newe = coerce_to_specific_type(pstate, newe, XMLOID,
- 											   "XMLCONCAT");
- 				break;
  			case IS_XMLELEMENT:
  				/* no coercion necessary */
  				break;
--- 1706,1711 ----
Index: src/backend/parser/parse_target.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.168
diff -u -3 -p -c -r1.168 parse_target.c
*** src/backend/parser/parse_target.c	7 Oct 2008 01:47:55 -0000	1.168
--- src/backend/parser/parse_target.c	17 Nov 2008 11:17:09 -0000
*************** FigureColnameInternal(Node *node, char *
*** 1399,1407 ****
  			/* make SQL/XML functions act like a regular function */
  			switch (((XmlExpr *) node)->op)
  			{
- 				case IS_XMLCONCAT:
- 					*name = "xmlconcat";
- 					return 2;
  				case IS_XMLELEMENT:
  					*name = "xmlelement";
  					return 2;
--- 1399,1404 ----
Index: src/backend/utils/adt/ruleutils.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ruleutils.c,v
retrieving revision 1.287
diff -u -3 -p -c -r1.287 ruleutils.c
*** src/backend/utils/adt/ruleutils.c	6 Oct 2008 20:29:38 -0000	1.287
--- src/backend/utils/adt/ruleutils.c	17 Nov 2008 11:17:09 -0000
*************** get_rule_expr(Node *node, deparse_contex
*** 4519,4527 ****
  
  				switch (xexpr->op)
  				{
- 					case IS_XMLCONCAT:
- 						appendStringInfoString(buf, "XMLCONCAT(");
- 						break;
  					case IS_XMLELEMENT:
  						appendStringInfoString(buf, "XMLELEMENT(");
  						break;
--- 4519,4524 ----
*************** get_rule_expr(Node *node, deparse_contex
*** 4586,4592 ****
  						appendStringInfoString(buf, ", ");
  					switch (xexpr->op)
  					{
- 						case IS_XMLCONCAT:
  						case IS_XMLELEMENT:
  						case IS_XMLFOREST:
  						case IS_XMLPI:
--- 4583,4588 ----
Index: src/backend/utils/adt/xml.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/xml.c,v
retrieving revision 1.81
diff -u -3 -p -c -r1.81 xml.c
*** src/backend/utils/adt/xml.c	10 Nov 2008 18:02:20 -0000	1.81
--- src/backend/utils/adt/xml.c	17 Nov 2008 11:17:10 -0000
*************** xmlcomment(PG_FUNCTION_ARGS)
*** 422,448 ****
   * TODO: xmlconcat needs to merge the notations and unparsed entities
   * of the argument values.	Not very important in practice, though.
   */
! xmltype *
! xmlconcat(List *args)
  {
  #ifdef USE_LIBXML
  	int			global_standalone = 1;
  	xmlChar    *global_version = NULL;
  	bool		global_version_no_value = false;
  	StringInfoData buf;
! 	ListCell   *v;
  
  	initStringInfo(&buf);
! 	foreach(v, args)
  	{
- 		xmltype    *x = DatumGetXmlP(PointerGetDatum(lfirst(v)));
  		size_t		len;
  		xmlChar    *version;
  		int			standalone;
  		char	   *str;
  
! 		len = VARSIZE(x) - VARHDRSZ;
! 		str = text_to_cstring((text *) x);
  
  		parse_xml_decl((xmlChar *) str, &len, &version, NULL, &standalone);
  
--- 422,449 ----
   * TODO: xmlconcat needs to merge the notations and unparsed entities
   * of the argument values.	Not very important in practice, though.
   */
! static xmltype *
! xmlconcat_internal(int num, xmltype *args[])
  {
  #ifdef USE_LIBXML
  	int			global_standalone = 1;
  	xmlChar    *global_version = NULL;
  	bool		global_version_no_value = false;
  	StringInfoData buf;
! 	int			i;
! 
! 	Assert(num > 0);
  
  	initStringInfo(&buf);
! 	for (i = 0; i < num; i++)
  	{
  		size_t		len;
  		xmlChar    *version;
  		int			standalone;
  		char	   *str;
  
! 		len = VARSIZE(args[i]) - VARHDRSZ;
! 		str = text_to_cstring((text *) args[i]);
  
  		parse_xml_decl((xmlChar *) str, &len, &version, NULL, &standalone);
  
*************** xmlconcat(List *args)
*** 485,490 ****
--- 486,524 ----
  }
  
  
+ Datum
+ xmlconcatv(PG_FUNCTION_ARGS)
+ {
+ 	ArrayType  *arg = PG_GETARG_ARRAYTYPE_P(0);
+ 	xmltype	   **newargs;
+ 	int			num;
+ 	int			i;
+ 
+ 	Assert(ARR_NDIM(arg) == 1);
+ 	Assert(ARR_ELEMTYPE(arg) == XMLOID);
+ 
+ 	newargs = palloc(sizeof(*newargs) * ARR_DIMS(arg)[0]);
+ 
+ 	num = 0;
+ 	for (i = ARR_LBOUND(arg)[0]; i < ARR_LBOUND(arg)[0] + ARR_DIMS(arg)[0]; i++)
+ 	{
+ 		Datum		el;
+ 		bool		isnull;
+ 
+ 		el = array_ref(arg, 1, &i, -1, -1, false, 'i', &isnull);
+ 		if (isnull)
+ 			continue;
+ 		else
+ 			newargs[num++] = DatumGetXmlP(el);
+ 	}
+ 
+ 	if (num > 0)
+ 		PG_RETURN_XML_P(xmlconcat_internal(num, newargs));
+ 	else
+ 		PG_RETURN_NULL();
+ }
+ 
+ 
  /*
   * XMLAGG support
   */
*************** xmlconcat2(PG_FUNCTION_ARGS)
*** 501,508 ****
  	else if (PG_ARGISNULL(1))
  		PG_RETURN_XML_P(PG_GETARG_XML_P(0));
  	else
! 		PG_RETURN_XML_P(xmlconcat(list_make2(PG_GETARG_XML_P(0),
! 											 PG_GETARG_XML_P(1))));
  }
  
  
--- 535,547 ----
  	else if (PG_ARGISNULL(1))
  		PG_RETURN_XML_P(PG_GETARG_XML_P(0));
  	else
! 	{
! 		xmltype *newargs[2];
! 
! 		newargs[0] = PG_GETARG_XML_P(0);
! 		newargs[1] = PG_GETARG_XML_P(1);
! 		PG_RETURN_XML_P(xmlconcat_internal(2, newargs));
! 	}
  }
  
  
Index: src/include/catalog/pg_proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/catalog/pg_proc.h,v
retrieving revision 1.528
diff -u -3 -p -c -r1.528 pg_proc.h
*** src/include/catalog/pg_proc.h	14 Nov 2008 00:51:46 -0000	1.528
--- src/include/catalog/pg_proc.h	17 Nov 2008 11:17:10 -0000
*************** DATA(insert OID = 2898 (  xml_recv		   P
*** 4193,4198 ****
--- 4193,4200 ----
  DESCR("I/O");
  DATA(insert OID = 2899 (  xml_send		   PGNSP PGUID 12 1 0 0 f f t f s 1 17 "142" _null_ _null_ _null_ xml_send _null_ _null_ _null_ ));
  DESCR("I/O");
+ DATA(insert OID = 2328 (  xmlconcat		   PGNSP PGUID 12 1 0 142 f f f f i 1 142 "143" "{143}" "{v}" _null_ xmlconcatv _null_ _null_ _null_ ));
+ DESCR("concatenate a list of XML values");
  DATA(insert OID = 2900 (  xmlconcat2	   PGNSP PGUID 12 1 0 0 f f f f i 2 142 "142 142" _null_ _null_ _null_ xmlconcat2 _null_ _null_ _null_ ));
  DESCR("aggregate transition function");
  DATA(insert OID = 2901 (  xmlagg		   PGNSP PGUID 12 1 0 0 t f f f i 1 142 "142" _null_ _null_ _null_ aggregate_dummy _null_ _null_ _null_ ));
Index: src/include/nodes/primnodes.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/primnodes.h,v
retrieving revision 1.143
diff -u -3 -p -c -r1.143 primnodes.h
*** src/include/nodes/primnodes.h	6 Oct 2008 17:39:26 -0000	1.143
--- src/include/nodes/primnodes.h	17 Nov 2008 11:17:10 -0000
*************** typedef struct MinMaxExpr
*** 837,843 ****
   */
  typedef enum XmlExprOp
  {
- 	IS_XMLCONCAT,				/* XMLCONCAT(args) */
  	IS_XMLELEMENT,				/* XMLELEMENT(name, xml_attributes, args) */
  	IS_XMLFOREST,				/* XMLFOREST(xml_attributes) */
  	IS_XMLPARSE,				/* XMLPARSE(text, is_doc, preserve_ws) */
--- 837,842 ----
Index: src/include/utils/xml.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/xml.h,v
retrieving revision 1.24
diff -u -3 -p -c -r1.24 xml.h
*** src/include/utils/xml.h	4 Apr 2008 08:33:15 -0000	1.24
--- src/include/utils/xml.h	17 Nov 2008 11:17:10 -0000
*************** extern Datum xml_out(PG_FUNCTION_ARGS);
*** 32,37 ****
--- 32,38 ----
  extern Datum xml_recv(PG_FUNCTION_ARGS);
  extern Datum xml_send(PG_FUNCTION_ARGS);
  extern Datum xmlcomment(PG_FUNCTION_ARGS);
+ extern Datum xmlconcatv(PG_FUNCTION_ARGS);
  extern Datum xmlconcat2(PG_FUNCTION_ARGS);
  extern Datum texttoxml(PG_FUNCTION_ARGS);
  extern Datum xmltotext(PG_FUNCTION_ARGS);
*************** typedef enum
*** 63,69 ****
  	XML_STANDALONE_OMITTED
  } XmlStandaloneType;
  
- extern xmltype *xmlconcat(List *args);
  extern xmltype *xmlelement(XmlExprState *xmlExpr, ExprContext *econtext);
  extern xmltype *xmlparse(text *data, XmlOptionType xmloption, bool preserve_whitespace);
  extern xmltype *xmlpi(char *target, text *arg, bool arg_is_null, bool *result_is_null);
--- 64,69 ----
Index: src/test/regress/expected/xml.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/xml.out,v
retrieving revision 1.23
diff -u -3 -p -c -r1.23 xml.out
*** src/test/regress/expected/xml.out	15 Nov 2008 20:52:35 -0000	1.23
--- src/test/regress/expected/xml.out	17 Nov 2008 11:17:10 -0000
*************** SELECT xmlconcat('hello', 'you');
*** 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');
*************** SELECT table_name, view_definition FROM 
*** 435,441 ****
   table_name |                                                      view_definition                                                       
  ------------+----------------------------------------------------------------------------------------------------------------------------
   xmlview1   | SELECT xmlcomment('test'::text) AS xmlcomment;
!  xmlview2   | SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
   xmlview3   | SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS "xmlelement";
   xmlview4   | SELECT XMLELEMENT(NAME employee, XMLFOREST(emp.name AS name, emp.age AS age, emp.salary AS pay)) AS "xmlelement" FROM emp;
   xmlview5   | SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
--- 436,442 ----
   table_name |                                                      view_definition                                                       
  ------------+----------------------------------------------------------------------------------------------------------------------------
   xmlview1   | SELECT xmlcomment('test'::text) AS xmlcomment;
!  xmlview2   | SELECT xmlconcat(VARIADIC ARRAY['hello'::xml, 'you'::xml]) AS xmlconcat;
   xmlview3   | SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS "xmlelement";
   xmlview4   | SELECT XMLELEMENT(NAME employee, XMLFOREST(emp.name AS name, emp.age AS age, emp.salary AS pay)) AS "xmlelement" FROM emp;
   xmlview5   | SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
#2Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Peter Eisentraut (#1)
Re: 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

#3Merlin Moncure
mmoncure@gmail.com
In reply to: Peter Eisentraut (#1)
Re: xmlconcat as variadic function

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

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#1)
Re: xmlconcat as variadic function

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

#5Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#4)
Re: xmlconcat as variadic function

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.

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#5)
Re: xmlconcat as variadic function

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

#7Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#6)
Re: xmlconcat as variadic function

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?

#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Eisentraut (#7)
Re: xmlconcat as variadic function

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

#9Joshua D. Drake
jd@commandprompt.com
In reply to: Tom Lane (#8)
Re: xmlconcat as variadic function

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

--

#10Tom Lane
tgl@sss.pgh.pa.us
In reply to: Joshua D. Drake (#9)
Re: xmlconcat as variadic function

"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

#11Peter Eisentraut
peter_e@gmx.net
In reply to: Tom Lane (#10)
Re: xmlconcat as variadic function

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.