From a9ff5dbd9139c83bd6241765f74c3b637059e87b Mon Sep 17 00:00:00 2001 From: David Fetter 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' - transform_null_equals (boolean) + transform_null_equals (enum) IS NULL transform_null_equals configuration parameter @@ -8044,15 +8044,19 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' - When on, expressions of the form expr = + When enabled, 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 SQL-spec-compliant behavior of - expr = NULL is to always - return null (unknown). Therefore this parameter defaults to - off. + expr = null_expression + is to always return null (unknown); PostgreSQL allows NULL + as a null-valued expression of unknown type, which is an extension to the spec. + The transformation of expr = NULL + to expr IS NULL is inconsistent + with the normal semantics of the SQL specification, and is therefore set to "warn" + by default. 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