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

[Tom Lane]

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

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

[Tom Lane]

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.  I could set up the patch to treat
composite domains the same way, or not, but it wasn't obvious what
to do because the point of the exclusion wasn't clear.

Some excavation in our git history shows that the exclusion was
added here:

commit 990eb8552e69a492840d46b58ceb630a8a295e54
Author: Tom Lane 
Date:   Wed Dec 12 03:28:49 2001 +

Don't accept names of complex types (ie, relation types) as being
requests for implicit trivial coercions.  Prevents sillinesses like
this one:
regression=# select x.int8_tbl.q1 from int8_tbl x;
ERROR:  fmgr_info: function 270997776: cache lookup failed

I could not find any contemporaneous discussion in the list archives,
so I'm guessing that I tripped over this corner case and did not think
it was worth spending any great amount of effort on.  But in hindsight
the fix sure looks like a band-aid that's covering up a deeper problem.
Whatever that problem was, it's gone now --- if I dike out the check
then the parser correctly concludes that coercing int8_tbl to int8_tbl
is a no-op.  You need more parens nowadays, but either of these work:

select (x.int8_tbl).q1 from int8_tbl x;
select (int8_tbl(x)).q1 from int8_tbl x;

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.
It seems weirdly asymmetric that we allow typename(x) to be a coercion
request unless typename is a composite type.  And that exception is
not documented in any of the places that talk about this syntax,
e.g. section 4.2.9, section 10.3 rule 3, or the CREATE CAST man page.

So I'm inclined to remove the exception for composite types, effectively
reverting 990eb8552.  Alternatively, if we keep it, it needs to be
documented as to the reasoning for having it.

Thoughts?

regards, tom lane


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