Re: Make foo=null a warning by default. - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: Make foo=null a warning by default.
Date
Msg-id alpine.DEB.2.21.1807160910570.3767@lancre
Whole thread Raw
In response to Make foo=null a warning by default.  (David Fetter <david@fetter.org>)
Responses Re: Make foo=null a warning by default.  (David Fetter <david@fetter.org>)
List pgsql-hackers
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?

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


pgsql-hackers by date:

Previous
From: Heikki Linnakangas
Date:
Subject: Re: Make foo=null a warning by default.
Next
From: Fabien COELHO
Date:
Subject: Re: Make foo=null a warning by default.