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
>From 33b9f51b1374ede71a6479d44adfa0588c7fb4c3 Mon Sep 17 00:00:00 2001
From: David Fetter <da...@fetter.org>
Date: Sun, 15 Jul 2018 16:11:08 -0700
Subject: [PATCH] Make foo=null a warning by default v2
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.17.1"
This is a multi-part message in MIME format.
--------------2.17.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
The transform_null_equals GUC is now an enum consisting of warn, error, off, on.
---
doc/src/sgml/config.sgml | 20 ++++++---
src/backend/parser/parse_expr.c | 30 ++++++++++---
src/backend/utils/misc/guc.c | 44 +++++++++++++------
src/backend/utils/misc/postgresql.conf.sample | 2 +-
src/include/parser/parse_expr.h | 11 ++++-
src/test/regress/expected/guc.out | 30 +++++++++++++
src/test/regress/expected/inherit.out | 1 +
src/test/regress/sql/guc.sql | 18 ++++++++
8 files changed, 129 insertions(+), 27 deletions(-)
--------------2.17.1
Content-Type: text/x-patch; name="0001-Make-foo-null-a-warning-by-default.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Make-foo-null-a-warning-by-default.patch"
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e307bb4e8e..62cccc6a2d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8036,7 +8036,7 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
<variablelist>
<varlistentry id="guc-transform-null-equals" xreflabel="transform_null_equals">
- <term><varname>transform_null_equals</varname> (<type>boolean</type>)
+ <term><varname>transform_null_equals</varname> (<type>enum</type>)
<indexterm><primary>IS NULL</primary></indexterm>
<indexterm>
<primary><varname>transform_null_equals</varname> configuration parameter</primary>
@@ -8044,15 +8044,23 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</term>
<listitem>
<para>
- When on, expressions of the form <literal><replaceable>expr</replaceable> =
+ When enabled, expressions of the form <literal><replaceable>expr</replaceable> =
NULL</literal> (or <literal>NULL =
<replaceable>expr</replaceable></literal>) are treated as
<literal><replaceable>expr</replaceable> IS NULL</literal>, that is, they
return true if <replaceable>expr</replaceable> evaluates to the null value,
- and false otherwise. The correct SQL-spec-compliant behavior of
- <literal><replaceable>expr</replaceable> = NULL</literal> is to always
- return null (unknown). Therefore this parameter defaults to
- <literal>off</literal>.
+ and false otherwise. Valid values are <literal>warn</literal>, <literal>error</literal>, <literal>off</literal>, and <literal>on</literal>.
+ </para>
+
+ <para>
+ The correct SQL-spec-compliant behavior of
+ <literal><replaceable>expr</replaceable> = <replacable>null_expression</replaceable></literal>
+ is to always return null (unknown); PostgreSQL allows <literal>NULL</literal>
+ as a null-valued expression of unknown type, which is an extension to the spec.
+ The transformation of <literal><replaceable>expr</replaceable> = NULL</literal>
+ to <literal><replaceable>expr</replaceable> IS NULL</literal> is inconsistent
+ with the normal semantics of the SQL specification, and is therefore set to "warn"
+ by default.
</para>
<para>
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a9b6..58e3e07382 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -41,8 +41,8 @@
/* GUC parameters */
-bool operator_precedence_warning = false;
-bool Transform_null_equals = false;
+bool operator_precedence_warning = false;
+enum TransformNullEquals transform_null_equals = TRANSFORM_NULL_EQUALS_WARN;
/*
* Node-type groups for operator precedence warnings
@@ -850,6 +850,7 @@ transformAExprOp(ParseState *pstate, A_Expr *a)
Node *lexpr = a->lexpr;
Node *rexpr = a->rexpr;
Node *result;
+ bool need_transform_null_equals = false;
if (operator_precedence_warning)
{
@@ -872,17 +873,34 @@ transformAExprOp(ParseState *pstate, A_Expr *a)
}
/*
- * Special-case "foo = NULL" and "NULL = foo" for compatibility with
+ * Deal with "foo = NULL" and "NULL = foo" for compatibility with
* standards-broken products (like Microsoft's). Turn these into IS NULL
* exprs. (If either side is a CaseTestExpr, then the expression was
* generated internally from a CASE-WHEN expression, and
* transform_null_equals does not apply.)
*/
- if (Transform_null_equals &&
- list_length(a->name) == 1 &&
+ need_transform_null_equals = (list_length(a->name) == 1 &&
strcmp(strVal(linitial(a->name)), "=") == 0 &&
(exprIsNullConstant(lexpr) || exprIsNullConstant(rexpr)) &&
- (!IsA(lexpr, CaseTestExpr) &&!IsA(rexpr, CaseTestExpr)))
+ (!IsA(lexpr, CaseTestExpr) &&!IsA(rexpr, CaseTestExpr)));
+
+ if (need_transform_null_equals && transform_null_equals == TRANSFORM_NULL_EQUALS_WARN)
+ ereport(WARNING,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("= NULL can only produce a NULL"),
+ parser_errposition(pstate, a->location)));
+
+ if (need_transform_null_equals && transform_null_equals == TRANSFORM_NULL_EQUALS_ERR)
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("= NULL does not comply with the SQL specification"),
+ parser_errposition(pstate, a->location)));
+
+ /*
+ * We don't need to handle TRANSFORM_NULL_EQUALS_OFF here because it's a noop
+ */
+
+ if (need_transform_null_equals && transform_null_equals == TRANSFORM_NULL_EQUALS_ON)
{
NullTest *n = makeNode(NullTest);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 17292e04fe..3f7f5c9b74 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -310,6 +310,23 @@ static const struct config_enum_entry track_function_options[] = {
{NULL, 0, false}
};
+/*
+ * Accept the usual variants of boolean-ish, along with warn and error.
+ */
+static const struct config_enum_entry transform_null_equals_options[] = {
+ {"warn", TRANSFORM_NULL_EQUALS_WARN, false},
+ {"error", TRANSFORM_NULL_EQUALS_ERR, false},
+ {"off", TRANSFORM_NULL_EQUALS_OFF, false},
+ {"false", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"no", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"0", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"on", TRANSFORM_NULL_EQUALS_ON, false},
+ {"true", TRANSFORM_NULL_EQUALS_ON, true},
+ {"yes", TRANSFORM_NULL_EQUALS_ON, true},
+ {"1", TRANSFORM_NULL_EQUALS_ON, true},
+ {NULL, 0, false}
+};
+
static const struct config_enum_entry xmlbinary_options[] = {
{"base64", XMLBINARY_BASE64, false},
{"hex", XMLBINARY_HEX, false},
@@ -1407,19 +1424,6 @@ static struct config_bool ConfigureNamesBool[] =
false,
NULL, NULL, NULL
},
- {
- {"transform_null_equals", PGC_USERSET, COMPAT_OPTIONS_CLIENT,
- gettext_noop("Treats \"expr=NULL\" as \"expr IS NULL\"."),
- gettext_noop("When turned on, expressions of the form expr = NULL "
- "(or NULL = expr) are treated as expr IS NULL, that is, they "
- "return true if expr evaluates to the null value, and false "
- "otherwise. The correct behavior of expr = NULL is to always "
- "return null (unknown).")
- },
- &Transform_null_equals,
- false,
- NULL, NULL, NULL
- },
{
{"db_user_namespace", PGC_SIGHUP, CONN_AUTH_AUTH,
gettext_noop("Enables per-database user names."),
@@ -4067,6 +4071,20 @@ static struct config_enum ConfigureNamesEnum[] =
NULL, NULL, NULL
},
+ {
+ {"transform_null_equals", PGC_USERSET, COMPAT_OPTIONS_CLIENT,
+ gettext_noop("Treats \"expr=NULL\" as \"expr IS NULL\"."),
+ gettext_noop("When set to on, expressions of the form expr = NULL "
+ "(or NULL = expr) are treated as expr IS NULL. When "
+ "set to off, warn, or error, they are not, with increasing "
+ "insistence. The default behavior of expr = NULL is to "
+ "return null (unknown) and issue a warning.")
+ },
+ &transform_null_equals,
+ TRANSFORM_NULL_EQUALS_WARN, transform_null_equals_options,
+ NULL, NULL, NULL
+ },
+
{
{"wal_level", PGC_POSTMASTER, WAL_SETTINGS,
gettext_noop("Set the level of information written to the WAL."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 657c3f81f8..a498c3aebc 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -655,7 +655,7 @@
# - Other Platforms and Clients -
-#transform_null_equals = off
+#transform_null_equals = warn # warn, error, off, or on
#------------------------------------------------------------------------------
diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h
index e5aff61b8f..276a631daf 100644
--- a/src/include/parser/parse_expr.h
+++ b/src/include/parser/parse_expr.h
@@ -17,10 +17,19 @@
/* GUC parameters */
extern bool operator_precedence_warning;
-extern bool Transform_null_equals;
extern Node *transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind);
extern const char *ParseExprKindName(ParseExprKind exprKind);
+typedef enum TransformNullEquals
+{
+ TRANSFORM_NULL_EQUALS_WARN = 0, /* Issue a warning */
+ TRANSFORM_NULL_EQUALS_ERR, /* Error out */
+ TRANSFORM_NULL_EQUALS_OFF, /* Disabled */
+ TRANSFORM_NULL_EQUALS_ON /* Enabled */
+} TransformNullEquals;
+
+extern TransformNullEquals transform_null_equals;
+
#endif /* PARSE_EXPR_H */
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 43ac5f5f11..561b1b6d96 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -767,3 +767,33 @@ NOTICE: text search configuration "no_such_config" does not exist
select func_with_bad_set();
ERROR: invalid value for parameter "default_text_search_config": "no_such_config"
reset check_function_bodies;
+set transform_null_equals = on;
+select 1=null;
+ ?column?
+----------
+ f
+(1 row)
+
+set transform_null_equals = off;
+select 1=null;
+ ?column?
+----------
+
+(1 row)
+
+set transform_null_equals = warn;
+select 1=null;
+WARNING: = NULL can only produce a NULL
+LINE 1: select 1=null;
+ ^
+ ?column?
+----------
+
+(1 row)
+
+set transform_null_equals = error;
+select 1=null;
+ERROR: = NULL does not comply with the SQL specification
+LINE 1: select 1=null;
+ ^
+reset transform_null_equals;
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 4f29d9f891..afbf7cb37b 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1690,6 +1690,7 @@ reset enable_bitmapscan;
--
create table cnullparent (f1 int);
create table cnullchild (check (f1 = 1 or f1 = null)) inherits(cnullparent);
+WARNING: = NULL can only produce a NULL
insert into cnullchild values(1);
insert into cnullchild values(2);
insert into cnullchild values(null);
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 23e5029780..29b8c888e2 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -288,3 +288,21 @@ set default_text_search_config = no_such_config;
select func_with_bad_set();
reset check_function_bodies;
+
+set transform_null_equals = on;
+
+select 1=null;
+
+set transform_null_equals = off;
+
+select 1=null;
+
+set transform_null_equals = warn;
+
+select 1=null;
+
+set transform_null_equals = error;
+
+select 1=null;
+
+reset transform_null_equals;
--------------2.17.1--
