Re: [HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

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

I realized that the pending patch for jsonb_build_object doesn't
actually have any conflict with what I needed to touch here, so
I went ahead and fixed the JSON functions that needed fixing,
along with hstore's populate_record.  I ended up rewriting the
argument-metadata-collection portions of populate_record_worker
and populate_recordset_worker rather heavily, because I didn't
like them at all: aside from not working for domains over composite,
they were pretty inefficient (redoing a lot of work on each call
for no good reason) and they were randomly different from each
other, resulting in json{b}_populate_recordset rejecting some cases
that worked in json{b}_populate_record.

I've also updated the documentation.

I think that this patch version is done so far as the core code
and contrib are concerned.  The PLs need varying amounts of work,
but as I said earlier, I think it would be better to tackle those
in separate patches instead of continuing to enlarge the footprint
of the core patch.  So, barring objection, I'd like to go ahead
and commit this.

regards, tom lane

diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index d828401..e999a8e 100644
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
*** typedef struct RecordIOData
*** 752,757 
--- 752,759 
  {
  	Oid			record_type;
  	int32		record_typmod;
+ 	/* this field is used only if target type is domain over composite: */
+ 	void	   *domain_info;	/* opaque cache for domain checks */
  	int			ncolumns;
  	ColumnIOData columns[FLEXIBLE_ARRAY_MEMBER];
  } RecordIOData;
*** hstore_from_record(PG_FUNCTION_ARGS)
*** 780,788 
  		Oid			argtype = get_fn_expr_argtype(fcinfo->flinfo, 0);
  
  		/*
! 		 * have no tuple to look at, so the only source of type info is the
! 		 * argtype. The lookup_rowtype_tupdesc call below will error out if we
! 		 * don't have a known composite type oid here.
  		 */
  		tupType = argtype;
  		tupTypmod = -1;
--- 782,792 
  		Oid			argtype = get_fn_expr_argtype(fcinfo->flinfo, 0);
  
  		/*
! 		 * We have no tuple to look at, so the only source of type info is the
! 		 * argtype --- which might be domain over composite, but we don't care
! 		 * here, since we have no need to be concerned about domain
! 		 * constraints.  The lookup_rowtype_tupdesc_domain call below will
! 		 * error out if we don't have a known composite type oid here.
  		 */
  		tupType = argtype;
  		tupTypmod = -1;
*** hstore_from_record(PG_FUNCTION_ARGS)
*** 793,804 
  	{
  		rec = PG_GETARG_HEAPTUPLEHEADER(0);
  
! 		/* Extract type info from the tuple itself */
  		tupType = HeapTupleHeaderGetTypeId(rec);
  		tupTypmod = HeapTupleHeaderGetTypMod(rec);
  	}
  
! 	tupdesc = lookup_rowtype_tupdesc(tupType, tupTypmod);
  	ncolumns = tupdesc->natts;
  
  	/*
--- 797,811 
  	{
  		rec = PG_GETARG_HEAPTUPLEHEADER(0);
  
! 		/*
! 		 * Extract type info from the tuple itself -- this will work even for
! 		 * anonymous record types.
! 		 */
  		tupType = HeapTupleHeaderGetTypeId(rec);
  		tupTypmod = HeapTupleHeaderGetTypMod(rec);
  	}
  
! 	tupdesc = lookup_rowtype_tupdesc_domain(tupType, tupTypmod, false);
  	ncolumns = tupdesc->natts;
  
  	/*
*** hstore_populate_record(PG_FUNCTION_ARGS)
*** 943,951 
  		rec = NULL;
  
  		/*
! 		 * have no tuple to look at, so the only source of type info is the
! 		 * argtype. The lookup_rowtype_tupdesc call below will error out if we
! 		 * don't have a known composite type oid here.
  		 */
  		tupType = argtype;
  		tupTypmod = -1;
--- 950,958 
  		rec = NULL;
  
  		/*
! 		 * We have no tuple to look at, so the only source of type info is the
! 		 * argtype.  The lookup_rowtype_tupdesc_domain call below will error
! 		 * out if we don't have a known composite type oid here.
  		 */
  		tupType = argtype;
  		tupTypmod = -1;
