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