Re: Domains versus polymorphic functions, redux

In http://archives.postgresql.org/pgsql-bugs/2011-05/msg00171.php
Regina Obe complains that this fails in 9.1, though it worked before:

regression=# CREATE DOMAIN topoelementarray AS integer[];
CREATE DOMAIN
regression=# SELECT array_upper(ARRAY[[1,2], [3,4]]::topoelementarray, 1);
ERROR: function array_upper(topoelementarray, integer) does not exist

This is a consequence of the changes I made to fix bug #5717,
particularly the issues around ANYARRAY matching discussed here:
http://archives.postgresql.org/pgsql-hackers/2010-10/msg01545.php

Regina is the second or third beta tester to complain about domains over
arrays no longer matching ANYARRAY, so I think we'd better do something
about it. I haven't tried to code anything up yet, but the ideas I'm
considering trying to implement go like this:

1. If a domain type is passed to an ANYARRAY argument, automatically
downcast it to its base type (which of course had better then be an
array). This would include inserting an implicit cast into the
expression tree, so that if the function uses get_fn_expr_argtype or
similar, it would see the base type. Also, if the function returns
ANYARRAY, its result is considered to be of the base type not the
domain.

2. If a domain type is passed to an ANYELEMENT argument, automatically
downcast it to its base type if there is any ANYARRAY argument, or if
the function result type is ANYARRAY, or if any other ANYELEMENT
argument is not of the same domain type. The first two cases are
necessary since we don't have arrays of domains: the match is guaranteed
to fail if we don't do this, since there can be no matching array type
for the domain. The third case is meant to handle cases like
function(domain-over-int, 42) where the function has two ANYELEMENT
arguments: we now fail, but reducing the domain to int would allow
success.

An alternative rule we could use in place of #2 is just "smash domains
to base types always, when they're matched to ANYELEMENT". That would
be simpler and more in keeping with #1, but it might change the behavior
in cases where the historical behavior is reasonable (unlike the cases
discussed in my message referenced above...) I find this simpler rule
tempting from an implementor's standpoint, but am unsure if there'll be
complaints.

Comments, better ideas?

regards, tom lane

On Tue, May 24, 2011 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 1. If a domain type is passed to an ANYARRAY argument, automatically
> downcast it to its base type (which of course had better then be an
> array).  This would include inserting an implicit cast into the
> expression tree, so that if the function uses get_fn_expr_argtype or
> similar, it would see the base type.  Also, if the function returns
> ANYARRAY, its result is considered to be of the base type not the
> domain.

Does that mean that plpgsql %type variable declarations will see the
base type (and miss any constraint checks?). I think it's fine either
way, but that's worth noting.

> An alternative rule we could use in place of #2 is just "smash domains
> to base types always, when they're matched to ANYELEMENT".  That would
> be simpler and more in keeping with #1, but it might change the behavior
> in cases where the historical behavior is reasonable (unlike the cases
> discussed in my message referenced above...)  I find this simpler rule
> tempting from an implementor's standpoint, but am unsure if there'll be
> complaints.

#2a seems cleaner to me (superficially). Got an example of a behavior
you think is changed? In particular, is there a way the new function
would fail where it used to not fail?

merlin

On May 24, 2011, at 9:12 AM, Tom Lane wrote:

> An alternative rule we could use in place of #2 is just "smash domains
> to base types always, when they're matched to ANYELEMENT". That would
> be simpler and more in keeping with #1, but it might change the behavior
> in cases where the historical behavior is reasonable (unlike the cases
> discussed in my message referenced above...) I find this simpler rule
> tempting from an implementor's standpoint, but am unsure if there'll be
> complaints.

I'm not sure where the historical behavior manifests, but this certainly seems like it might be the most consistent implementation, as well. Which option is least likely to violate the principal of surprise?

Best,

David

On Tue, May 24, 2011 at 12:12:55PM -0400, Tom Lane wrote:
> In http://archives.postgresql.org/pgsql-bugs/2011-05/msg00171.php
> Regina Obe complains that this fails in 9.1, though it worked before:
>
> regression=# CREATE DOMAIN topoelementarray AS integer[];
> CREATE DOMAIN
> regression=# SELECT array_upper(ARRAY[[1,2], [3,4]]::topoelementarray, 1);
> ERROR: function array_upper(topoelementarray, integer) does not exist
>
> This is a consequence of the changes I made to fix bug #5717,
> particularly the issues around ANYARRAY matching discussed here:
> http://archives.postgresql.org/pgsql-hackers/2010-10/msg01545.php
>
> Regina is the second or third beta tester to complain about domains over
> arrays no longer matching ANYARRAY, so I think we'd better do something
> about it. I haven't tried to code anything up yet, but the ideas I'm
> considering trying to implement go like this:
>
> 1. If a domain type is passed to an ANYARRAY argument, automatically
> downcast it to its base type (which of course had better then be an
> array). This would include inserting an implicit cast into the
> expression tree, so that if the function uses get_fn_expr_argtype or
> similar, it would see the base type. Also, if the function returns
> ANYARRAY, its result is considered to be of the base type not the
> domain.

We discussed this a few weeks ago:
http://archives.postgresql.org/message-id/20110511093217.GB26552@tornado.gateway.2wire.net

What's to recommend #1 over what I proposed then? Seems like a discard of
functionality for little benefit.

> 2. If a domain type is passed to an ANYELEMENT argument, automatically
> downcast it to its base type if there is any ANYARRAY argument, or if
> the function result type is ANYARRAY, or if any other ANYELEMENT
> argument is not of the same domain type. The first two cases are
> necessary since we don't have arrays of domains: the match is guaranteed
> to fail if we don't do this, since there can be no matching array type
> for the domain. The third case is meant to handle cases like
> function(domain-over-int, 42) where the function has two ANYELEMENT
> arguments: we now fail, but reducing the domain to int would allow
> success.