*** hstore_populate_record(PG_FUNCTION_ARGS)
*** 957,963 
  		if (PG_ARGISNULL(1))
  			PG_RETURN_POINTER(rec);
  
! 		/* Extract type info from the tuple itself */
  		tupType = HeapTupleHeaderGetTypeId(rec);
  		tupTypmod = HeapTupleHeaderGetTypMod(rec);
  	}
--- 964,973 
  		if (PG_ARGISNULL(1))
  			PG_RETURN_POINTER(rec);
  
! 		/*
! 		 * Extract type info from the tuple itself -- this will work even for
! 		 * anonymous record types.
! 		 */
  		tupType = HeapTupleHeaderGetTypeId(rec);
  		tupTypmod = HeapTupleHeaderGetTypMod(rec);
  	}
*** hstore_populate_record(PG_FUNCTION_ARGS)
*** 975,981 
  	if (HS_COUNT(hs) == 0 && rec)
  		PG_RETURN_POINTER(rec);
  
! 	tupdesc = lookup_rowtype_tupdesc(tupType, tupTypmod);
  	ncolumns = tupdesc->natts;
  
  	if (rec)
--- 985,995 
  	if (HS_COUNT(hs) == 0 && rec)
  		PG_RETURN_POINTER(rec);
  
! 	/*
! 	 * Lookup the input record's tupdesc.  For the moment, 

Re: [HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

I wrote:
> Andrew Dunstan  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

diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index 245a374..1bd8a58 100644
*** a/src/backend/catalog/pg_inherits.c
--- b/src/backend/catalog/pg_inherits.c
*** has_superclass(Oid relationId)
*** 301,306 
--- 301,311 
  /*
   * Given two type OIDs, determine whether the first is a complex type
   * (class type) that inherits from the second.
+  *
+  * This essentially asks whether the first type is guaranteed to be coercible
+  * to the second.  Therefore, we allow the first type to be a domain over a
+  * complex type that inherits from the second; that creates no difficulties.
+  * But the second type cannot be a domain.
   */
  bool
  typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId)
