Re: Domains and arrays and composites, oh my

I wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> On 09/28/2017 01:02 PM, Tom Lane wrote:
>>>> I do think that treating a function returning a domain-over-composite
>>>> differently from one returning a base composite is a POLA. We'd be very
>>>> hard put to explain the reasons for it to an end user.

>>> Do you have any thoughts about how we ought to resolve that?

>> Not offhand. Maybe we need to revisit the decision not to modify the
>> executor at all.

> I think it's more of a parse analysis change: the issue is whether to
> smash a function's result type to base when determining whether it emits
> columns.  Maybe we could just do that in that context, and otherwise leave
> domains alone.

After fooling with that for awhile, I concluded that the only reasonable
path forward is to go ahead and modify the behavior of
get_expr_result_type and sibling routines.  While this fixes the parser
behavior to be pretty much what I think we want, it means that we've got
holes to fill in a lot of other places.  Most of them will manifest as
unexpected "domaintypename is not a composite type" errors, but there
are definitely places where the net effect is to silently fail to enforce
domain constraints against a constructed row value :-(.  In the attached
still-WIP patch, I think that I've got most of the core code fixed, but
there are at least these holes remaining to fill:

* json_populate_record and sibling routines won't enforce domain
constraints; depending on how they're called, you might or might not
get a "not a composite type" error.  This is because they use two
different methods for getting the target type OID depending on whether
the input prototype record is NULL.  Maybe that was a bad idea.
(I'm disinclined to try to fix this code right now since there are
pending bug fixes nearby; better to wait till that dust settles.)

* Ditto for hstore's populate_record, which is pretty much same logic.

* plpgsql mostly seems to work, but not quite 100%: RETURN QUERY will
fail to enforce domain constraints if the return type is domain over
composite.  It also still needs feature extension to handle d-over-c
variables more fully (e.g. allow field assignment).

* I haven't looked at the other PLs much; I believe they will mostly
fail safe with "not a composite type" errors, but I wouldn't swear
that all code paths will.

It seems like this is probably the way forward, but I'm slightly
discouraged by the fact that the patch footprint is getting bigger
and there are paths where we can get domain-enforcement omissions
rather than something more benign.  Still, we had lots of
domain-enforcement omissions in the early days of the existing
domain feature, if memory serves.  Maybe we should just accept
that working through that will be a process.

>> One thought I had was that we could invent a new return
>> type of TYPEFUNC_DOMAIN_COMPOSITE so there would be less danger of a PL
>> just treating it as an unconstrained base type as it might do if it saw
>> TYPEFUNC_COMPOSITE.

> Hmm.  That would be a way of forcing the issue, no doubt ...

I did that, but it turns out not to help much; turns out a lot of the
broken code is doing stuff on the basis of type_is_rowtype(), which
this patch allows to return true for domains over composite.  Maybe
we should undo that and invent a separate type_is_rowtype_or_domain()
function to be used only by repaired code, but that seems pretty ugly :-(

Anyway, PFA an updated patch that also fixes some conflicts with the
already-committed arrays-of-domains patch.

			regards, tom lane