Bug with plpgsql handling of NULL argument of compound type

Jim Nasby <Jim.Nasby@BlueTreble.com> writes:

CREATE DOMAIN text_not_null AS text NOT NULL;
CREATE TYPE c AS( t text_not_null, i int );
CREATE TABLE test_table( id serial, c c );
CREATE OR REPLACE FUNCTION test_func(i test_table) RETURNS oid LANGUAGE
plpgsql AS $$
BEGIN
RETURN pg_typeof(i);
END$$;
SELECT test_func(NULL);
ERROR: domain text_not_null does not allow null values
CONTEXT: PL/pgSQL function test_func(test_table) while storing call
arguments into local variables

Arguably, that error is perfectly correct, not a bug.

There is a rather squishy question as to whether NULL::composite_type
should be semantically equivalent to ROW(NULL,NULL,...)::composite_type.
If it is, then the SELECT should have failed before even getting into the
plpgsql function, because ROW(NULL,NULL) is surely not a valid value of
type c. The SQL standard seems to believe that these things *are*
equivalent (at least, that was how we read the spec for IS [NOT] NULL).
We're not very good at making them actually act alike, but if they do act
alike in plpgsql, it's hard to call that a bug.

FWIW, the only reason I created 'text_not_null' in my real-word case is
because I have a compound type that I don't want to allow NULLS for some
of it's fields.

FWIW, there is a very good argument that any not-null restriction on a
datatype (as opposed to a stored column) is broken by design. How do
you expect a LEFT JOIN to a table with such a column to work? We
certainly are not going to enforce the not-nullness in that context,
and that leads to the thought that maybe we should just deny the validity
of such restrictions across the board.

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

On 7/22/16 1:13 PM, Tom Lane wrote:

There is a rather squishy question as to whether NULL::composite_type
should be semantically equivalent to ROW(NULL,NULL,...)::composite_type.
If it is, then the SELECT should have failed before even getting into the
plpgsql function, because ROW(NULL,NULL) is surely not a valid value of
type c. The SQL standard seems to believe that these things *are*
equivalent (at least, that was how we read the spec for IS [NOT] NULL).
We're not very good at making them actually act alike, but if they do act
alike in plpgsql, it's hard to call that a bug.

I was afraid this was an artifact of the spec...

FWIW, the only reason I created 'text_not_null' in my real-word case is
because I have a compound type that I don't want to allow NULLS for some
of it's fields.

FWIW, there is a very good argument that any not-null restriction on a
datatype (as opposed to a stored column) is broken by design. How do
you expect a LEFT JOIN to a table with such a column to work? We
certainly are not going to enforce the not-nullness in that context,
and that leads to the thought that maybe we should just deny the validity
of such restrictions across the board.

Because if the column storing the compound type is NULL itself, that
means the only thing you know is what the type of the column is. While
that does mean you know what it's structure would be if it was actually
a known quantity, the reality is it's not a known quantity. I would
argue that if test_table.c IS NULL that's not the same thing as
test_table.c = row(NULL,NULL).

Likewise, while pondering actually enforcing NOT NULL on types I worried
about how you'd handle SELECT test_func(NULL) until I realized that
(again), that's not the same thing as test_func(row(NULL,NULL)), nor is
it the same as test_func(row(1,row(NULL,NULL))).

The reason any of this actually matters is it seriously diminishes the
usefulness of composite types if you want a type that does useful
validation. In my case, it would be completely invalid for any of the
fields in the composite type to be NULL, but I should still be able to
allow something (a table or type) that uses that composite type to be NULL.

It occurs to me... does the spec actually indicate that
row(NULL,NULL)::c should work? I can see arguments for why (NULL::c).t
IS NULL might be allowed (special case retrieving field values from a
composite that's not actually defined).
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

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

On Fri, Jul 22, 2016 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There is a rather squishy question as to whether NULL::composite_type
should be semantically equivalent to ROW(NULL,NULL,...)::composite_type.
If it is, then the SELECT should have failed before even getting into the
plpgsql function, because ROW(NULL,NULL) is surely not a valid value of
type c. The SQL standard seems to believe that these things *are*
equivalent (at least, that was how we read the spec for IS [NOT] NULL).

​I dislike that they are considered equal in various circumstances but if
that's we are guided toward c'est la vie.
​

FWIW, there is a very good argument that any not-null restriction on a
datatype (as opposed to a stored column) is broken by design. How do
you expect a LEFT JOIN to a table with such a column to work?

​As described - except "NULL::composite_type" isn't atomic.

I/we would like: If you have a non-null value of this composite type then
the first field of the composite must itself be non-null.​

We

certainly are not going to enforce the not-nullness in that context,
and that leads to the thought that maybe we should just deny the validity
of such restrictions across the board.

​Soft or hard we should do this.​ Being allowed to set NOT NULL on a
domain with these traps built into the architecture is just asking for
angry users when their data model fails to be usable throughout their
application. The only thing we can offer is that we will respect NOT NULL
during the persisting a record to storage..

David J.

On Fri, Jul 22, 2016 at 1:39 PM, David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Fri, Jul 22, 2016 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There is a rather squishy question as to whether NULL::composite_type
should be semantically equivalent to ROW(NULL,NULL,...)::composite_type.
If it is, then the SELECT should have failed before even getting into the
plpgsql function, because ROW(NULL,NULL) is surely not a valid value of
type c. The SQL standard seems to believe that these things *are*
equivalent (at least, that was how we read the spec for IS [NOT] NULL).

I dislike that they are considered equal in various circumstances but if
that's we are guided toward c'est la vie.

Not sure we are guided there. Currently we follow the spec
specifically with the IS NULL operator but not in other cases. For
example.
postgres=# select row(null, null) is null;
?column?
──────────
t

postgres=# select coalesce(row(null, null), row(1,1));
coalesce
──────────
(,)

Seem not to agree (at all) since I'm pretty sure the spec defines
coalesce in terms of IS NULL.

The basic problem we have is that in postgres the record variable is a
distinct thing from its contents and the spec does not treat it that
was. For my part, I think the spec is totally out to lunch on this
point but we've been stuck with the status quo for quite some time now
-- there's been no compelling narrative that suggests how things
should be changed and to what.

merlin

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

Merlin Moncure <mmoncure@gmail.com> writes:

Not sure we are guided there. Currently we follow the spec
specifically with the IS NULL operator but not in other cases.

Yeah. ExecEvalNullTest() has been taught about this, but most other
places that check null-ness just check overall datum null-ness without
any concern for composite types. This is a stopgap situation, obviously.

The basic problem we have is that in postgres the record variable is a
distinct thing from its contents and the spec does not treat it that
was. For my part, I think the spec is totally out to lunch on this
point but we've been stuck with the status quo for quite some time now
-- there's been no compelling narrative that suggests how things
should be changed and to what.

I tend to agree that the spec is poorly designed on this point.
Some reasons:

* Per spec, a partly-null row value does not satisfy either IS NULL or
IS NOT NULL. This at least fails to be non-astonishing. Worse: as
implemented in ExecEvalNullTest, a zero-column row satisfies *both*
IS NULL and IS NOT NULL. The SQL committee would no doubt argue that
that's not their problem because they disallow zero-column rows, but
it's still an indication that the behavior isn't well-designed.

* What about other container types? If ROW(NULL,NULL) IS NULL, should
it not also be the case that ARRAY[NULL,NULL] IS NULL? Then we'd also
have to think about range types, JSON, etc. That way madness lies.
Anyway it seems pretty bogus to claim that ARRAY[NULL,NULL] IS NULL,
because it has for example well-defined dimensions. You could maybe try
to justify treating the case differently because such an array value has
metadata, ie dimensions, in addition to its element values --- but I deny
the claim that a row value lacks any metadata.

So personally I'd much prefer to consider that ROW(NULL, NULL) isn't NULL,
and I'm not in a hurry to propagate ExecEvalNullTest's behavior to other
places.

The question of whether we should allow NOT NULL constraints on a datatype
is somewhat independent of that, though.

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

On Fri, Jul 22, 2016 at 3:04 PM, Merlin Moncure <mmoncure@gmail.com> wrote:

On Fri, Jul 22, 2016 at 1:39 PM, David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Fri, Jul 22, 2016 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

There is a rather squishy question as to whether NULL::composite_type
should be semantically equivalent to ROW(NULL,NULL,...)::composite_type.
If it is, then the SELECT should have failed before even getting into

the

plpgsql function, because ROW(NULL,NULL) is surely not a valid value of
type c. The SQL standard seems to believe that these things *are*
equivalent (at least, that was how we read the spec for IS [NOT] NULL).

I dislike that they are considered equal in various circumstances but if
that's we are guided toward c'est la vie.

Not sure we are guided there. Currently we follow the spec
specifically with the IS NULL operator but not in other cases. For
example.
postgres=# select row(null, null) is null;
?column?
──────────
t

​[...]

​

The basic problem we have is that in postgres the record variable is a
distinct thing from its contents and the spec does not treat it that
was. For my part, I think the spec is totally out to lunch on this
point but we've been stuck with the status quo for quite some time now
-- there's been no compelling narrative that suggests how things
should be changed and to what.

​In short,

1) We should discourage/remove the NOT NULL aspect of DOMAINs.

2) If one wishes to implement composite types defining not null components
it should
2a) be done on the CREATE TYPE statement
2b) involve behind-the-scenes transformation of row(null, null)::ctype to
null::ctype and null::ctype should not be validated, ever

​I cannot speak to the standard nor the entirety of our implementation in
this area, but...​

​I don't personally have a problem with (conceptually, not actual
evaluations):

select row(null, null) is null --> true
select null is null --> true
select null = row(null, null) --> false (at least with respect to
implementation)

IS NULL and equality are two different things. That both constructs
evaluate to null but are not implementation equivalent, while maybe a bit
ugly, doesn't offend me. I'd just consider "row(null, null) is null" to be
a special case circumstance required by the spec and move on.

Then, forcing "null::composite" to be evaluated like "row(null,
null)::composite" ​can be considered incorrect.

​If anything, ROW(null, null)::ctype should hard transformed to
"null::ctype" but not the other way around. Only after an attempt to
transform row(null, null) is performed should the type constraints be
applied to those values not able to be transformed.

That all said I'm still disinclined to suggest/allow people to add NOT NULL
constraints to DOMAINs. All other types in the system are basically
validated using the rule: "if there is a non-null value of this type ensure
that said value conforms". As domains are types they should conform to
this policy. A composite type is a container for other types. The
container itself should be allowed to have its own rules - in much the same
way as a table does [1]I see a problem here if one were to change the behavior of explicit composite types. w.r.t. tables the NOT NULL constraints is not part of the implicitly created type but if we allow an explicitly declared composite to use NOT NULL and don't default the implicit types the disconnect could be confusing..

My concern regarding the above is that the row/isnull behavior is all
defined around the composite type yet the notnull constraint is attached to
the DOMAIN and I dislike that disconnect. Having the NOT NULL on the
composite type and only having it applied after any value of the form
row(null, null)::ctype is reduced to null::ctype - a form in which all
subfield constraints are ignored - would be closer to my liking. It also
avoids other problems related to DOMAINs but not requiring their use.

David J.

[1]: I see a problem here if one were to change the behavior of explicit composite types. w.r.t. tables the NOT NULL constraints is not part of the implicitly created type but if we allow an explicitly declared composite to use NOT NULL and don't default the implicit types the disconnect could be confusing.
composite types. w.r.t. tables the NOT NULL constraints is not part of the
implicitly created type but if we allow an explicitly declared composite to
use NOT NULL and don't default the implicit types the disconnect could be
confusing.

Tom Lane wrote:

Merlin Moncure <mmoncure@gmail.com> writes:

Not sure we are guided there. Currently we follow the spec
specifically with the IS NULL operator but not in other cases.

Yeah. ExecEvalNullTest() has been taught about this, but most other
places that check null-ness just check overall datum null-ness without
any concern for composite types. This is a stopgap situation, obviously.

On this topic, see also
/messages/by-id/20160708024746.1410.57282@wrigleys.postgresql.org

--
�lvaro Herrera http://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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On this topic, see also
/messages/by-id/20160708024746.1410.57282@wrigleys.postgresql.org

I'd forgotten about that thread :-( ... but on looking at it, it's very
relevant indeed. See my followup there.

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