From a9ff5dbd9139c83bd6241765f74c3b637059e87b Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Sun, 15 Jul 2018 16:11:08 -0700
Subject: [PATCH] Make foo=null a warning by default
To: pgsql-hackers@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

