Subject: Make foo=null a warning by default.

From: David Fetter <[email-masked]>
Date: Mon, 16 Jul 2018 03:40:42 +0200

Folks,

Per a discussion with Andrew Gierth and Vik Fearing, both of whom
helped make this happen, please find attached a patch which makes it
possible to get SQL standard behavior for "= NULL", which is an error.
It's been upgraded to a warning, and can still be downgraded to
silence (off) and MS-SQL-compatible behavior (on).

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 16 Jul 2018 10:49:02 +0300

> silence (off) and MS-SQL-compatible behavior (on).

I don't agree with changing the default to 'warn'. "foo = NULL" is 
perfectly legal SQL, even if it's not very useful in practice.

The use case for transform_null_equals='warn' that I see is to wean off 
an application that uses transform_null_equals='on', to find all the 
queries that rely on it. But I'm not too excited about that either. The 
documented case for using transform_null_equals='on' is compatibility 
with Microsoft Access, and this wouldn't help with that. Besides, it's 
easy enough to grep the application or the server log for " = NULL", to 
find all such queries.

- Heikki

---

From: Fabien COELHO <[email-masked]>
Date: Mon, 16 Jul 2018 09:49:14 +0200 (CEST)

Hello David,

> silence (off) and MS-SQL-compatible behavior (on).

That's nice, and good for students, and probably others as well:-)

A few comments:

Patch applies & compiles, "make check" ok.

   #backslash_quote = safe_encoding    # on, off, or safe_encoding
   [...]
   #transform_null_equals = warn

I think it would be nice to have the possible values as a comment in 
"postgresql.conf".

* Code:

   -bool           operator_precedence_warning = false;
   +bool   operator_precedence_warning = false;

Is this reindentation useful/needed?

  +     parser_errposition(pstate, a->location)));
  +   if (need_transform_null_equals && transform_null_equals == TRANSFORM_NULL_EQUALS_ON)

