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