This seems generally consistent with other function-resolution rules around
domains. On the other hand, existing users have supposedly coped by adding an
explicit cast to one or the other argument to get the behavior they want. New
applications will quietly get the cast, as it were, on the domain argument(s).
I hesitate to say this is so clearly right as to warrant that change. Even if
it is right, though, this smells like 9.2 material.

nm

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On May 24, 2011, at 9:12 AM, Tom Lane wrote:
>> An alternative rule we could use in place of #2 is just "smash domains
>> to base types always, when they're matched to ANYELEMENT". That would
>> be simpler and more in keeping with #1, but it might change the behavior
>> in cases where the historical behavior is reasonable (unlike the cases
>> discussed in my message referenced above...) I find this simpler rule
>> tempting from an implementor's standpoint, but am unsure if there'll be
>> complaints.

> I'm not sure where the historical behavior manifests, but this
> certainly seems like it might be the most consistent implementation,
> as well. Which option is least likely to violate the principal of
> surprise?

Well, the basic issue here is what happens when a function like

create function noop(anyelement) returns anyelement ...

is applied to a domain argument. Currently, the result is thought to be
of the domain type, whereas if we smash to base unconditionally, the
result will be thought to be of the domain's base type. You can make an
argument for either behavior, but I think the argument for the current
behavior hinges on the assumption that such a function isn't "doing
anything" to the argument value, only passing it through as-is.

I should probably also point out the previous discussion of this area
from a couple weeks ago, notably here:
http://archives.postgresql.org/pgsql-hackers/2011-05/msg00640.php
The example I gave there seems relevant:

create function negate(anyelement) returns anyelement as
$$ select - $1 $$ language sql;

create domain pos as int check (value > 0);

select negate(42::pos);

This example function isn't quite silly --- it will work on any datatype
having a unary '-' operator, and you could imagine someone wanting to do
something roughly like this in more realistic cases. But if you want to
assume that the function returns pos when handed pos, you'd better be
prepared to insert a CastToDomain node to recheck the domain constraint.
Right now the SQL-function code doesn't support such cases:

regression=# select negate(42::pos);
ERROR: return type mismatch in function declared to return pos
DETAIL: Actual return type is integer.
CONTEXT: SQL function "negate" during inlining

If we smashed to base type then this issue would go away.

On the other hand it feels like we'd be taking yet another step away
from allowing domains to be usefully used in function declarations.
I can't put my finger on any concrete consequence of that sort, since
what we're talking about here is ANYELEMENT/ANYARRAY functions not
functions declared to take domains --- but it sure seems like this
would put domains even further away from the status of first-class
citizenship in the type system.

regards, tom lane

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Tue, May 24, 2011 at 11:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 1. If a domain type is passed to an ANYARRAY argument, automatically
>> downcast it to its base type (which of course had better then be an
>> array).

> Does that mean that plpgsql %type variable declarations will see the
> base type (and miss any constraint checks?).

No, this has nothing to do with %type. What's at stake is matching to
functions/operators that are declared to take ANYARRAY.

> #2a seems cleaner to me (superficially). Got an example of a behavior
> you think is changed?

See my response to David Wheeler.

regards, tom lane

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Tue, May 24, 2011 at 12:12:55PM -0400, Tom Lane wrote:
>> This is a consequence of the changes I made to fix bug #5717,
>> particularly the issues around ANYARRAY matching discussed here:
>> http://archives.postgresql.org/pgsql-hackers/2010-10/msg01545.php

> We discussed this a few weeks ago:
> http://archives.postgresql.org/message-id/20110511093217.GB26552@tornado.gateway.2wire.net

> What's to recommend #1 over what I proposed then? Seems like a discard of
> functionality for little benefit.

I am unwilling to commit to making #2 work, especially not under time
constraints; and you apparently aren't either, since you haven't
produced the patch you alluded to at the end of that thread. Even if
you had, though, I'd have no confidence that all holes of the sort had
been closed. What you're proposing is to ratchet up the implementation
requirements for every PL and and every C function declared to accept
polymorphic types, and there are a lot of members of both classes that
we don't control.

> I hesitate to say this is so clearly right as to warrant that change. Even if
> it is right, though, this smells like 9.2 material.

Well, I'd been hoping to leave it for later too, but it seems like we
have to do something about the ANYARRAY case for 9.1. Making ANYARRAY's
response to domains significantly inconsistent with ANYELEMENT's
response doesn't seem like a good plan.

regards, tom lane

On Tue, May 24, 2011 at 01:28:38PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > On Tue, May 24, 2011 at 12:12:55PM -0400, Tom Lane wrote:
> >> This is a consequence of the changes I made to fix bug #5717,
> >> particularly the issues around ANYARRAY matching discussed here:
> >> http://archives.postgresql.org/pgsql-hackers/2010-10/msg01545.php
>
> > We discussed this a few weeks ago:
> > http://archives.postgresql.org/message-id/20110511093217.GB26552@tornado.gateway.2wire.net
>
> > What's to recommend #1 over what I proposed then? Seems like a discard of
> > functionality for little benefit.
>
> I am unwilling to commit to making #2 work, especially not under time
> constraints; and you apparently aren't either, since you haven't
> produced the patch you alluded to at the end of that thread.

I took your lack of any response as non-acceptance of the plan I outlined.
Alas, the wrong conclusion. I'll send a patch this week.

> Even if
> you had, though, I'd have no confidence that all holes of the sort had
> been closed. What you're proposing is to ratchet up the implementation
> requirements for every PL and and every C function declared to accept
> polymorphic types, and there are a lot of members of both classes that
> we don't control.

True. I will not give you that confidence. Those omissions would have to
remain bugs to be fixed as they're found.

nm

On May 24, 2011, at 10:11 AM, Tom Lane wrote:

