Thread: Proposal: revert behavior of IS NULL on row types

In light of the fact that it is an endless cause of bugs both in pg and
potentially to applications, I propose that we cease attempting to
conform to the spec's definition of IS NULL in favour of the following
rules:

1. x IS NULL  is true if and only if x has the null value (isnull set).

2. x IS NOT NULL  if and only if  NOT (x IS NULL)

3. ROW() and other row constructors never return the null value.
Whole-row vars when constructed never contain the null value. 

4. Columns or variables of composite type can (if not declared NOT NULL)
contain the null value (isnull set) which is distinct from an
all-columns-null value.

5. COALESCE(x,y) continues to return y if and only if x is the null
value. (We currently violate the spec here.)

(X. Optionally, consider adding new predicates:
 x IS ALL NULL x IS NOT ALL NULL x IS ALL NOT NULL x IS NOT ALL NOT NULL

which would examine the fields of x non-recursively.)

Justification:

https://www.postgresql.org/message-id/4f6a90a0-c6e8-22eb-3b7a-727f8a60f3b1%40BlueTreble.com
https://www.postgresql.org/message-id/20160708024746.1410.57282%40wrigleys.postgresql.org

Further rationale:

https://www.postgresql.org/message-id/87zip9qti4.fsf%40news-spur.riddles.org.uk

-- 
Andrew (irc:RhodiumToad)

>>>>> "David" == David G Johnston <david.g.johnston@gmail.com> writes:
>> 2. x IS NOT NULL  if and only if  NOT (x IS NULL)
David> ​I would rather prohibit "IS NOT NULL" altogether.​ If one needsDavid> to test "NOT (x IS NULL)" they can write
itthat way. 

Prohibiting IS NOT NULL is not on the cards; it's very widely used.
>> Whole-row vars when constructed never contain the null value.
David> ...but what does this mean in end-user terms?​

It means for example that this query:
 select y from x left join y on (x.id=y.id) where y is null;

would always return 0 rows.

--
Andrew (irc:RhodiumToad)

>>>>> "David" == David G Johnston <david.g.johnston@gmail.com> writes:
>> 1. x IS NULL  is true if and only if x has the null value (isnull set).
David> ​I don't have a problem conforming to "ROW(NULL, NULL) IS NULL"David> being true...​if you somehow get a hold of
somethingin thatDavid> form, which your others points address. 

This seems harmless, but I think it's not worth the pain.

I'm informed that for example on Oracle:
 select case when foo_type(null, null) is null             then 'true' else 'false'         end from dual;

returns 'false'.

--
Andrew (irc:RhodiumToad)

>>>>> "David" == David G Johnston <david.g.johnston@gmail.com> writes:
>> Prohibiting IS NOT NULL is not on the cards; it's very widely used.
David> ​Yet changing how it behaves, invisibly, is?

Did you mean prohibiting it only for composite-type args? It's obviously
widely used for non-composite args.

I would expect that >95% of cases where someone has written (x IS NOT
NULL) for x being a composite type, it's actually a bug and that NOT (x
IS NULL) was intended.

--
Andrew (irc:RhodiumToad)

>>>>> "David" == David G Johnston <david.g.johnston@gmail.com> writes:

 >> Prohibiting IS NOT NULL is not on the cards; it's very widely used.

 David> ​Yet changing how it behaves, invisibly, is?

Did you mean prohibiting it only for composite-type args? It's obviously
widely used for non-composite args.

I would expect that >95% of cases where someone has written (x IS NOT
NULL) for x being a composite type, it's actually a bug and that NOT (x
IS NULL) was intended.

On 7/22/16 8:05 PM, David G. Johnston wrote:
>
>     I would expect that >95% of cases where someone has written (x IS NOT
>     NULL) for x being a composite type, it's actually a bug and that NOT (x
>     IS NULL) was intended.
>
>
> Yeah, it would need to be targeted there.  I agree with the numbers and
> the sentiment but it's still allowed and defined behavior which changing
> invisibly is disconcerting.