For consistency, maybe skip a line before the if?

   transform_null_equals_options[] = { [...]

I do not really understand the sort order of this array. Maybe it could be 
alphabetical, or per value? Or maybe it is standard values and then 
synonyms, in which is case maybe a comment on the end of the list.

Guc help text: ISTM that it should spell out the possible values and their 
behavior. Maybe something like:

"""
When turned on, turns expr = NULL into expr IS NULL.
With off, warn or error, do not do so and be silent, issue a warning or an error.
The standard behavior is not to perform this transformation.
Default is warn.
"""

Or anything in better English.

* Test

  +select 1=null;
  + ?column?

Maybe use as to describe the expected behavior, so that it is 
easier to check, and I think that "?column?" looks silly eg:

   select 1=null as expect_{true,false}[_and_a_warning/error];

    create table cnullchild (check (f1 = 1 or f1 = null)) inherits(cnullparent);
   +WARNING:  = NULL can only produce a NULL

I'm not sure of this test case. Should it be turned into "is null"?

Maybe the behavior could be retested after the reset?

* Documentation:

The "error" value is not documented.

More generally, The value is said to be an enum, but the list of values is 
not listed anywhere in the documentation.

ISTM that the doc would be clearer if for each of the four values of the 
parameter the behavior is spelled out.

Maybe "warn" in the doc should be <literal>warn</literal> or something.

-- 
Fabien.

---

From: Fabien COELHO <[email-masked]>
Date: Mon, 16 Jul 2018 09:53:55 +0200 (CEST)

Hello Heikki,

> The use case for transform_null_equals='warn' that I see is to wean off an 
> application that uses transform_null_equals='on', to find all the queries 
> that rely on it. But I'm not too excited about that either. The documented 
> case for using transform_null_equals='on' is compatibility with Microsoft 
> Access, and this wouldn't help with that. Besides, it's easy enough to grep 
> the application or the server log for " = NULL", to find all such queries.

I'm in favor in having this feature, even if off by default, because I 
could enable it and it would help my students when learning SQL in 
interactive sessions, where "grep '= *NULL'" cannot be issued.

-- 
Fabien.

---

From: Robert Haas <[email-masked]>
Date: Mon, 16 Jul 2018 11:06:48 -0400

On Mon, Jul 16, 2018 at 3:49 AM, Heikki Linnakangas <[email-masked]> wrote:
> I don't agree with changing the default to 'warn'. "foo = NULL" is perfectly
> legal SQL, even if it's not very useful in practice.

+1.  I think that will probably generate lots of annoying warnings in
server logs compared to the value it adds.  I don't object to having
an optional feature to generate warnings, but I think it should be
something a particular user needs to activate, not something we enable
by default.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Mon, 16 Jul 2018 11:10:27 -0400

> perfectly legal SQL, even if it's not very useful in practice.

I think that there's a very narrow argument to be made that SQL doesn't
allow a NULL literal with context-determined type in this context.  But
we decided to generalize that restriction long ago, and suddenly deciding
to enforce it only in this one context makes no sense to me.  The idea
that we would ever decide that it's an error seems flat out ridiculous.

TBH I'm not really excited about investing any work in this area at all.
Considering how seldom we hear any questions about transform_null_equals
anymore[1], I'm wondering if we couldn't just rip the "feature" out
entirely.  But to the extent that we want to leave it in place, it's
100% for backwards compatibility, and that is a strong argument against
changing anything about it.

I'm also pretty dubious that issuing a warning here will accomplish much
to help anyone find bugs.  There are too many small variants of the same
problem that it will not catch[2].

			regards, tom lane

[1] In a quick search, the most recent discussion I could find was from
2011:
https://www.postgresql.org/message-id/flat/[email-masked]
Before that it came up maybe once a year or so, but it's just fallen
off a cliff since then.  I wonder whether Microsoft fixed Access to
not need it.

[2] Compare for instance the discussion about bug #6064,
https://www.postgresql.org/message-id/flat/4DFE481F020000250003E8EA%40gw.wicourts.gov
A very large fraction of the pre-2011 threads mentioning
transform_null_equals are essentially of the form "why didn't/can't
transform_null_equals fix this bad code for me?".  Each of those cases
would also be a case that the proposed warning doesn't catch.

---

From: Heikki Linnakangas <[email-masked]>
Date: Mon, 16 Jul 2018 18:19:22 +0300

On 16/07/18 18:10, Tom Lane wrote:
> entirely.
Yeah, I was wondering about that too. But Fabien brought up a completely 
new use-case for this: people learning SQL. For beginners who don't 
understand the behavior of NULLs yet, I can see a warning or error being 
useful training wheels. Perhaps a completely new "training_wheels=on" 
option, which could check may for many other beginner errors, too, would 
be better for that.

- Heikki

---

From: Tom Lane <[email-masked]>
Date: Mon, 16 Jul 2018 11:37:28 -0400

> option, which could check may for many other beginner errors, too, would 

Agreed --- but what we'd want that to do seems only vaguely related to
the existing behavior of transform_null_equals.  As an example, we
intentionally made transform_null_equals *not* trigger on

	CASE x WHEN NULL THEN ...

but a training-wheels warning for that would likely be reasonable.

For that matter, many of the old threads about this are complaining
about nulls that aren't simple literals in the first place.  I wonder
whether a training-wheels feature that whined *at runtime* about null
WHERE-qual or case-test results would be more useful than a parser
check.

			regards, tom lane

---

From: David Fetter <[email-masked]>
Date: Mon, 16 Jul 2018 17:59:34 +0200

>   #transform_null_equals = warn

Fixed.

> Is this reindentation useful/needed?

I believe so because it fits with the line just below it.

> For consistency, maybe skip a line before the if?

Fixed.

> synonyms, in which is case maybe a comment on the end of the list.

I've changed it to per value. Is this better?

> Or anything in better English.

I've changed this, and hope it's clearer.

> * Test
> 
>  +select 1=null;
>  + ?column?
> 
> Maybe use as to describe the expected behavior, so that it is easier to
> check, and I think that "?column?" looks silly eg:
> 

Changed to more descriptive headers.

> I'm not sure of this test case. Should it be turned into "is null"?

This was just adjusting the existing test output to account for the
new warning. I presume it was phrased that way for a reason.

> Maybe "warn" in the doc should be <literal>warn</literal> or something.

Fixed.

Best,
David.

Remember to vote!

---

From: David Fetter <[email-masked]>
Date: Tue, 17 Jul 2018 03:49:22 +0200

> WHERE-qual or case-test results would be more useful than a parser

Am I understanding correctly that this would be looking for bogus
conditions in the plan tree?

Best,
David.

Remember to vote!

---

From: Fabien COELHO <[email-masked]>
Date: Tue, 17 Jul 2018 07:58:50 -0400 (EDT)

Hello,

> Yeah, I was wondering about that too. But Fabien brought up a completely new 
> use-case for this: people learning SQL.

Indeed.

This year, about 30% of my students wrote "= NULL" in a query at least 
once. Probably I or a colleague were called to the rescue for help.

So this warning would save me time, which explains why I reviewed the 
patch with such enthousiasm:-)