> regression=# select negate(42::pos);
> ERROR: return type mismatch in function declared to return pos
> DETAIL: Actual return type is integer.
> CONTEXT: SQL function "negate" during inlining
>
> If we smashed to base type then this issue would go away.

+1

> On the other hand it feels like we'd be taking yet another step away
> from allowing domains to be usefully used in function declarations.
> I can't put my finger on any concrete consequence of that sort, since
> what we're talking about here is ANYELEMENT/ANYARRAY functions not
> functions declared to take domains --- but it sure seems like this
> would put domains even further away from the status of first-class
> citizenship in the type system.

I agree. It sure seems to me like DOMAINs should act exactly like any other type. I know that has improved over time, and superficially at least, the above will make it seem like more like than it does with the error. But maybe it's time to re-think how domains are implemented? (Not for 9.1, mind.) I mean, why *don't* they act like first class types?

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On May 24, 2011, at 10:11 AM, Tom Lane wrote:
>> On the other hand it feels like we'd be taking yet another step away
>> from allowing domains to be usefully used in function declarations.

> I agree. It sure seems to me like DOMAINs should act exactly like any
> other type. I know that has improved over time, and superficially at
> least, the above will make it seem like more like than it does with
> the error. But maybe it's time to re-think how domains are
> implemented? (Not for 9.1, mind.) I mean, why *don't* they act like
> first class types?

Well, if they actually were first-class types, they probably wouldn't
be born with an implicit cast to some other type to handle 99% of
operations on them ;-). I think the hard part here is having that cake
and eating it too, ie, supporting domain-specific functions without
breaking the implicit use of the base type's functions.

I guess that the question that's immediately at hand is sort of a
variant of that, because using a polymorphic function declared to take
ANYARRAY on a domain-over-array really is using a portion of the base
type's functionality. What we've learned from bug #5717 and the
subsequent issues is that using that base functionality without
immediately abandoning the notion that the domain has some life of its
own (ie, immediately casting to the base type) is harder than it looks.

regards, tom lane

On May 24, 2011, at 11:30 AM, Tom Lane wrote:

> Well, if they actually were first-class types, they probably wouldn't
> be born with an implicit cast to some other type to handle 99% of
> operations on them ;-). I think the hard part here is having that cake
> and eating it too, ie, supporting domain-specific functions without
> breaking the implicit use of the base type's functions.

Yeah.

> I guess that the question that's immediately at hand is sort of a
> variant of that, because using a polymorphic function declared to take
> ANYARRAY on a domain-over-array really is using a portion of the base
> type's functionality. What we've learned from bug #5717 and the
> subsequent issues is that using that base functionality without
> immediately abandoning the notion that the domain has some life of its
> own (ie, immediately casting to the base type) is harder than it looks.

Well, in the ANYELEMENT context (or ANYARRAY), what could be lost by "abandoning the notion that the domain has some life of its own"?

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On May 24, 2011, at 11:30 AM, Tom Lane wrote:
>> I guess that the question that's immediately at hand is sort of a
>> variant of that, because using a polymorphic function declared to take
>> ANYARRAY on a domain-over-array really is using a portion of the base
>> type's functionality. What we've learned from bug #5717 and the
>> subsequent issues is that using that base functionality without
>> immediately abandoning the notion that the domain has some life of its
>> own (ie, immediately casting to the base type) is harder than it looks.

> Well, in the ANYELEMENT context (or ANYARRAY), what could be lost by "abandoning the notion that the domain has some life of its own"?

I'm starting to think that maybe we should separate the two cases after
all. If we force a downcast for ANYARRAY matching, we will fix the loss
of functionality induced by the bug #5717 patch, and it doesn't seem
like anyone has a serious objection to that. What to do for ANYELEMENT
seems to be a bit more controversial, and at least some of the proposals
aren't reasonable to do in 9.1 at this stage. Maybe we should just
leave ANYELEMENT as-is for the moment, and reconsider that issue later?

regards, tom lane

On Tue, May 24, 2011 at 02:00:54PM -0400, Noah Misch wrote:
> On Tue, May 24, 2011 at 01:28:38PM -0400, Tom Lane wrote:
> > Noah Misch <noah(at)leadboat(dot)com> writes:
> > > On Tue, May 24, 2011 at 12:12:55PM -0400, Tom Lane wrote:
> > >> This is a consequence of the changes I made to fix bug #5717,
> > >> particularly the issues around ANYARRAY matching discussed here:
> > >> http://archives.postgresql.org/pgsql-hackers/2010-10/msg01545.php
> >
> > > We discussed this a few weeks ago:
> > > http://archives.postgresql.org/message-id/20110511093217.GB26552@tornado.gateway.2wire.net
> >
> > > What's to recommend #1 over what I proposed then? Seems like a discard of
> > > functionality for little benefit.
> >
> > I am unwilling to commit to making #2 work, especially not under time
> > constraints; and you apparently aren't either, since you haven't
> > produced the patch you alluded to at the end of that thread.
>
> I took your lack of any response as non-acceptance of the plan I outlined.
> Alas, the wrong conclusion. I'll send a patch this week.

See attached arrdom1v1-polymorphism.patch. This currently adds one syscache
lookup per array_append, array_prepend or array_cat call when the anyarray
type is not a domain. When the type is a domain, it adds a few more. We
could add caching without too much trouble. I suppose someone out there uses
these functions in bulk operations, though I've yet to see it. Is it worth
optimizing this straightway?

For a function like
CREATE FUNCTION f(anyarray, VARIADIC anyarray) RETURNS int LANGUAGE sql
AS 'SELECT array_length($1, 1) + array_length($2, 1)'
we must coerce the variadic argument array to a domain type when the other
anyarray argument(s) compel it. Having implemented that, it was nearly free
to re-support a VARIADIC parameter specifically declared with a domain over an
array. Consequently, I've done that as well.

