Thread: Check constraint on domain over an array not executed for array literals

Hi

While trying to create a domain over an array type to enforce a certain
shape or certain contents of an array (like the array being only
one-dimensional or not containing NULLs), I've stumbled over what I
believe to be a bug in postgresql 8.4

It seems that check constraints on domains are *not* executed for
literals of the domain-over-array-type - in other words, for expressions
like:
array[...]::<my-domain-over-array-type>.

They are, however, executed if I first force the array to be of the base
type, and then cast it to the array type.

Here is an example that reproduces the problem:
----------------------------------------
create domain myintarray as int[] check (  -- Check that the array is neither null, nor empty,  -- nor
multi-dimensional (value is not null) and  (array_length(value,1) is not null) and  (array_length(value,1) > 0) and
(array_length(value,2)is null)
 
);

select null::myintarray; -- Fails (Right)

select array[]::myintarray; -- Succeeds (Wrong)
select array[]::int[]::myintarray; -- Fails (Right)

select array[1]::myintarray; -- Succeeds (Right)
select array[1]::int[]::myintarray; -- Succeeds (Right)

select array[array[1]]::myintarray; -- Succeeds (Wrong)
select array[array[1]]::int[][]::myintarray; -- Fails (Right)
----------------------------------------

I guess the reason is that the "::arraytype" part of
"array[...]::arraytype" isn't really a cast at all, but instead part of
the array literal syntax. Hence, array[]::myintarray probably creates an
empty myintarray instance, and then adds the elements between the square
brackets (none) - with none of this steps triggering a run of the check
constraint.

I still have the feeling that this a bug, though. First, because it
leaves you with no way at guarantee that values of a given domain always
fulfill certain constraints. And second because "array[...]::arraytype"
at least *looks* like a cast, and hence should behave like one too.

best regards,
Florian Pflug

Florian G. Pflug wrote:
> While trying to create a domain over an array type to enforce a certain
> shape or certain contents of an array (like the array being only
> one-dimensional or not containing NULLs), I've stumbled over what I
> believe to be a bug in postgresql 8.4
>
> It seems that check constraints on domains are *not* executed for
> literals of the domain-over-array-type - in other words, for expressions
> like:
> array[...]::<my-domain-over-array-type>.
>
> They are, however, executed if I first force the array to be of the base
> type, and then cast it to the array type.
> ...
> I still have the feeling that this a bug, though. First, because it
> leaves you with no way at guarantee that values of a given domain always
> fulfill certain constraints. And second because "array[...]::arraytype"
> at least *looks* like a cast, and hence should behave like one too.

Agreed, it's a bug. A simpler example is just:

postgres=# create  domain myintarray as int[] check (value[1] < 10);
CREATE DOMAIN
postgres=# SELECT array['20']::myintarray; -- should fail
 array
───────
 {20}
(1 row)

There's a special case in transformExpr function to handle the
"ARRAY[...]::arraytype" construct, which skips the usual type-casting
and just constructs an ArrayExpr with the right target type. However,
it's not taking into account that the target type can be a domain.

Attached patch fixes that. Anyone see a problem with it?

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 04127bd..8ca4a2f 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -169,6 +169,20 @@ transformExpr(ParseState *pstate, Node *expr)
                                                     targetType,
                                                     elementType,
                                                     targetTypmod);
+
+                        /*
+                         * If the target array type is a domain, we still need
+                         * to check the domain constraint. (coerce_to_domain
+                         * is a no-op otherwise)
+                         */
+                        result = coerce_to_domain(result,
+                                                  InvalidOid,
+                                                  -1,
+                                                  targetType,
+                                                  COERCE_IMPLICIT_CAST,
+                                                  tc->location,
+                                                  false,
+                                                  true);
                         break;
                     }

Heikki Linnakangas wrote:
> Agreed, it's a bug. A simpler example is just: [snipped]

Will this fix for this be included in 8.4.2 (or .3), or will it have to
wait for 8.4 because it changes behavior?

