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 a9ff5dbd9139c83bd6241765f74c3b637059e87b 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
To: pgsql-hack...@postgresql.org
The transform_null_equals GUC is now an enum consisting of on, off, warn (default), and error.
---
doc/src/sgml/config.sgml | 14 +++---
src/backend/parser/parse_expr.c | 28 +++++++++---
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, 122 insertions(+), 26 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e307bb4e8e..58ab2d213d 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,19 @@ 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>.
+ <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..c4f714792e 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,32 @@ 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)));
+
+ /*
+ * 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_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)));
+ 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..ae76b3d335 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[] = {
+ {"off", TRANSFORM_NULL_EQUALS_OFF, false},
+ {"on", TRANSFORM_NULL_EQUALS_ON, false},
+ {"warn", TRANSFORM_NULL_EQUALS_WARN, false},
+ {"error", TRANSFORM_NULL_EQUALS_ERR, false},
+ {"false", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"no", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"0", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"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 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) 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..d7ebeb83b2 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
#------------------------------------------------------------------------------
diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h
index e5aff61b8f..4b6df4c97a 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_OFF = 0, /* disabled */
+ TRANSFORM_NULL_EQUALS_ON, /* enabled */
+ TRANSFORM_NULL_EQUALS_WARN, /* Issue a warning */
+ TRANSFORM_NULL_EQUALS_ERR /* Error out */
+} 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
