On Wed, Jul 18, 2018 at 08:25:46AM -0400, Fabien COELHO wrote:
>
> Hello David,
>
> >I assure you that you expression yourself in English a good deal
> >better than I do in Portuguese.
>
> Alas, despite a Portuguese "rabbit" name, I cannot speak the language which
> got lost between generations.
I fear my kids may lose French the same way. Mine's already not great,
and that's in just one generation.
> About this v3: Patch applies, compiles, "make check" ok.
>
> A few minor comments:
>
> Variable "need_transform_null_equals" may be better named "is_null_equals",
> because there is no "need" of the transformation as such, and the expression
> just checks for the pattern, really.
Done.
> I'm fine with the off/warn/error/on order.
>
> Doc could mention that the transformation allows compatibility with other
> products, without naming them? Or not.
I don't see a point in obscuring the origin.
> In doc, the list of valid values on a long line, where the practice seems to
> wrap around after about 80 columns in the XML file.
Fixed.
> I notice that the feature was not tested at all before this patch:-(
>
> Maybe there could be one test which results to true, which is the whole
> point of the transformation? eg "SELECT NULL = NULL".
Done.
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 1c0d16c79dba628ec10fd1554574cf497b3dfffb 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 v4
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.17.1"
This is a multi-part message in MIME format.
--------------2.17.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
The transform_null_equals GUC can now be off, warn, error, or on.
---
doc/src/sgml/config.sgml | 25 ++++++---
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 | 55 +++++++++++++++++++
src/test/regress/sql/guc.sql | 26 +++++++++
7 files changed, 164 insertions(+), 29 deletions(-)
--------------2.17.1
Content-Type: text/x-patch; name="0001-Make-transform_null_equals-into-an-enum-v4.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-Make-transform_null_equals-into-an-enum-v4.patch"
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4d48d93305..6468c2253f 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,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</term>
<listitem>
<para>
- When on, expressions of the form <literal><replaceable>expr</replaceable> =
- NULL</literal> (or <literal>NULL =
- <replaceable>expr</replaceable></literal>) are treated as
+ 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..53970a8ce6 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 is_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 &&
+ is_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 (is_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 (is_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 (is_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..cc4fdf2de8 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -767,3 +767,58 @@ 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)
+
+select null=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)
+
+select null=null as transform_null_equals_warn;
+WARNING: = NULL can only produce a NULL
+LINE 1: select null=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;
+ ^
+select null=null as transform_null_equals_error;
+ERROR: = NULL does not comply with the SQL specification
+LINE 1: select null=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)
+
+select null=null as transform_null_equals_on;
+ transform_null_equals_on
+--------------------------
+ t
+(1 row)
+
+reset transform_null_equals;
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 23e5029780..24fa8c5308 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -288,3 +288,29 @@ 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;
+
+select null=null as transform_null_equals_off;
+
+set transform_null_equals = warn;
+
+select 1=null as transform_null_equals_warn;
+
+select null=null as transform_null_equals_warn;
+
+set transform_null_equals = error;
+
+select 1=null as transform_null_equals_error;
+
+select null=null as transform_null_equals_error;
+
+set transform_null_equals = on;
+
+select 1=null as transform_null_equals_on;
+
+select null=null as transform_null_equals_on;
+
+reset transform_null_equals;
--------------2.17.1--