Yeah, that worries me too... I'm not sure what can be done about it 
other than a compatibility GUC (and we know how we love those... :( ).

On the LEFT JOIN scenario, I'm not sure why we should disallow that. Is 
that commonly error prone?

BTW, one thing that would be very nice about this is it'd let you do
DECLARE  r_new record := coalesce(NEW,OLD);  r_old record := coalesce(OLD,NEW);

which would be much nicer than how things work today (I'm going on the 
assumption that referencing NEW in a DELETE trigger would be legit and 
give you NULL with the new semantics, but trying to actually reference 
any of it's fields would produce an error).
-- 
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

>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>>> Whole-row vars when constructed never contain the null value.
David> ...but what does this mean in end-user terms?​
Andrew> It means for example that this query:
Andrew>   select y from x left join y on (x.id=y.id) where y is null;
Andrew> would always return 0 rows.

On second thoughts I'll take this one back. Specifying that whole-row
vars don't contain the null value when constructed doesn't actually
result in no rows, since the construction is logically below the join;
and hence even with that rule in place, the query would return a row for
each non-matched "x" row.

--
Andrew (irc:RhodiumToad)

On 7/22/16 7:01 PM, Andrew Gierth wrote:
> In light of the fact that it is an endless cause of bugs both in pg and
> potentially to applications, I propose that we cease attempting to
> conform to the spec's definition of IS NULL in favour of the following
> rules:

I can't see how we would incompatibly change existing
standards-conforming behavior merely because users are occasionally
confused and there are some bugs in corner cases.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jul 25, 2016 at 8:28 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 7/22/16 7:01 PM, Andrew Gierth wrote:
>> In light of the fact that it is an endless cause of bugs both in pg and
>> potentially to applications, I propose that we cease attempting to
>> conform to the spec's definition of IS NULL in favour of the following
>> rules:
>
> I can't see how we would incompatibly change existing
> standards-conforming behavior merely because users are occasionally
> confused and there are some bugs in corner cases.

+1

Anywhere we have standard-conforming behavior, changing the
semantics of the standard syntax seems insane.  We can create new
syntax for extensions where we are convinced we have a better idea.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 7/22/16 7:01 PM, Andrew Gierth wrote:
>> In light of the fact that it is an endless cause of bugs both in pg and
>> potentially to applications, I propose that we cease attempting to
>> conform to the spec's definition of IS NULL in favour of the following
>> rules:

> I can't see how we would incompatibly change existing
> standards-conforming behavior merely because users are occasionally
> confused and there are some bugs in corner cases.

It seems clear that Andrew's proposal to reject the spec's definition that
ROW(NULL,NULL,...) IS NULL is true has failed.  So ExecEvalNullTest()
should keep on doing what it's doing.  There are several related issues
however:

1. As per bug #14235, eval_const_expressions() behaves differently from
ExecEvalNullTest() for nested-composite-types cases.  It is inarguably
a bug that they don't give the same answers.  So far, no one has spoken
against the conclusion reached in that thread that ExecEvalNullTest()
correctly implements the spec's semantics and eval_const_expressions()
does not.  Therefore I propose to apply and back-patch Andrew's fix from
that thread.

2. Our (or at least my) previous understanding was that ROW(NULL,NULL,...)
ought to be considered NULL for all purposes, and that our failure to
do so anywhere except ExecEvalNullTest was a spec violation we should do
something about someday.  But the bug #14235 thread makes it clear that
that's not so, and that only in very limited cases is there an argument
for applying IS [NOT] NULL's behavior to other constructs.  COALESCE()
was mentioned as something that maybe should change.  I'm inclined to vote
"no, let's keep COALESCE as-is".  The spec's use of, essentially, macro
expansion to define COALESCE is just stupid, not least because it's
impossible to specify the expected at-most-once evaluation of each
argument expression that way.  (They appear to try to dodge that question
by forbidding argument expressions with side-effects, which is just lame.)
But had they written out a definition of COALESCE's behavior in words,
they would almost certainly have written "V is not the null value" not
"V IS NOT NULL".  Anyone who stopped to think about it would surely think
that the result of COALESCE(ROW(1,NULL), NULL) should not be NULL.  So I
think the apparent mandate to use IS NOT NULL's semantics for rowtype
values in COALESCE is just an artifact of careless drafting.  Between that
and the backwards-compatibility hazards of changing, it's not worth it.

3. Andrew also revived the bug #7808 thread in which it was complained
that ExecMakeTableFunctionResult should not fail on null results from
functions returning SETOF composite.  That's related only in that the
proposed fix is to translate a plain NULL into ROW(NULL,NULL,...).
I do not much like the implementation details of his proposed patch,
but I think making that translation is probably better than failing
altogether.  Given the infrequency of field complaints, my recommendation
here is to fix it in HEAD but not back-patch.

Comments?
        regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> The concept embodied by "NULL" in the operator "IS [NOT] NULL" is distinct
> from the concept embodied by "NULL" in the operator "IS [NOT] DISTINCT
> FROM".

> In short, the former smooths out the differences between composite and
> non-composite types while the later maintains their differences.  While a
> bit confusing I don't see that there is much to be done about it - aside
> from making the distinction more clear at:
> ​https://www.postgresql.org/docs/devel/static/functions-comparison.html

> Does spec support or refute this distinction in treatment?

AFAICS, the IS [NOT] DISTINCT FROM operator indeed is specified to do the
"obvious" thing when one operand is NULL: you get a simple nullness check
on the other operand.  So I went ahead and documented that it could be
used for that purpose.
        regards, tom lane

On Wed, Jul 27, 2016 at 7:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
>> The concept embodied by "NULL" in the operator "IS [NOT] NULL" is distinct
>> from the concept embodied by "NULL" in the operator "IS [NOT] DISTINCT
>> FROM".
>
>> In short, the former smooths out the differences between composite and
>> non-composite types while the later maintains their differences.  While a
>> bit confusing I don't see that there is much to be done about it - aside
>> from making the distinction more clear at:
>> https://www.postgresql.org/docs/devel/static/functions-comparison.html
>
>> Does spec support or refute this distinction in treatment?
>
> AFAICS, the IS [NOT] DISTINCT FROM operator indeed is specified to do the
> "obvious" thing when one operand is NULL: you get a simple nullness check
> on the other operand.  So I went ahead and documented that it could be
> used for that purpose.

By the way, our documentation says that NOT NULL constraints are
equivalent to CHECK (column_name IS NOT NULL).  That is what the SQL
standard says, but in fact our NOT NULL constraints are equivalent to
CHECK (column_name IS DISTINCT FROM NULL).  Should we update the
documentation with something like the attached?

--
Thomas Munro
http://www.enterprisedb.com

On 7/26/16 7:46 PM, Thomas Munro wrote:
> By the way, our documentation says that NOT NULL constraints are
> equivalent to CHECK (column_name IS NOT NULL).  That is what the SQL
> standard says, but in fact our NOT NULL constraints are equivalent to
> CHECK (column_name IS DISTINCT FROM NULL).  Should we update the
> documentation with something like the attached?

Couldn't we just fix that instead?  For NOT NULL constraints on
composite type columns, create a full CHECK (column_name IS NOT NULL)
constraint instead, foregoing the attnotnull optimization.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 7/26/16 7:46 PM, Thomas Munro wrote:
>> By the way, our documentation says that NOT NULL constraints are
>> equivalent to CHECK (column_name IS NOT NULL).  That is what the SQL
>> standard says, but in fact our NOT NULL constraints are equivalent to
>> CHECK (column_name IS DISTINCT FROM NULL).  Should we update the
>> documentation with something like the attached?

> Couldn't we just fix that instead?  For NOT NULL constraints on
> composite type columns, create a full CHECK (column_name IS NOT NULL)
> constraint instead, foregoing the attnotnull optimization.

Maybe.  There's a patch floating around that expands attnotnull into
CHECK constraints, which'd provide the infrastructure needed to consider
changing this behavior.  But that's not going to be back-patchable, and
as I noted in <10682.1469566308@sss.pgh.pa.us>, we have a problem right
now that the planner's constraint exclusion logic gets these semantics
wrong.
        regards, tom lane