Thread: CREATE CAST allows creation of binary-coercible cast to range over domain

CREATE CAST disallows creating a binary-coercible cast to a domain 
(because that would bypass checking the domain constraints).  But it 
allows it if the domain is wrapped inside a range type:

CREATE DOMAIN mydomain AS int4 CHECK (VALUE > 0);
CREATE CAST (int4 AS mydomain) WITHOUT FUNCTION;  -- error (ok)

CREATE TYPE mydomainrange AS range (subtype=mydomain);
CREATE CAST (int4range AS mydomainrange) WITHOUT FUNCTION;  -- FIXME

SELECT int4range(-5,-4)::mydomainrange;  -- this succeeds

This particular case seems straightforward to fix, but maybe there are 
also cases with more nesting to consider.

(I just found this while exploring other range-over-domain issues in 
some in-progress work.)

Peter Eisentraut <peter@eisentraut.org> writes:
> CREATE CAST disallows creating a binary-coercible cast to a domain 
> (because that would bypass checking the domain constraints).  But it 
> allows it if the domain is wrapped inside a range type:
> ...
> This particular case seems straightforward to fix, but maybe there are 
> also cases with more nesting to consider.

I think it's an oversight that we allow binary-coercible casts
involving range types at all.  There are no other nesting cases to
worry about, because CreateCast already rejects binary-coercible casts
applied to composites and arrays, explaining

         * We know that composite, enum and array types are never binary-
         * compatible with each other.  They all have OIDs embedded in them.

But range types also embed their own OID, so I don't see why that
concern doesn't apply to them.

            regards, tom lane

I wrote:
> I think it's an oversight that we allow binary-coercible casts
> involving range types at all.  There are no other nesting cases to
> worry about, because CreateCast already rejects binary-coercible casts
> applied to composites and arrays, explaining
>          * We know that composite, enum and array types are never binary-
>          * compatible with each other.  They all have OIDs embedded in them.
> But range types also embed their own OID, so I don't see why that
> concern doesn't apply to them.

In short, more or less as attached.  (I didn't bother with a
regression test, since none of the adjacent error checks are
covered either.)

I'm unsure whether to back-patch this.  If somebody were using
such a cast today, it might seem to mostly work, as long as the
two kinds of ranges had the same behavior.  So they might not
appreciate us breaking it in a minor release.  Maybe we should
put this into 17 but not further back.

            regards, tom lane

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 6593fd7d81..149642ab1b 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -1689,13 +1689,18 @@ CreateCast(CreateCastStmt *stmt)
                      errmsg("source and target data types are not physically compatible")));

         /*
-         * We know that composite, enum and array types are never binary-
-         * compatible with each other.  They all have OIDs embedded in them.
+         * We know that composite, array, range and enum types are never
+         * binary-compatible with each other.  They all have OIDs embedded in
+         * them.
          *
          * Theoretically you could build a user-defined base type that is
-         * binary-compatible with a composite, enum, or array type.  But we
-         * disallow that too, as in practice such a cast is surely a mistake.
-         * You can always work around that by writing a cast function.
+         * binary-compatible with such a type.  But we disallow that too, as
+         * in practice such a cast is surely a mistake.  You can always work
+         * around that by writing a cast function.
+         *
+         * NOTE: if we didn't exclude every kind of container type for this
+         * reason, we'd likely need to recursively apply all of these same
+         * checks to the contained type(s).
          */
         if (sourcetyptype == TYPTYPE_COMPOSITE ||
             targettyptype == TYPTYPE_COMPOSITE)
@@ -1703,6 +1708,14 @@ CreateCast(CreateCastStmt *stmt)
                     (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                      errmsg("composite data types are not binary-compatible")));

+        if (sourcetyptype == TYPTYPE_RANGE ||
+            targettyptype == TYPTYPE_RANGE ||
+            sourcetyptype == TYPTYPE_MULTIRANGE ||
+            targettyptype == TYPTYPE_MULTIRANGE)
+            ereport(ERROR,
+                    (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                     errmsg("range data types are not binary-compatible")));
+
         if (sourcetyptype == TYPTYPE_ENUM ||
             targettyptype == TYPTYPE_ENUM)
             ereport(ERROR,

On 20.08.24 19:30, Tom Lane wrote:
> I wrote:
>> I think it's an oversight that we allow binary-coercible casts
>> involving range types at all.  There are no other nesting cases to
>> worry about, because CreateCast already rejects binary-coercible casts
>> applied to composites and arrays, explaining
>>           * We know that composite, enum and array types are never binary-
>>           * compatible with each other.  They all have OIDs embedded in them.
>> But range types also embed their own OID, so I don't see why that
>> concern doesn't apply to them.
> 
> In short, more or less as attached.  (I didn't bother with a
> regression test, since none of the adjacent error checks are
> covered either.)

This patch looks right.

> I'm unsure whether to back-patch this.  If somebody were using
> such a cast today, it might seem to mostly work, as long as the
> two kinds of ranges had the same behavior.  So they might not
> appreciate us breaking it in a minor release.  Maybe we should
> put this into 17 but not further back.

Yeah, 17 and up seems ok to me.

On Wed, Aug 21, 2024 at 2:14 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 20.08.24 19:30, Tom Lane wrote:
> > In short, more or less as attached.  (I didn't bother with a
> > regression test, since none of the adjacent error checks are
> > covered either.)
>
> This patch looks right.

This patch also looks good to me.  To nitpick:

-    * We know that composite, enum and array types are never binary-
-    * compatible with each other.  They all have OIDs embedded in them.
+    * We know that composite, array, range and enum types are never
+    * binary-compatible with each other.  They all have OIDs embedded in
+    * them.

I wonder if it would be better for readability to list these types in
the order we check them in the code, as we did previously, i.e.:

    * We know that composite, range, enum and array types are never
    * ...

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:
> I wonder if it would be better for readability to list these types in
> the order we check them in the code, as we did previously, i.e.:

The previous order seemed quite random to me, because enums aren't
container types and the reason for excluding them isn't really the
same.  I thought about re-ordering the code to match the new comment,
but desisted.  We could do that though...

            regards, tom lane