On Tue, Jul 17, 2018 at 08:34:17AM -0400, Fabien COELHO wrote:
>
> Hello David,
>
> A few comments about this v2.
>
> ISTM that there is quite strong opposition to having "warn" as a default, so
> probably you should set if "off"?
Done.
> >>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?
>
> At least, I can see the logic.
I've now ordered it in what seems like a reasonable way, namely all
the "off" options, then the "on" option.
> >>Or anything in better English.
> >
> >I've changed this, and hope it's clearer.
>
> Yep, and more elegant than my proposal.
I assure you that you expression yourself in English a good deal
better than I do in Portuguese.
> >>* 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.
>
> Cannot see it in the patch update?
Oops. They should be there now.
> >> 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.
>
> Hmmm. Probably you are right. I think that the test case deserves better
> comments, as it is most peculiar. It was added by Tom for testing CHECK
> constant NULL expressions simplications.
>
> TRUE OR NULL is TRUE, but FALSE OR NULL is NULL. Why not. Combined with the
> fact that NULL is considered ok for a CHECK, this lead to strange but
> intentional behavior, such as:
>
> -- this CHECK is always nignored in practice
> CREATE TABLE foo (i INT CHECK(i = 1 OR NULL));
> INSERT INTO foo(i) VALUES(2); # ACCEPTED
>
> -- but this one is not
> CREATE TABLE foo (i INT CHECK(i = 1 OR FALSE));
> INSERT INTO foo(i) VALUES(2); # REFUSED
>
> Can't say I'm thrilled about that, and the added warning looks appropriate.
Per request, the warning is off by default, so the warning went away.
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 87d004f2a054a0c0999145aa9b44d97d7951b19c 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 transform_null_equals into an enum
To: pgsql-hack...@postgresql.org
The transform_null_equals GUC can now be off, warn, error, or 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/sql/guc.sql | 18 ++++++++
7 files changed, 128 insertions(+), 27 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4d48d93305..b0d951190f 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8066,7 +8066,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>
@@ -8074,15 +8074,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>off</literal>, <literal>warn</literal>, <literal>error</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 "off"
+ by default.
</para>
<para>
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 385e54a9b6..4ee31b3cb3 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_OFF;
/*
* 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)));
+
+ /*
+ * 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 a88ea6cfc9..d441bccb10 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},
+ {"false", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"no", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"0", TRANSFORM_NULL_EQUALS_OFF, true},
+ {"warn", TRANSFORM_NULL_EQUALS_WARN, false},
+ {"error", TRANSFORM_NULL_EQUALS_ERR, false},
+ {"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},
@@ -1414,19 +1431,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."),
@@ -4074,6 +4078,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).")
+ },
+ &transform_null_equals,
+ TRANSFORM_NULL_EQUALS_OFF, 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 c0d3fb8491..07dda7b746 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -656,7 +656,7 @@
# - Other Platforms and Clients -
-#transform_null_equals = off
+#transform_null_equals = off # off, warn, error, or on
#------------------------------------------------------------------------------
diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h
index e5aff61b8f..2f3dc28858 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_WARN, /* Issue a warning */
+ TRANSFORM_NULL_EQUALS_ERR, /* Error out */
+ 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..f50a1f153c 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 = off;
+select 1=null as transform_null_equals_off;
+ transform_null_equals_off
+---------------------------
+
+(1 row)
+
+set transform_null_equals = warn;
+select 1=null as transform_null_equals_warn;
+WARNING: = NULL can only produce a NULL
+LINE 1: select 1=null as transform_null_equals_warn;
+ ^
+ transform_null_equals_warn
+----------------------------
+
+(1 row)
+
+set transform_null_equals = error;
+select 1=null as transform_null_equals_error;
+ERROR: = NULL does not comply with the SQL specification
+LINE 1: select 1=null as transform_null_equals_error;
+ ^
+set transform_null_equals = on;
+select 1=null as transform_null_equals_on;
+ transform_null_equals_on
+--------------------------
+ f
+(1 row)
+
+reset transform_null_equals;
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 23e5029780..4b76e75048 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 = off;
+
+select 1=null as transform_null_equals_off;
+
+set transform_null_equals = warn;
+
+select 1=null as transform_null_equals_warn;
+
+set transform_null_equals = error;
+
+select 1=null as transform_null_equals_error;
+
+set transform_null_equals = on;
+
+select 1=null as transform_null_equals_on;
+
+reset transform_null_equals;
--
2.17.1
