Re: [HACKERS] Useless(?) asymmetry in parse_func.c

I wrote:
> While running down loose ends in my domains-over-composite patch,
> I wondered why parse_func.c's FuncNameAsType() excludes composite
> types as possible type names.
> ...
> There might still be an argument for rejecting the case on the grounds
> that it's confusing or likely to be user error, but I'm not sure.

After studying the code more closely, I realized that there are only two
cases this restriction actually rejects, because no other cases with a
composite destination type could get past the restrictions on a
function-like cast in func_get_detail:

1. Function-style coercion of an unknown literal, eg
    SELECT int8_tbl('(1,2)');

But I do not see any good argument why this should be rejected for
composite types when it works for other types.

2. Coercion of a composite type to itself, eg
    SELECT int8_tbl(int8_tbl) FROM int8_tbl;
    SELECT int8_tbl.int8_tbl FROM int8_tbl;

find_coercion_pathway would report that as a RELABELTYPE case, whereas
it would not recognize any other coercion to a composite type as
either RELABELTYPE or COERCEVIAIO.  (At least, not unless the user has
added such casts with CREATE CAST; in which case I think he's entitled
to expect that the system would be willing to use them.)

On the whole, probably restriction #2 is a good thing; if we were to lift
it, I think we'd start getting complaints about "why does my table seem to
have a column named after itself", rather like the complaints we got about
"why does my table seem to have a column named "text"" before we put in
the other arbitrary-seeming restriction in func_get_detail.

However, the existing code is certainly an opaque and undocumented way of
enforcing #2, plus it breaks #1 for no very good reason.  So I think we
should remove the restriction in FuncNameAsType and instead enforce #2
in a narrowly tailored way, as in the attached patch.

            regards, tom lane

diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 2f2f2c7..050779b 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
*************** func_get_detail(List *funcname,
*** 1357,1362 ****
--- 1357,1367 ----
           * never supported that historically, so we can insist that people
           * write it as a normal cast instead.
           *
+          * We also reject the specific case of casting a composite type to
+          * itself (with RELABELTYPE).  That's not really a useful thing to do,
+          * and it would likely produce user complaints about "why does my
+          * table seem to have a column named after itself?"
+          *
           * We also reject the specific case of COERCEVIAIO for a composite
           * source type and a string-category target type.  This is a case that
           * find_coercion_pathway() allows by default, but experience has shown
*************** func_get_detail(List *funcname,
*** 1395,1401 ****
                      switch (cpathtype)
                      {
                          case COERCION_PATH_RELABELTYPE:
!                             iscoercion = true;
                              break;
                          case COERCION_PATH_COERCEVIAIO:
                              if ((sourceType == RECORDOID ||
--- 1400,1411 ----
                      switch (cpathtype)
                      {
                          case COERCION_PATH_RELABELTYPE:
!                             if (sourceType == targetType &&
!                                 (sourceType == RECORDOID ||
!                                  ISCOMPLEX(sourceType)))
!                                 iscoercion = false;
!                             else
!                                 iscoercion = true;
                              break;
                          case COERCION_PATH_COERCEVIAIO:
                              if ((sourceType == RECORDOID ||
*************** make_fn_arguments(ParseState *pstate,
*** 1761,1767 ****
   *      convenience routine to see if a function name matches a type name
   *
   * Returns the OID of the matching type, or InvalidOid if none.  We ignore
!  * shell types and complex types.
   */
  static Oid
  FuncNameAsType(List *funcname)
--- 1771,1777 ----
   *      convenience routine to see if a function name matches a type name
   *
   * Returns the OID of the matching type, or InvalidOid if none.  We ignore
!  * shell types, since casting to a shell type would be useless.
   */
  static Oid
  FuncNameAsType(List *funcname)
*************** FuncNameAsType(List *funcname)
*** 1773,1780 ****
      if (typtup == NULL)
          return InvalidOid;

!     if (((Form_pg_type) GETSTRUCT(typtup))->typisdefined &&
!         !OidIsValid(typeTypeRelid(typtup)))
          result = typeTypeId(typtup);
      else
          result = InvalidOid;
--- 1783,1789 ----
      if (typtup == NULL)
          return InvalidOid;

!     if (((Form_pg_type) GETSTRUCT(typtup))->typisdefined)
          result = typeTypeId(typtup);
      else
          result = InvalidOid;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers