Обсуждение: Bug with plpgsql handling of NULL argument of compound type
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 I think what's happening is when plpgsql_exec_function() is copying the arguments into plpgsql variables it's recursing into test_table.c and attempting to create c(NULL,NULL) instead of just setting test_table.c to NULL. 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. I'm not sure why that's not supported, but it would be nice if I could just do CREATE TYPE c AS (t text NOT NULL, i int); -- 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
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
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
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
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
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].
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.
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 https://www.postgresql.org/message-id/20160708024746.1410.57282@wrigleys.postgresql.org -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes: > On this topic, see also > https://www.postgresql.org/message-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