> There's a special case in transformExpr function to handle the 
> "ARRAY[...]::arraytype" construct, which skips the usual type-casting
>  and just constructs an ArrayExpr with the right target type.
> However, it's not taking into account that the target type can be a
> domain.
> 
> Attached patch fixes that. Anyone see a problem with it?
I'm not familiar with the parser so I can't really judge this. However,
I've applied the patch to my development db and it seems to work fine,
and fixes the bug.

Thanks for the quick response!

best regards,
Florian Pflug

Florian G. Pflug wrote:
> Heikki Linnakangas wrote:
>> Agreed, it's a bug. A simpler example is just: [snipped]
> 
> Will this fix for this be included in 8.4.2 (or .3), or will it have to
> wait for 8.4 because it changes behavior?

It's a regression; 8.3 and earlier used to check the domain constraint
correctly.

I just committed it to CVS HEAD and REL8_4_STABLE. Thanks for the report!

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Florian G. Pflug wrote:
>> It seems that check constraints on domains are *not* executed for
>> literals of the domain-over-array-type - in other words, for expressions
>> like:
>> array[...]::<my-domain-over-array-type>.

> There's a special case in transformExpr function to handle the
> "ARRAY[...]::arraytype" construct, which skips the usual type-casting
> and just constructs an ArrayExpr with the right target type. However,
> it's not taking into account that the target type can be a domain.

> Attached patch fixes that. Anyone see a problem with it?

Hm.  I concur that this special-case code is failing to consider the
possibility that the target type is domain-over-array-type rather than
just array-type.  I think though that this patch is a bit of a kluge,
because it delivers a mislabeled expression tree.  The result of the
transformArrayExpr() is not really of type myintarray.  What it is is
a plain int[] value; we shouldn't label it as being already myintarray,
because it hasn't passed the domain checks.  At the moment there is
probably not any visible effect of that, but in the future it could
lead to misoptimization, so I think it's important to get it right.

My inclination is to apply getBaseTypeAndTypmod to the targetType and
pass that as the array type to transformArrayExpr, then instead of
considering the job done, fall through to transformTypeCast, which will
either do nothing or attach a domain coercion node.  I'm not sure about
the typmod handling (need more caffeine to work that out).

Do you want to have another go at it, or shall I?
        regards, tom lane

Tom Lane wrote:
> Hm.  I concur that this special-case code is failing to consider the
> possibility that the target type is domain-over-array-type rather than
> just array-type.  I think though that this patch is a bit of a kluge,
> because it delivers a mislabeled expression tree.  The result of the
> transformArrayExpr() is not really of type myintarray.  What it is is
> a plain int[] value; we shouldn't label it as being already myintarray,
> because it hasn't passed the domain checks.  At the moment there is
> probably not any visible effect of that, but in the future it could
> lead to misoptimization, so I think it's important to get it right.
> 
> My inclination is to apply getBaseTypeAndTypmod to the targetType and
> pass that as the array type to transformArrayExpr, then instead of
> considering the job done, fall through to transformTypeCast, which will
> either do nothing or attach a domain coercion node.

Hmm, yeah that's more accurate.

>  I'm not sure about
> the typmod handling (need more caffeine to work that out).

Seems straightforward to me. The ArrayExpr should have the typmod of the
base type, as returned by getBaseTypeAndTypmod, and domains don't have
typmods.

> Do you want to have another go at it, or shall I?

I'll give it a shot.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas wrote:
> Tom Lane wrote:
>> Hm.  I concur that this special-case code is failing to consider the
>> possibility that the target type is domain-over-array-type rather than
>> just array-type.  I think though that this patch is a bit of a kluge,
>> because it delivers a mislabeled expression tree.  The result of the
>> transformArrayExpr() is not really of type myintarray.  What it is is
>> a plain int[] value; we shouldn't label it as being already myintarray,
>> because it hasn't passed the domain checks.  At the moment there is
>> probably not any visible effect of that, but in the future it could
>> lead to misoptimization, so I think it's important to get it right.
>>
>> My inclination is to apply getBaseTypeAndTypmod to the targetType and
>> pass that as the array type to transformArrayExpr, then instead of
>> considering the job done, fall through to transformTypeCast, which will
>> either do nothing or attach a domain coercion node.
> 
> Hmm, yeah that's more accurate.

Ok, committed another patch doing exactly that.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com