See here for previously-disclosed rationale:
http://archives.postgresql.org/message-id/20110511191249.GA29592@tornado.gateway.2wire.net

I audited remaining get_element_type() callers. CheckAttributeType() needs to
recurse into domains over array types just like any other array type. Fixed
trivially in arrdom2v1-checkattr.patch; see its test case for an example hole.

nm

On Tue, May 24, 2011 at 2:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "David E. Wheeler" <david(at)kineticode(dot)com> writes:
>> On May 24, 2011, at 11:30 AM, Tom Lane wrote:
>>> I guess that the question that's immediately at hand is sort of a
>>> variant of that, because using a polymorphic function declared to take
>>> ANYARRAY on a domain-over-array really is using a portion of the base
>>> type's functionality.  What we've learned from bug #5717 and the
>>> subsequent issues is that using that base functionality without
>>> immediately abandoning the notion that the domain has some life of its
>>> own (ie, immediately casting to the base type) is harder than it looks.
>
>> Well, in the ANYELEMENT context (or ANYARRAY), what could be lost by "abandoning the notion that the domain has some life of its own"?
>
> I'm starting to think that maybe we should separate the two cases after
> all.  If we force a downcast for ANYARRAY matching, we will fix the loss
> of functionality induced by the bug #5717 patch, and it doesn't seem
> like anyone has a serious objection to that.  What to do for ANYELEMENT
> seems to be a bit more controversial, and at least some of the proposals
> aren't reasonable to do in 9.1 at this stage.  Maybe we should just
> leave ANYELEMENT as-is for the moment, and reconsider that issue later?

If we haven't lost any functionality with respect to ANYELEMENT in
9.1, then I don't think we ought to try to improve/change/break it in
9.1 either. But I do think we need to do something about ANYARRAY
matching, and your proposed fix seems pretty reasonable to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, May 24, 2011 at 2:54 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm starting to think that maybe we should separate the two cases after
>> all. If we force a downcast for ANYARRAY matching, we will fix the loss
>> of functionality induced by the bug #5717 patch, and it doesn't seem
>> like anyone has a serious objection to that. What to do for ANYELEMENT
>> seems to be a bit more controversial, and at least some of the proposals
>> aren't reasonable to do in 9.1 at this stage. Maybe we should just
>> leave ANYELEMENT as-is for the moment, and reconsider that issue later?

> If we haven't lost any functionality with respect to ANYELEMENT in
> 9.1, then I don't think we ought to try to improve/change/break it in
> 9.1 either. But I do think we need to do something about ANYARRAY
> matching, and your proposed fix seems pretty reasonable to me.

Yeah, the thread seems to have died off without anyone having a better
idea. I'll see about making this happen.

regards, tom lane

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Tue, May 24, 2011 at 02:00:54PM -0400, Noah Misch wrote:
>> I took your lack of any response as non-acceptance of the plan I outlined.
>> Alas, the wrong conclusion. I'll send a patch this week.

> See attached arrdom1v1-polymorphism.patch. This currently adds one syscache
> lookup per array_append, array_prepend or array_cat call when the anyarray
> type is not a domain. When the type is a domain, it adds a few more. We
> could add caching without too much trouble. I suppose someone out there uses
> these functions in bulk operations, though I've yet to see it. Is it worth
> optimizing this straightway?

I took another look at this, and I think I fundamentally don't agree
with the approach you're taking, quite aside from any implementation
difficulties or performance penalties. It makes more sense to me for
operations like array_cat to downcast their arguments to plain arrays.
If you then assign the result to a target of the domain-over-array type,
there will be an automatic upcast to the domain type, and then any
constraints on the domain will be rechecked at that time. If you don't,
well, it's an array value. The direction you want to go here makes as
little sense as arguing that

create domain pos as int check (value > 0);

select 2::pos - 42;

ought to fail because somehow the domain should override the result type
of the subtraction operator.

So I'm back to thinking that alternative #1 (downcast a domain to its
base array type when matching to an ANYARRAY argument) is the way to go.

> I audited remaining get_element_type() callers. CheckAttributeType() needs to
> recurse into domains over array types just like any other array type. Fixed
> trivially in arrdom2v1-checkattr.patch; see its test case for an example hole.

Yeah, that is a bug; I applied a slightly different patch for it.

I'm not convinced whether any of the get_element_type ->
get_base_element_type changes in your first patch are needed. When
I made the don't-expose-the-typelem change originally, I intentionally
created a separate function because I thought that most existing callers
shouldn't look through domain types. I'm not convinced that a
wholesale readjustment of those callers is appropriate.

regards, tom lane

On Thu, Jun 02, 2011 at 06:56:02PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > On Tue, May 24, 2011 at 02:00:54PM -0400, Noah Misch wrote:
> >> I took your lack of any response as non-acceptance of the plan I outlined.
> >> Alas, the wrong conclusion. I'll send a patch this week.
>
> > See attached arrdom1v1-polymorphism.patch. This currently adds one syscache
> > lookup per array_append, array_prepend or array_cat call when the anyarray
> > type is not a domain. When the type is a domain, it adds a few more. We
> > could add caching without too much trouble. I suppose someone out there uses
> > these functions in bulk operations, though I've yet to see it. Is it worth
> > optimizing this straightway?
>
> I took another look at this, and I think I fundamentally don't agree
> with the approach you're taking, quite aside from any implementation
> difficulties or performance penalties. It makes more sense to me for
> operations like array_cat to downcast their arguments to plain arrays.
> If you then assign the result to a target of the domain-over-array type,
> there will be an automatic upcast to the domain type, and then any
> constraints on the domain will be rechecked at that time. If you don't,
> well, it's an array value.