I'm fine with keeping the current behavior as a default.

-- 
Fabien.

---

From: Fabien COELHO <[email-masked]>
Date: Tue, 17 Jul 2018 08:34:17 -0400 (EDT)

Hello David,

A few comments about this v2.

ISTM that there is quite strong opposition to having "warn" as a default, 
so probably you should set if "off"?

> I've changed it to per value. Is this better?

At least, I can see the logic.

> I've changed this, and hope it's clearer.

Yep, and more elegant than my proposal.

> Changed to more descriptive headers.

Cannot see it in the patch update?

> new warning. I presume it was phrased that way for a reason.

Hmmm. Probably you are right. I think that the test case deserves better 
comments, as it is most peculiar. It was added by Tom for testing CHECK 
constant NULL expressions simplications.

TRUE OR NULL is TRUE, but FALSE OR NULL is NULL. Why not. Combined with 
the fact that NULL is considered ok for a CHECK, this lead to strange but 
intentional behavior, such as:

   -- this CHECK is always nignored in practice
   CREATE TABLE foo (i INT CHECK(i = 1 OR NULL));
   INSERT INTO foo(i) VALUES(2); # ACCEPTED

   -- but this one is not
   CREATE TABLE foo (i INT CHECK(i = 1 OR FALSE));
   INSERT INTO foo(i) VALUES(2); # REFUSED

Can't say I'm thrilled about that, and the added warning looks 
appropriate.

-- 
Fabien.

---

From: David Fetter <[email-masked]>
Date: Wed, 18 Jul 2018 06:11:23 +0200

On Tue, Jul 17, 2018 at 08:34:17AM -0400, Fabien COELHO wrote:
> ISTM that there is quite strong opposition to having "warn" as a default, so
> probably you should set if "off"?

Done.

> At least, I can see the logic.

I've now ordered it in what seems like a reasonable way, namely all
the "off" options, then the "on" option.

> Yep, and more elegant than my proposal.

I assure you that you expression yourself in English a good deal
better than I do in Portuguese.

> Cannot see it in the patch update?

Oops. They should be there now.

> TRUE OR NULL is TRUE, but FALSE OR NULL is NULL. Why not. Combined with the
> fact that NULL is considered ok for a CHECK, this lead to strange but
> Can't say I'm thrilled about that, and the added warning looks appropriate.

Per request, the warning is off by default, so the warning went away.

Best,
David.

Remember to vote!

---

From: Fabien COELHO <[email-masked]>
Date: Wed, 18 Jul 2018 08:25:46 -0400 (EDT)

Hello David,

> better than I do in Portuguese.

Alas, despite a Portuguese "rabbit" name, I cannot speak the language 
which got lost between generations.

About this v3: Patch applies, compiles, "make check" ok.

A few minor comments:

Variable "need_transform_null_equals" may be better named 
"is_null_equals", because there is no "need" of the transformation as 
such, and the expression just checks for the pattern, really.

I'm fine with the off/warn/error/on order.

Doc could mention that the transformation allows compatibility with other 
products, without naming them? Or not.

In doc, the list of valid values on a long line, where the practice seems 
to wrap around after about 80 columns in the XML file.

