Useless(?) asymmetry in parse_func.c

Started by Tom Laneover 8 years ago2 messageshackers
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

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 <tgl@sss.pgh.pa.us>
Date: Wed Dec 12 03:28:49 2001 +0000

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

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#1)
Re: 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

Attachments:

allow-function-syntax-cast-to-complex.patchtext/x-diff; charset=us-ascii; name=allow-function-syntax-cast-to-complex.patchDownload+17-12