Re: Domains versus polymorphic functions, redux

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