Обсуждение: xmlconcat as variadic function

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.
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";

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
todayalready: 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.