I don't doubt that's usable, and folks would find the behavior of their
applications acceptable given that approach. However, it's an arbitrary (from
the user's perspective) difference in behavior compared to the interaction of
polymorphic functions with domains over scalars. You did propose removing that
inconsistency, but that just builds up a fresh inconsistency between domains
over scalars and plain scalars. And for what gain apart from implementation
ease or performance improvements?

> The direction you want to go here makes as
> little sense as arguing that
>
> create domain pos as int check (value > 0);
>
> select 2::pos - 42;
>
> ought to fail because somehow the domain should override the result type
> of the subtraction operator.

I'm only contending for the behavior of polymorphic functions. We'll always
downcast a domain over int to use an (int, int) function, and we'll always
downcast a domain over int[] to use an (int[], int[]) function. The question is
whether we also downcast before using an (anyarray, anyarray) function. If you
update your example like this, it mirrors my argument:

create domain pos as int check (value > 0);
create function subany(anyelement, anyelement) returns anyelement
language internal as 'implementation_left_as_exercise';

select subany(2::pos, 42::pos);

That had better fail. Currently, whether it does so is up to the C code
implementing the function, just like I propose be the case for arrays.

> So I'm back to thinking that alternative #1 (downcast a domain to its
> base array type when matching to an ANYARRAY argument) is the way to go.
>
> > I audited remaining get_element_type() callers. CheckAttributeType() needs to
> > recurse into domains over array types just like any other array type. Fixed
> > trivially in arrdom2v1-checkattr.patch; see its test case for an example hole.
>
> Yeah, that is a bug; I applied a slightly different patch for it.

Looks better; thanks.

> I'm not convinced whether any of the get_element_type ->
> get_base_element_type changes in your first patch are needed. When
> I made the don't-expose-the-typelem change originally, I intentionally
> created a separate function because I thought that most existing callers
> shouldn't look through domain types. I'm not convinced that a
> wholesale readjustment of those callers is appropriate.

How would you restructure those checks, supposing you were writing a similar
patch to allow domains for the cases under examination at those call sites?
Every site I changed needed to comprehend domains over arrays, though doubtless
there are other ways to make them do so.

Thanks,
nm

On Thu, Jun 2, 2011 at 8:25 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> I don't doubt that's usable, and folks would find the behavior of their
> applications acceptable given that approach.  However, it's an arbitrary (from
> the user's perspective) difference in behavior compared to the interaction of
> polymorphic functions with domains over scalars.  You did propose removing that
> inconsistency, but that just builds up a fresh inconsistency between domains
> over scalars and plain scalars.  And for what gain apart from implementation
> ease or performance improvements?

Perhaps this is stating the obvious, but implementation ease and
performance improvements can sometimes be non-trivial benefits.

But in this case I really don't quite understand why you don't like
the proposed behavior. AIUI, the case we're talking about is a
function foo that takes, say, anyarray, and returns anyarray.

Now, let us say we attempt to call foo(bar), bar being a type name.
What happens? Well, in general, you get back an error saying that no
such function exists, because bar is not an array type. Certainly if
bar is int4 or box or a composite type or a domain over a scalar type,
we are done for. The function call does not match and nothing is left
to us to throw an error. The only way that this call can possibly
match is if bar happens to be a domain over an array type, and we
treat the call as a request to smash bar to the underlying array type.
And it seems that is the behavior people want. But once we've done
that, I don't see why we should then feel compelled to also insert a
cast in the other direction. In fact, doing so would seem totally
arbitrary and counterintuitive.

I mean, suppose bar is a domain over int[]. If the user had called
foo(some_bar_value::int[]), what should the return type be?
Presumably int[], no? If he called
foo(some_bar_value::int[]::numeric[]), then the return value should be
numeric[], I would think. So if he just calls foo(some_bar_value),
what should the return type be? Well, again, to make the function
call match at all, we have to insert a cast there... otherwise the
input is not of an array type and the call should just fail. And once
we've inserted the cast, then ISTM that we're bound to make the return
type match the cast we stuck in there.

I might be all wet here... but that's how it seems to me ATM.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jun 03, 2011 at 12:27:35AM -0400, Robert Haas wrote:
> But in this case I really don't quite understand why you don't like
> the proposed behavior. AIUI, the case we're talking about is a
> function foo that takes, say, anyarray, and returns anyarray.
>
> Now, let us say we attempt to call foo(bar), bar being a type name.
> What happens? Well, in general, you get back an error saying that no
> such function exists, because bar is not an array type. Certainly if
> bar is int4 or box or a composite type or a domain over a scalar type,
> we are done for. The function call does not match and nothing is left
> to us to throw an error. The only way that this call can possibly
> match is if bar happens to be a domain over an array type,

Correct so far.

> and we
> treat the call as a request to smash bar to the underlying array type.

No, there's no need to do that. The domain "is" an array, not merely something
that can be coerced to an array. Therefore, it can be chosen as the polymorphic
type directly. Indeed, all released versions do this.

> And it seems that is the behavior people want. But once we've done
> that, I don't see why we should then feel compelled to also insert a
> cast in the other direction. In fact, doing so would seem totally
> arbitrary and counterintuitive.

I agree. Both the argument and the return type need to be resolved the same
way, either both as the domain or both as the base array type.

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Fri, Jun 03, 2011 at 12:27:35AM -0400, Robert Haas wrote:
>> and we
>> treat the call as a request to smash bar to the underlying array type.

> No, there's no need to do that. The domain "is" an array, not merely
> something that can be coerced to an array. Therefore, it can be
> chosen as the polymorphic type directly. Indeed, all released
> versions do this.

No, it cannot "be chosen as the polymorphic type directly". The problem
with that is that there is no principled way to resolve ANYELEMENT
unless you consider that you have downcasted the domain to the array
type. You could perhaps ignore that problem for polymorphic functions
that use only ANYARRAY and not ANYELEMENT in their arguments and return
type --- but I'm not happy with the idea that that case would work
differently from a function that does use both.