*** typeInheritsFrom(Oid subclassTypeId, Oid
*** 314,322 
  	ListCell   *queue_item;
  
  	/* We need to work with the associated relation OIDs */
! 	subclassRelid = typeidTypeRelid(subclassTypeId);
  	if (subclassRelid == InvalidOid)
! 		return false;			/* not a complex type */
  	superclassRelid = typeidTypeRelid(superclassTypeId);
  	if (superclassRelid == InvalidOid)
  		return false;			/* not a complex type */
--- 319,327 
  	ListCell   *queue_item;
  
  	/* We need to work with the associated relation OIDs */
! 	subclassRelid = typeOrDomainTypeRelid(subclassTypeId);
  	if (subclassRelid == 

Re: [HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

Andrew Dunstan  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.

> 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 ...

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

Re: [HACKERS] Domains and arrays and composites, oh my

[Andrew Dunstan]

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. Obviously that would make the patch a good deal more
invasive ;-(  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.

Maybe I'm wrong, but I have a strong suspicion that of we leave it like
this now we'll regret it in the future.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



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

Re: [HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

Andrew Dunstan  writes:
> On 07/13/2017 03:19 PM, Tom Lane wrote:
>> Attached is a draft patch that allows domains over composite types.
>> I think it's probably complete on its own terms, but there are some
>> questions around behavior of functions returning domain-over-composite
>> that could use discussion, and some of the PLs need some follow-on work.

> This is a pretty nice patch, and very small indeed all things
> considered. From a code point of view I have no criticism, although
> maybe we need to be a bit more emphatic in the header file comments
> about the unwisdom of using get_expr_result_tupdesc().

Thanks for reviewing!

> 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?

> I also think we shouldn't commit this until we have accompanying patches
> for the core PLs, at least for plpgsql but I bet there are things that
> should be fixed for the others too.

For my own part, I think it would be reasonable to commit the core patch
once we've resolved the question of what to do with the case of
function-in-FROM returning domain over composite.  That's core parser
behavior so it should be part of the same patch.  I think addressing
each PL separately in followon patches would be fine and would help to
avoid the giant-unreviewable-patch syndrome.  It is important to get
all the related work done in one release cycle, but since we're just
starting v11 I'm not too worried about that.

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

Re: [HACKERS] Domains and arrays and composites, oh my

[Andrew Dunstan]

On 07/13/2017 03:19 PM, Tom Lane wrote:
> I wrote:
>> I started to look into allowing domains over composite types, which is
>> another never-implemented case that there's no very good reason not to
>> allow.  Well, other than the argument that the SQL standard only allows
>> domains over "predefined" (built-in) types ... but we blew past that
>> restriction ages ago.
> Attached is a draft patch that allows domains over composite types.
> I think it's probably complete on its own terms, but there are some
> questions around behavior of functions returning domain-over-composite
> that could use discussion, and some of the PLs need some follow-on work.
>
> The core principle here was to not modify execution-time behavior by
> adding domain checks to existing code paths.  Rather, I wanted the
> parser to insert CoerceToDomain nodes wherever checks would be needed.
> Row-returning node types such as RowExpr and FieldStore only return
> regular composite types, as before; to generate a value of a composite
> domain, we construct a value of the base type and then CoerceToDomain.
> (This is pretty analogous to what happens for domains over arrays.)
> Whole-row Vars can only have regular composite types as vartype,
> TupleDescs can only have regular composite types as tdtypeid, composite
> Datums only have regular composite type OIDs in datum_typeid.  (The last
> would be expected anyway, since the physical representation of a domain
> value is never different from that of its base type.)
>
> Doing it that way led to a nicely small patch, only about 160 net new
> lines of code.  However, not touching the executor meant not touching
> the behavior of functions that return domains, even if the domain is
> domain-over-composite.  In code terms this means that 
> get_call_result_type() and sibling functions will return TYPEFUNC_SCALAR
> not TYPEFUNC_COMPOSITE for such a result type.  The difficulty with
> changing that is that if these functions look through the domain, then
> the calling code (in, usually, a PL) will simply build and return a result
> of the underlying composite type, failing to apply any domain constraints.
> Trying to get out-of-core PLs on board with a change in those requirements
> seems like a risky proposition.
>
> Concretely, consider
>
> create type complex as (r float8, i float8);
> create domain dcomplex as complex;
>
> You can make a SQL-language function to return complex in either of two
> ways:
>
> create function fc() returns complex language sql
> as 'select 1.0::float8, 2.0::float8';
>
> create function fc() returns complex language sql
> as 'select row(1.0::float8, 2.0::float8)::complex';
>
> As the patch stands, though, only the second way works for domains over
> composite:
>
> regression=# create function fdc() returns dcomplex language sql
> as 'select 1.0::float8, 2.0::float8';
> ERROR:  return type mismatch in function declared to return dcomplex
> DETAIL:  Final statement must return exactly one column.
> CONTEXT:  SQL function "fdc"
> regression=# create function fdc() returns dcomplex language sql
> as 'select row(1.0::float8, 2.0)::dcomplex';
> CREATE FUNCTION
>
> Now, maybe that's fine.  SQL-language functions have never been very
> willing to insert implicit casts to get to the function result type,
> and certainly the only way that the first definition could be legal
> is if there were an implicit up-cast to the domain type.  It might be
> OK to just leave it like this, though some documentation about it
> would be a good idea.
>
> plpgsql functions seem generally okay as far as composite domain return
> types go, because they don't have anything corresponding to the row
> return convention of SQL functions.  And plpgsql's greater willingness
> to do implicit coercions reduces the notational burden, too.  But
> there's some work yet to be done to get plpgsql to realize that
> composite domain local variables have substructure.  For example,
> this works:
>
>   declare x complex;
>   ...
>   x.r := 1;
>
> but it fails if x is dcomplex.  But ISTM that that would be better
> handled as a followon feature patch.  I suspect that the other PLs may
> have similar issues where it'd be nice to allow domain-over-composite
> to act like a plain composite for specific purposes; but I've not looked.
>
> Another issue related to function result types is that the parser
> considers a function-in-FROM returning a composite domain to be
> producing a scalar result not a rowtype.  Thus you get this for a
> function returning complex:
>
> regression=# select * from fc();
>  r | i 
> ---+---
>  1 | 2
> (1 row)
>
> but this for a function returning dcomplex:
>
> regression=# select * from fdc();
>   fdc  
> ---
>  (1,2)
> (1 row)
>
> I think that that could be changed with only local changes in parse
> analysis, but do we want to change it?  Arguably, making fdc() act the
> same as fc() here would amount to implicitly downcasting the domain to
> its base type.  But doing 

Re: [HACKERS] Domains and arrays and composites, oh my

[Robert Haas]

On Thu, Jul 13, 2017 at 3:42 PM, Tom Lane  wrote:
> Yeah, it does, although I'm not sure how intuitive it is that the
> parentheses are significant ...
>
> regression=# select fdc.* from fdc();
>   fdc
> ---
>  (1,2)
> (1 row)
>
> regression=# select (fdc).* from fdc();
>  r | i
> ---+---
>  1 | 2
> (1 row)

Not intuitive at all.

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


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

Re: [HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

"David G. Johnston"  writes:
> On Thursday, July 13, 2017, Tom Lane  wrote:
>> regression=# select * from fdc();
>> fdc
>> ---
>> (1,2)
>> (1 row)

> Select (fdc).* from fdc();  is considerably more intuitive that the cast.
> Does that give the expected multi-column result?

Yeah, it does, although I'm not sure how intuitive it is that the
parentheses are significant ...

regression=#  select * from fdc();
  fdc  
---
 (1,2)
(1 row)

regression=# select fdc from fdc();
  fdc  
---
 (1,2)
(1 row)

regression=# select fdc.* from fdc();
  fdc  
---
 (1,2)
(1 row)

regression=# select (fdc).* from fdc();
 r | i 
---+---
 1 | 2
(1 row)

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

Re: [HACKERS] Domains and arrays and composites, oh my

[David G. Johnston]

On Thursday, July 13, 2017, Tom Lane  wrote:
>
> regression=# select * from fdc();
>   fdc
> ---
>  (1,2)
> (1 row)
>
>
Select (fdc).* from fdc();  is considerably more intuitive that the cast.
Does that give the expected multi-column result?

David J.

Re: [HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

I wrote:
> I started to look into allowing domains over composite types, which is
> another never-implemented case that there's no very good reason not to
> allow.  Well, other than the argument that the SQL standard only allows
> domains over "predefined" (built-in) types ... but we blew past that
> restriction ages ago.

Attached is a draft patch that allows domains over composite types.
I think it's probably complete on its own terms, but there are some
questions around behavior of functions returning domain-over-composite
that could use discussion, and some of the PLs need some follow-on work.

The core principle here was to not modify execution-time behavior by
adding domain checks to existing code paths.  Rather, I wanted the
parser to insert CoerceToDomain nodes wherever checks would be needed.
Row-returning node types such as RowExpr and FieldStore only return
regular composite types, as before; to generate a value of a composite
domain, we construct a value of the base type and then CoerceToDomain.
(This is pretty analogous to what happens for domains over arrays.)
Whole-row Vars can only have regular composite types as vartype,
TupleDescs can only have regular composite types as tdtypeid, composite
Datums only have regular composite type OIDs in datum_typeid.  (The last
would be expected anyway, since the physical representation of a domain
value is never different from that of its base type.)

Doing it that way led to a nicely small patch, only about 160 net new
lines of code.  However, not touching the executor meant not touching
the behavior of functions that return domains, even if the domain is
domain-over-composite.  In code terms this means that 
get_call_result_type() and sibling functions will return TYPEFUNC_SCALAR
not TYPEFUNC_COMPOSITE for such a result type.  The difficulty with
changing that is that if these functions look through the domain, then
the calling code (in, usually, a PL) will simply build and return a result
of the underlying composite type, failing to apply any domain constraints.
Trying to get out-of-core PLs on board with a change in those requirements
seems like a risky proposition.

Concretely, consider

create type complex as (r float8, i float8);
create domain dcomplex as complex;

You can make a SQL-language function to return complex in either of two
ways:

create function fc() returns complex language sql
as 'select 1.0::float8, 2.0::float8';

create function fc() returns complex language sql
as 'select row(1.0::float8, 2.0::float8)::complex';

As the patch stands, though, only the second way works for domains over
composite:

regression=# create function fdc() returns dcomplex language sql
as 'select 1.0::float8, 2.0::float8';
ERROR:  return type mismatch in function declared to return dcomplex
DETAIL:  Final statement must return exactly one column.
CONTEXT:  SQL function "fdc"
regression=# create function fdc() returns dcomplex language sql
as 'select row(1.0::float8, 2.0)::dcomplex';
CREATE FUNCTION

Now, maybe that's fine.  SQL-language functions have never been very
willing to insert implicit casts to get to the function result type,
and certainly the only way that the first definition could be legal
is if there were an implicit up-cast to the domain type.  It might be
OK to just leave it like this, though some documentation about it
would be a good idea.

plpgsql functions seem generally okay as far as composite domain return
types go, because they don't have anything corresponding to the row
return convention of SQL functions.  And plpgsql's greater willingness
to do implicit coercions reduces the notational burden, too.  But
there's some work yet to be done to get plpgsql to realize that
composite domain local variables have substructure.  For example,
this works:

declare x complex;
...
x.r := 1;

but it fails if x is dcomplex.  But ISTM that that would be better
handled as a followon feature patch.  I suspect that the other PLs may
have similar issues where it'd be nice to allow domain-over-composite
to act like a plain composite for specific purposes; but I've not looked.

Another issue related to function result types is that the parser
considers a function-in-FROM returning a composite domain to be
producing a scalar result not a rowtype.  Thus you get this for a
function returning complex:

regression=# select * from fc();
 r | i 
---+---
 1 | 2
(1 row)

but this for a function returning dcomplex:

regression=# select * from fdc();
  fdc  
---
 (1,2)
(1 row)

I think that that could be changed with only local changes in parse
analysis, but do we want to change it?  Arguably, making fdc() act the
same as fc() here would amount to implicitly downcasting the domain to
its base type.  But doing so here is optional, not necessary in order to
make the statement sane at all, and it's arguable that we shouldn't do
that if the user didn't tell us to.  A user who does want that to happen
can downcast explicitly:

regression=# 

[HACKERS] Domains and arrays and composites, oh my

[Tom Lane]

I started to look into allowing domains over composite types, which is
another never-implemented case that there's no very good reason not to
allow.  Well, other than the argument that the SQL standard only allows
domains over "predefined" (built-in) types ... but we blew past that
restriction ages ago.

Any thought that there might be some fundamental problem with that was
soon dispelled when I noticed that we allow domains over arrays of
composite types.  Ahem.

They even work, mostly.  I wrote a few test cases and couldn't find
anything that failed except for attempts to insert or update multiple
subfields of the same base column; that's because process_matched_tle()
fails to look through CoerceToDomain nodes.  But that turns out to be a
bug even for the simpler case of domains over arrays of scalars, which
is certainly supported.  That is, given

create domain domainint4arr int4[];
create table domarrtest (testint4arr domainint4arr);

this ought to work:

insert into domarrtest (testint4arr[1], testint4arr[3]) values (11,22);

It would work with a plain-array target column, but with the domain
it fails with

ERROR:  multiple assignments to same column "testint4arr"

Hence, the attached proposed patch, which fixes that oversight and
adds some relevant test cases.  (I've not yet looked to see if any
documentation changes would be appropriate.)

I think this is a bug fix and should be back-patched.  Any objections?

regards, tom lane

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index f3c7526..0a6f4f3 100644
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*** process_matched_tle(TargetEntry *src_tle
*** 934,939 
--- 934,940 
  	const char *attrName)
  {
  	TargetEntry *result;
+ 	CoerceToDomain *coerce_expr = NULL;
  	Node	   *src_expr;
  	Node	   *prior_expr;
  	Node	   *src_input;
*** process_matched_tle(TargetEntry *src_tle
*** 970,979 
--- 971,1000 
  	 * For FieldStore, instead of nesting we can generate a single
  	 * FieldStore with multiple target fields.  We must nest when
  	 * ArrayRefs are involved though.
+ 	 *
+ 	 * As a further complication, the destination column might be a domain,
+ 	 * resulting in each assignment containing a CoerceToDomain node over a
+ 	 * FieldStore or ArrayRef.  These should have matching target domains, so
+ 	 * we strip them and reconstitute a single CoerceToDomain over the
+ 	 * combined FieldStore/ArrayRef nodes.  (Notice that this has the result
+ 	 * that the domain's checks are applied only after we do all the field or
+ 	 * element updates, not after each one.  This is arguably desirable.)
  	 *--
  	 */
  	src_expr = (Node *) src_tle->expr;
  	prior_expr = (Node *) prior_tle->expr;
+ 
+ 	if (src_expr && IsA(src_expr, CoerceToDomain) &&
+ 		prior_expr && IsA(prior_expr, CoerceToDomain) &&
+ 		((CoerceToDomain *) src_expr)->resulttype ==
+ 		((CoerceToDomain *) prior_expr)->resulttype)
+ 	{
+ 		/* we assume without checking that resulttypmod/resultcollid match */
+ 		coerce_expr = (CoerceToDomain *) src_expr;
+ 		src_expr = (Node *) ((CoerceToDomain *) src_expr)->arg;
+ 		prior_expr = (Node *) ((CoerceToDomain *) prior_expr)->arg;
+ 	}
+ 
  	src_input = get_assignment_input(src_expr);
  	prior_input = get_assignment_input(prior_expr);
  	if (src_input == NULL ||
*** process_matched_tle(TargetEntry *src_tle
*** 1042,1047 
--- 1063,1078 
  		newexpr = NULL;
  	}
  
+ 	if (coerce_expr)
+ 	{
+ 		/* put back the CoerceToDomain */
+ 		CoerceToDomain *newcoerce = makeNode(CoerceToDomain);
+ 
+ 		memcpy(newcoerce, coerce_expr, sizeof(CoerceToDomain));
+ 		newcoerce->arg = (Expr *) newexpr;
+ 		newexpr = (Node *) newcoerce;
+ 	}
+ 
  	result = flatCopyTargetEntry(src_tle);
  	result->expr = (Expr *) newexpr;
  	return result;
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 41b70e2..9fb750d 100644
*** a/src/test/regress/expected/domain.out
--- b/src/test/regress/expected/domain.out
*** INSERT INTO domarrtest values ('{2,2}', 
*** 107,112 
--- 107,113 
  INSERT INTO domarrtest values (NULL, '{{"a","b","c"},{"d","e","f"}}');
  INSERT INTO domarrtest values (NULL, '{{"toolong","b","c"},{"d","e","f"}}');
  ERROR:  value too long for type character varying(4)
+ INSERT INTO domarrtest (testint4arr[1], testint4arr[3]) values (11,22);
  select * from domarrtest;
testint4arr  |testchar4arr 
  ---+-
*** select * from domarrtest;
*** 115,121 
   {2,2} | {{a,b},{c,d},{e,f}}
   {2,2} | {{a},{c}}
 | {{a,b,c},{d,e,f}}
! (5 rows)
  
  select testint4arr[1], testchar4arr[2:2] from domarrtest;
   testint4arr | testchar4arr 
--- 116,123 
   {2,2} | {{a,b},{c,d},{e,f}}
   {2,2} | {{a},{c}}
 | {{a,b,c},{d,e,f}}
!