I notice that the feature was not tested at all before this patch:-(

Maybe there could be one test which results to true, which is the whole 
point of the transformation? eg "SELECT NULL = NULL".

-- 
Fabien.

---

From: David Fetter <[email-masked]>
Date: Wed, 18 Jul 2018 17:21:53 +0200

On Wed, Jul 18, 2018 at 08:25:46AM -0400, Fabien COELHO wrote:
> Alas, despite a Portuguese "rabbit" name, I cannot speak the language which
> got lost between generations.

I fear my kids may lose French the same way. Mine's already not great,
and that's in just one generation.

> Variable "need_transform_null_equals" may be better named "is_null_equals",
> because there is no "need" of the transformation as such, and the expression
> just checks for the pattern, really.

Done.

> products, without naming them? Or not.

I don't see a point in obscuring the origin.

> In doc, the list of valid values on a long line, where the practice seems to
> wrap around after about 80 columns in the XML file.

Fixed.

Done.

Best,
David.

Remember to vote!

---

From: Fabien COELHO <[email-masked]>
Date: Wed, 18 Jul 2018 16:15:30 -0400 (EDT)

> [...] Done.

All looks well, but I just noticed a warning:

  gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -O2 -I. -I. -I../../../../src/include  -D_GNU_SOURCE   -c -o guc.o guc.c
  guc.c:4090:3: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
    &transform_null_equals,
    ^
  guc.c:4090:3: note: (near initialization for ‘ConfigureNamesEnum[16].variable’)

Too bad, gcc does not like an (enum*) there. Maybe cast to (int *), or 
declare the variable as an int (which seems to be what is done for 
others)?

-- 
Fabien.

---

From: David Fetter <[email-masked]>
Date: Thu, 19 Jul 2018 07:52:18 +0200

On Wed, Jul 18, 2018 at 04:15:30PM -0400, Fabien COELHO wrote:
> declare the variable as an int (which seems to be what is done for others)?

Done that way.

Best,
David.

Remember to vote!

---

From: Fabien COELHO <[email-masked]>
Date: Thu, 19 Jul 2018 06:37:39 -0400 (EDT)

Hello David,

All is nearly well, although "make docs" found a typo. I should have 
tested doc building before, sorry for this new round which should have 
been avoided.

   "<replacable>null_expression</replaceable>"

s/<replacable>/<replaceable>/;

Also, while reading the full documentation about the setting: I find this 
sentence a bit strange:

"""
But new users are frequently confused about the semantics of expressions 
involving null values, so this option is off by default.
"""

It appears to justify why the option is off by default, but this is not 
the actual reason, which was already provided at the end of the preceding 
paragraph.

I would suggest to remove this "But..." sentence, or turn it differently, 
maybe something like:

""" New users are frequently confused about the semantics of expressions 
involving null values: beware that turning this option on might bring even 
more confusion. """

-- 
Fabien.

---

From: David Fetter <[email-masked]>
Date: Fri, 20 Jul 2018 07:06:37 +0200

On Thu, Jul 19, 2018 at 06:37:39AM -0400, Fabien COELHO wrote:
> 
> Hello David,
> 
> All is nearly well, although "make docs" found a typo. I should have tested
> doc building before, sorry for this new round which should have been
> avoided.
> 

Fixed.

> It appears to justify why the option is off by default, but this is not the
> actual reason, which was already provided at the end of the preceding
> paragraph.
> 

I've changed it to something that makes more sense to me. Hope it
makes more sense to others.

Best,
David.

Remember to vote!

---

From: Fabien COELHO <[email-masked]>
Date: Sat, 21 Jul 2018 16:23:14 -0400 (EDT)

Hello David,

> makes more sense to others.

Ok, all is fine for me.

I could not find a CF entry, so I created one:

 	https://commitfest.postgresql.org/19/1728/

-- 
Fabien.

---

From: David Fetter <[email-masked]>
Date: Sat, 21 Jul 2018 22:29:49 +0200

> 	https://commitfest.postgresql.org/19/1728/

Thanks!

Best,
David.

Remember to vote!