So far as the other points you raise are concerned, I'm still of the
opinion that we might be best off to consider that domains should be
smashed to their base types when matching them to ANYELEMENT, too.
Again, if we don't do that, we have a problem with figuring out what
ANYARRAY ought to be (since we don't create an array type to go with a
domain). More generally, this dodges the whole problem of needing
polymorphic functions to enforce domain constraints, something I still
believe is entirely impractical from an implementation standpoint.

regards, tom lane

On Fri, Jun 3, 2011 at 1:14 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> No, there's no need to do that.  The domain "is" an array, not merely something
> that can be coerced to an array.  Therefore, it can be chosen as the polymorphic
> type directly.  Indeed, all released versions do this.

Well, as Bill Clinton once said, "it depends on what the meaning of
the word 'is' is". I think of array types in PostgreSQL as meaning
"the types whose monikers end in a pair of square brackets". We don't
in general have the ability to create a type that behaves "like"
another type. In particular, you can't create a user-defined type
that "is" an array in the same way that a domain-over-array "is" an
array. If we had some kind of type interface facility that might be
possible, but we don't.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jun 03, 2011 at 11:22:34AM -0400, Robert Haas wrote:
> On Fri, Jun 3, 2011 at 1:14 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > No, there's no need to do that.  The domain "is" an array, not merely something
> > that can be coerced to an array.  Therefore, it can be chosen as the polymorphic
> > type directly.  Indeed, all released versions do this.
>
> Well, as Bill Clinton once said, "it depends on what the meaning of
> the word 'is' is". I think of array types in PostgreSQL as meaning
> "the types whose monikers end in a pair of square brackets". We don't
> in general have the ability to create a type that behaves "like"
> another type. In particular, you can't create a user-defined type
> that "is" an array in the same way that a domain-over-array "is" an
> array. If we had some kind of type interface facility that might be
> possible, but we don't.
>
Early on in this thread, one of the users of domains-over-array-type
mentioned that he really didn't want to use them that way, he'd be
perfectly happy with array-over-domain: i.e.: mydomain[]. How does that
impact all this at the rhetorical level under discussion?

Ross
--
Ross Reedstrom, Ph.D. reedstrm(at)rice(dot)edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
Connexions http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE

On Jun 3, 2011, at 8:22 AM, Robert Haas wrote:

> Well, as Bill Clinton once said, "it depends on what the meaning of
> the word 'is' is". I think of array types in PostgreSQL as meaning
> "the types whose monikers end in a pair of square brackets".

Man, range types are going to fuck with your brainz.

D

On Fri, Jun 03, 2011 at 10:42:01AM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > On Fri, Jun 03, 2011 at 12:27:35AM -0400, Robert Haas wrote:
> >> and we
> >> treat the call as a request to smash bar to the underlying array type.
>
> > No, there's no need to do that. The domain "is" an array, not merely
> > something that can be coerced to an array. Therefore, it can be
> > chosen as the polymorphic type directly. Indeed, all released
> > versions do this.
>
> No, it cannot "be chosen as the polymorphic type directly".

This already happens today.

> The problem
> with that is that there is no principled way to resolve ANYELEMENT
> unless you consider that you have downcasted the domain to the array
> type.

I have nothing material to add to my statement in
http://archives.postgresql.org/message-id/20110511093217.GB26552@tornado.gateway.2wire.net

Let's be concrete; run this on 9.0:

CREATE DOMAIN foo AS int[];

SELECT pg_typeof(array_append('{1}'::foo, 1));
SELECT pg_typeof(array_prepend(1, '{1}'::foo));
SELECT pg_typeof(array_fill(1, array[3]));

