Re: Make foo=null a warning by default.

From: David Fetter <david(at)fetter(dot)org>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
Cc: PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Make foo=null a warning by default.
Date: 2018-07-16 15:59:34
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Jul 16, 2018 at 09:49:14AM +0200, Fabien COELHO wrote:
> Hello David,
> >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).
> 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?

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

> + 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.

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

> 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.

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:
> select 1=null as expect_{true,false}[_and_a_warning/error];

Changed to more descriptive headers.

> 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"?

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 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.


David Fetter <david(at)fetter(dot)org>
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres:

Attachment Content-Type Size
0001-Make-foo-null-a-warning-by-default.patch text/x-diff 11.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2018-07-16 16:15:46 Re: [HACKERS] logical decoding of two-phase transactions
Previous Message Heikki Linnakangas 2018-07-16 15:47:45 Re: Vacuum: allow usage of more than 1GB of work mem