Which of those type resolutions do you find unprincipled, or what hypothetical
function does 9.0 resolve in an unprincipled way? (What the function actually
does isn't important.)

> You could perhaps ignore that problem for polymorphic functions
> that use only ANYARRAY and not ANYELEMENT in their arguments and return
> type --- but I'm not happy with the idea that that case would work
> differently from a function that does use both.

It would be no good to bifurcate the rules that way.

> So far as the other points you raise are concerned, I'm still of the
> opinion that we might be best off to consider that domains should be
> smashed to their base types when matching them to ANYELEMENT, too.
> Again, if we don't do that, we have a problem with figuring out what
> ANYARRAY ought to be (since we don't create an array type to go with a
> domain).

Perhaps, though it paints us into a backward compatibility corner should we ever
support arrays of domains. How many field complaints has the longstanding error
outcome produced?

> More generally, this dodges the whole problem of needing
> polymorphic functions to enforce domain constraints, something I still
> believe is entirely impractical from an implementation standpoint.

I wrote about the implementation scope in
http://archives.postgresql.org/message-id/20110511191249.GA29592@tornado.gateway.2wire.net

While I don't doubt the presence of implementation challenges beyond those I
identified, we can't meaningfully discuss them as generalities.

nm

On Fri, Jun 3, 2011 at 1:00 PM, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> On Jun 3, 2011, at 8:22 AM, Robert Haas wrote:
>
>> Well, as Bill Clinton once said, "it depends on what the meaning of
>> the word 'is' is".  I think of array types in PostgreSQL as meaning
>> "the types whose monikers end in a pair of square brackets".
>
> Man, range types are going to fuck with your brainz.

They may, but probably not for this reason. Domain types have this
weird property that we want all of the base type operations to still
work on them, except when we don't want that. Range types won't have
that property, or at least I don't think so. Someone might expect
1::foo + 2::foo to work when foo is a domain over int, but they
probably won't expect '[1,2]'::intrange + '[2,3)'::intrange to work.
The real crux of the issue here is: under what circumstances should we
look through the domain wrapper around an underlying type, and under
what circumstances should we refrain from doing so?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> The real crux of the issue here is: under what circumstances should we
> look through the domain wrapper around an underlying type, and under
> what circumstances should we refrain from doing so?

That's half of it. The other half is: when we *do* look through the
wrapper, is that equivalent to having implicitly inserted a downcast
to the base type, so that the results are now indistinguishable from
having given a base-type value to begin with? Or is the expression's
behavior still affected by the fact of having given a domain value,
and if so, how exactly?

I assert that matching a domain-over-array to an ANYARRAY parameter
certainly involves having "looked through the wrapper". It's
considerably fuzzier though what should happen when matching a domain
to ANYELEMENT.

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> The real crux of the issue here is: under what circumstances
> should we look through the domain wrapper around an underlying
> type, and under what circumstances should we refrain from doing
> so?

I don't know if this is the intent of domains in the SQL standard,
but I find them useful to "extend" other types (in the
object-oriented sense). "CaseNoT" *is a* varchar(14), but not every
varchar(14) is a "CaseNoT". In my view, behaviors should be
determined based on that concept.

In the long run, it would be fabulous if a domain could extend
another domain -- or event better, multiple other domains with the
same base type. DOT hair color code and DOT eye color code domains
might both extend a DOT color codes domain.

Another long-range nicety would be something which I have seen in
some other databases, and which is consistent with the inheritance
theme, is that you can't compare or assign dissimilar domains -- an
error is thrown. So if you try to join from the eye color column in
a person table to the key of a hair color table, you get an error
unless you explicitly cast one or both of them to the common type.

I know that doesn't directly answer the question, but I think it
provides some framework for making choices....

-Kevin

BTW, a possibly relevant point here is that SQL:2008 says (in 4.12)

The purpose of a domain is to constrain the set of valid values
that can be stored in a column of a base table by various
operations.

and in 4.17.4

A domain constraint is a constraint that is specified for a domain.
It is applied to all columns that are based on that domain, and
to all values cast to that domain.

If you take that literally, it means that domain constraints are applied
(1) in an assignment to a table column of a domain type, and
(2) in an explicit CAST to the domain type, and
(3) nowhere else.

In particular I fail to see any support in the spec for the notion that
domain constraints should be applied to the results of expressions that
happen to include a domain value.

In our terms, that definitely suggests that domains should always be
implicitly downcast to base type when passed to polymorphic functions,
so that the result of the function is never automatically of the domain
type.

This all leads to the conclusion that domains are not first-class types
and can't be made so ...

regards, tom lane

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Another long-range nicety would be something which I have seen in
> some other databases, and which is consistent with the inheritance
> theme, is that you can't compare or assign dissimilar domains -- an
> error is thrown. So if you try to join from the eye color column in
> a person table to the key of a hair color table, you get an error
> unless you explicitly cast one or both of them to the common type.

[ raised eyebrow ... ] This is all pretty cute, but I think it goes
against both the letter and spirit of the SQL standard. What you
are describing might be a useful thing to have, but it isn't a SQL
domain.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> A domain constraint is a constraint that is specified for a
> domain.
> It is applied to all columns that are based on that domain, and
> to all values cast to that domain.
>
> If you take that literally, it means that domain constraints are
> applied
> (1) in an assignment to a table column of a domain type, and
> (2) in an explicit CAST to the domain type, and
> (3) nowhere else.

I'm curious how you jumped from "all values cast" to "explicit CAST"
and "nowhere else". The standard does describe implicit casts.

That said, section 4.7.5 talks about supertypes and subtypes, so if
I ever want such behavior, it seems to match pretty well (on a quick
scan) to what the standard outlines there. No need to invent
different mechanisms.

-Kevin

On fre, 2011-06-03 at 13:53 -0500, Kevin Grittner wrote:
> Another long-range nicety would be something which I have seen in
> some other databases, and which is consistent with the inheritance
> theme, is that you can't compare or assign dissimilar domains -- an
> error is thrown. So if you try to join from the eye color column in
> a person table to the key of a hair color table, you get an error
> unless you explicitly cast one or both of them to the common type.

What you are looking for is the SQL feature called "distinct types".
The makers of the SQL standard have sort of deprecated domains in favor
of distinct types, because distinct types address your sort of use case
better, and prescribing the behavior of domains becomes weirder and
weirder as the type system becomes more complex. Which is pretty much
the same experience we've been having over the years.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> What you are looking for is the SQL feature called "distinct types".
> The makers of the SQL standard have sort of deprecated domains in favor
> of distinct types, because distinct types address your sort of use case
> better, and prescribing the behavior of domains becomes weirder and
> weirder as the type system becomes more complex. Which is pretty much
> the same experience we've been having over the years.

Yeah ... the one thing that is actually completely broken about (our
current interpretation of) domains is that a first-class SQL datatype
cannot sanely have a NOT NULL constraint attached to it. That just
doesn't work in conjunction with outer joins, to take one glaring
example.

As I mentioned upthread, a closer look at the standard leads me to think
that the committee doesn't actually intend that a domain's constraints
follow it through operations --- I now think they only intend that the
constraints get checked in the context of a cast to the domain
(including assignment casts). In our terminology that would mean that a
domain gets downcast to its base type as soon as you do anything at all
to the value, even just pass it through a join.

There are certainly applications where such a behavior isn't what you
want, but trying to force domains to do something else is just not going
to lead to desirable results. It's better to invent some other concept,
and it sounds like the committee reached the same conclusion.

Anyway, I think we're out of time to do anything about the issue for
9.1. I think what we'd better do is force a downcast in the context
of matching to an ANYARRAY parameter, and leave the other cases to
revisit later.

regards, tom lane

I wrote:
> Anyway, I think we're out of time to do anything about the issue for
> 9.1. I think what we'd better do is force a downcast in the context
> of matching to an ANYARRAY parameter, and leave the other cases to
> revisit later.

Attached is a draft patch to do the above. It's only lightly tested,
and could use some regression test additions, but it seems to fix
Regina's complaint.

Note that I changed coerce_type's behavior for both ANYARRAY and ANYENUM
targets, but the latter behavioral change is unreachable since the other
routines in parse_coerce.c will not match a domain-over-enum to ANYENUM.
I am half tempted to extend the patch so they will, which would allow
cases like this to work:

regression=# create type color as enum('red','green','blue');
CREATE TYPE
regression=# select enum_first('green'::color);
enum_first
------------
red
(1 row)

regression=# create domain dcolor as color;
CREATE DOMAIN
regression=# select enum_first('green'::dcolor);
ERROR: function enum_first(dcolor) does not exist
LINE 1: select enum_first('green'::dcolor);
^
HINT: No function matches the given name and argument types. You might need to add explicit type casts.

I'm unsure though if there's any support for this further adventure,
since it wouldn't be fixing a 9.1 regression.

Comments?

regards, tom lane

On Tue, Jun 7, 2011 at 6:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Anyway, I think we're out of time to do anything about the issue for
>> 9.1.  I think what we'd better do is force a downcast in the context
>> of matching to an ANYARRAY parameter, and leave the other cases to
>> revisit later.
>
> Attached is a draft patch to do the above.  It's only lightly tested,
> and could use some regression test additions, but it seems to fix
> Regina's complaint.
>
> Note that I changed coerce_type's behavior for both ANYARRAY and ANYENUM
> targets, but the latter behavioral change is unreachable since the other
> routines in parse_coerce.c will not match a domain-over-enum to ANYENUM.
> I am half tempted to extend the patch so they will, which would allow
> cases like this to work:
>
> regression=#  create type color as enum('red','green','blue');
> CREATE TYPE
> regression=# select enum_first('green'::color);
>  enum_first
> ------------
>  red
> (1 row)
>
> regression=# create domain dcolor as color;
> CREATE DOMAIN
> regression=# select enum_first('green'::dcolor);
> ERROR:  function enum_first(dcolor) does not exist
> LINE 1: select enum_first('green'::dcolor);
>               ^
> HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
>
> I'm unsure though if there's any support for this further adventure,
> since it wouldn't be fixing a 9.1 regression.
>
> Comments?

Well, on the one hand, if we're doing it for arrays, it's hard to
imagine that the same behavior for enums can be an outright disaster.
On the flip side, people get reeeeeally cranky about changes that
break application code, so it would not be nice if we had to pull this
one back. How likely is that?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jun 7, 2011 at 6:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Note that I changed coerce_type's behavior for both ANYARRAY and ANYENUM
>> targets, but the latter behavioral change is unreachable since the other
>> routines in parse_coerce.c will not match a domain-over-enum to ANYENUM.
>> I am half tempted to extend the patch so they will, which would allow
>> cases like this to work:
>>
>> regression=# select enum_first('green'::dcolor);
>> ERROR: function enum_first(dcolor) does not exist

> Well, on the one hand, if we're doing it for arrays, it's hard to
> imagine that the same behavior for enums can be an outright disaster.
> On the flip side, people get reeeeeally cranky about changes that
> break application code, so it would not be nice if we had to pull this
> one back. How likely is that?

It's hard to see how allowing this match where there was no match before
would break existing code. A more plausible objection is that we'd be
foreclosing any possibility of handling the match-domain-to-ANYENUM case
differently, since once 9.1 had been out in the field doing this for a
year, you can be sure there *would* be some apps depending on it.
So I think the real question is whether we have totally destroyed the
argument for letting domains pass through polymorphic functions without
getting smashed to their base types. Personally I think that idea is
pretty much dead in the water, but I sense that Noah hasn't given up on
it yet ;-) If we aren't yet willing to treat ANYELEMENT that way, maybe
it's premature to adopt the stance for ANYENUM.

regards, tom lane

On Tue, Jun 7, 2011 at 9:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Jun 7, 2011 at 6:28 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Note that I changed coerce_type's behavior for both ANYARRAY and ANYENUM
>>> targets, but the latter behavioral change is unreachable since the other
>>> routines in parse_coerce.c will not match a domain-over-enum to ANYENUM.
>>> I am half tempted to extend the patch so they will, which would allow
>>> cases like this to work:
>>>
>>> regression=# select enum_first('green'::dcolor);
>>> ERROR:  function enum_first(dcolor) does not exist
>
>> Well, on the one hand, if we're doing it for arrays, it's hard to
>> imagine that the same behavior for enums can be an outright disaster.
>> On the flip side, people get reeeeeally cranky about changes that
>> break application code, so it would not be nice if we had to pull this
>> one back.  How likely is that?
>
> It's hard to see how allowing this match where there was no match before
> would break existing code.  A more plausible objection is that we'd be
> foreclosing any possibility of handling the match-domain-to-ANYENUM case
> differently, since once 9.1 had been out in the field doing this for a
> year, you can be sure there *would* be some apps depending on it.

Yes, that's the point I was trying to get at.

> So I think the real question is whether we have totally destroyed the
> argument for letting domains pass through polymorphic functions without
> getting smashed to their base types.  Personally I think that idea is
> pretty much dead in the water, but I sense that Noah hasn't given up on
> it yet ;-)  If we aren't yet willing to treat ANYELEMENT that way, maybe
> it's premature to adopt the stance for ANYENUM.

Given that we have no field demand for this behavior, maybe it's
better not to add it, so that we have the option later to change our
mind about how it should work.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I wrote:
>> Anyway, I think we're out of time to do anything about the issue for
>> 9.1. I think what we'd better do is force a downcast in the context
>> of matching to an ANYARRAY parameter, and leave the other cases to
>> revisit later.

> Attached is a draft patch to do the above. It's only lightly tested,
> and could use some regression test additions, but it seems to fix
> Regina's complaint.

Applied with regression-test additions.

regards, tom lane