Thread: Make foo=null a warning by default.
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
Attachment
On 16/07/18 04:40, David Fetter wrote: > 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). I don't agree with changing the default to 'warn'. "foo = NULL" is perfectly legal SQL, even if it's not very useful in practice. The use case for transform_null_equals='warn' that I see is to wean off an application that uses transform_null_equals='on', to find all the queries that rely on it. But I'm not too excited about that either. The documented case for using transform_null_equals='on' is compatibility with Microsoft Access, and this wouldn't help with that. Besides, it's easy enough to grep the application or the server log for " = NULL", to find all such queries. - Heikki
Hello David,
> 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).
That's nice, and good for students, and probably others as well:-)
A few comments:
Patch applies & compiles, "make check" ok.
#backslash_quote = safe_encoding # on, off, or safe_encoding
[...]
#transform_null_equals = warn
I think it would be nice to have the possible values as a comment in
"postgresql.conf".
* Code:
-bool operator_precedence_warning = false;
+bool operator_precedence_warning = false;
Is this reindentation useful/needed?
+ parser_errposition(pstate, a->location)));
+ if (need_transform_null_equals && transform_null_equals == TRANSFORM_NULL_EQUALS_ON)
For consistency, maybe skip a line before the if?
transform_null_equals_options[] = { [...]
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.
Guc help text: ISTM that it should spell out the possible values and their
behavior. Maybe something like:
"""
When turned on, turns expr = NULL into expr IS NULL.
With off, warn or error, do not do so and be silent, issue a warning or an error.
The standard behavior is not to perform this transformation.
Default is warn.
"""
Or anything in better English.
* 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];
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"?
Maybe the behavior could be retested after the reset?
* Documentation:
The "error" value is not documented.
More generally, The value is said to be an enum, but the list of values is
not listed anywhere in the documentation.
ISTM that the doc would be clearer if for each of the four values of the
parameter the behavior is spelled out.
Maybe "warn" in the doc should be <literal>warn</literal> or something.
--
Fabien.
Hello Heikki, > The use case for transform_null_equals='warn' that I see is to wean off an > application that uses transform_null_equals='on', to find all the queries > that rely on it. But I'm not too excited about that either. The documented > case for using transform_null_equals='on' is compatibility with Microsoft > Access, and this wouldn't help with that. Besides, it's easy enough to grep > the application or the server log for " = NULL", to find all such queries. I'm in favor in having this feature, even if off by default, because I could enable it and it would help my students when learning SQL in interactive sessions, where "grep '= *NULL'" cannot be issued. -- Fabien.
On Mon, Jul 16, 2018 at 3:49 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I don't agree with changing the default to 'warn'. "foo = NULL" is perfectly > legal SQL, even if it's not very useful in practice. +1. I think that will probably generate lots of annoying warnings in server logs compared to the value it adds. I don't object to having an optional feature to generate warnings, but I think it should be something a particular user needs to activate, not something we enable by default. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 16/07/18 04:40, David Fetter wrote:
>> 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).
> I don't agree with changing the default to 'warn'. "foo = NULL" is
> perfectly legal SQL, even if it's not very useful in practice.
I think that there's a very narrow argument to be made that SQL doesn't
allow a NULL literal with context-determined type in this context. But
we decided to generalize that restriction long ago, and suddenly deciding
to enforce it only in this one context makes no sense to me. The idea
that we would ever decide that it's an error seems flat out ridiculous.
TBH I'm not really excited about investing any work in this area at all.
Considering how seldom we hear any questions about transform_null_equals
anymore[1], I'm wondering if we couldn't just rip the "feature" out
entirely. But to the extent that we want to leave it in place, it's
100% for backwards compatibility, and that is a strong argument against
changing anything about it.
I'm also pretty dubious that issuing a warning here will accomplish much
to help anyone find bugs. There are too many small variants of the same
problem that it will not catch[2].
regards, tom lane
[1] In a quick search, the most recent discussion I could find was from
2011:
https://www.postgresql.org/message-id/flat/201110070729.p977Tdcx075504@wwwmaster.postgresql.org
Before that it came up maybe once a year or so, but it's just fallen
off a cliff since then. I wonder whether Microsoft fixed Access to
not need it.
[2] Compare for instance the discussion about bug #6064,
https://www.postgresql.org/message-id/flat/4DFE481F020000250003E8EA%40gw.wicourts.gov
A very large fraction of the pre-2011 threads mentioning
transform_null_equals are essentially of the form "why didn't/can't
transform_null_equals fix this bad code for me?". Each of those cases
would also be a case that the proposed warning doesn't catch.
On 16/07/18 18:10, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> On 16/07/18 04:40, David Fetter wrote: >>> 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). >> >> I don't agree with changing the default to 'warn'. "foo = NULL" is >> perfectly legal SQL, even if it's not very useful in practice. > > I think that there's a very narrow argument to be made that SQL doesn't > allow a NULL literal with context-determined type in this context. But > we decided to generalize that restriction long ago, and suddenly deciding > to enforce it only in this one context makes no sense to me. The idea > that we would ever decide that it's an error seems flat out ridiculous. > > TBH I'm not really excited about investing any work in this area at all. > Considering how seldom we hear any questions about transform_null_equals > anymore[1], I'm wondering if we couldn't just rip the "feature" out > entirely. Yeah, I was wondering about that too. But Fabien brought up a completely new use-case for this: people learning SQL. For beginners who don't understand the behavior of NULLs yet, I can see a warning or error being useful training wheels. Perhaps a completely new "training_wheels=on" option, which could check may for many other beginner errors, too, would be better for that. - Heikki
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 16/07/18 18:10, Tom Lane wrote:
>> TBH I'm not really excited about investing any work in this area at all.
>> Considering how seldom we hear any questions about transform_null_equals
>> anymore[1], I'm wondering if we couldn't just rip the "feature" out
>> entirely.
> Yeah, I was wondering about that too. But Fabien brought up a completely
> new use-case for this: people learning SQL. For beginners who don't
> understand the behavior of NULLs yet, I can see a warning or error being
> useful training wheels. Perhaps a completely new "training_wheels=on"
> option, which could check may for many other beginner errors, too, would
> be better for that.
Agreed --- but what we'd want that to do seems only vaguely related to
the existing behavior of transform_null_equals. As an example, we
intentionally made transform_null_equals *not* trigger on
CASE x WHEN NULL THEN ...
but a training-wheels warning for that would likely be reasonable.
For that matter, many of the old threads about this are complaining
about nulls that aren't simple literals in the first place. I wonder
whether a training-wheels feature that whined *at runtime* about null
WHERE-qual or case-test results would be more useful than a parser
check.
regards, tom lane
On Mon, Jul 16, 2018 at 09:49:14AM +0200, Fabien COELHO wrote:
> Hello David,
>
> >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).
>
> That's nice, and good for students, and probably others as well:-)
>
> A few comments:
>
> Patch applies & compiles, "make check" ok.
>
> #backslash_quote = safe_encoding # on, off, or safe_encoding
> [...]
> #transform_null_equals = warn
Fixed.
> I think it would be nice to have the possible values as a comment in
> "postgresql.conf".
>
> * Code:
>
> -bool operator_precedence_warning = false;
> +bool operator_precedence_warning = false;
>
> Is this reindentation useful/needed?
I believe so because it fits with the line just below it.
> + parser_errposition(pstate, a->location)));
> + if (need_transform_null_equals && transform_null_equals == TRANSFORM_NULL_EQUALS_ON)
>
> For consistency, maybe skip a line before the if?
Fixed.
> transform_null_equals_options[] = { [...]
>
> 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?
> Guc help text: ISTM that it should spell out the possible values and their
> behavior. Maybe something like:
>
> """
> When turned on, turns expr = NULL into expr IS NULL.
> With off, warn or error, do not do so and be silent, issue a warning or an error.
> The standard behavior is not to perform this transformation.
> Default is warn.
> """
>
> Or anything in better English.
I've changed this, and hope it's clearer.
> * 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.
> 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.
> Maybe the behavior could be retested after the reset?
>
> * Documentation:
>
> The "error" value is not documented.
>
> More generally, The value is said to be an enum, but the list of values is
> not listed anywhere in the documentation.
>
> ISTM that the doc would be clearer if for each of the four values of the
> parameter the behavior is spelled out.
>
> Maybe "warn" in the doc should be <literal>warn</literal> or something.
Fixed.
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
Attachment
On Mon, Jul 16, 2018 at 11:37:28AM -0400, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > On 16/07/18 18:10, Tom Lane wrote: > >> TBH I'm not really excited about investing any work in this area at all. > >> Considering how seldom we hear any questions about transform_null_equals > >> anymore[1], I'm wondering if we couldn't just rip the "feature" out > >> entirely. > > > Yeah, I was wondering about that too. But Fabien brought up a completely > > new use-case for this: people learning SQL. For beginners who don't > > understand the behavior of NULLs yet, I can see a warning or error being > > useful training wheels. Perhaps a completely new "training_wheels=on" > > option, which could check may for many other beginner errors, too, would > > be better for that. > > Agreed --- but what we'd want that to do seems only vaguely related to > the existing behavior of transform_null_equals. As an example, we > intentionally made transform_null_equals *not* trigger on > > CASE x WHEN NULL THEN ... > > but a training-wheels warning for that would likely be reasonable. > > For that matter, many of the old threads about this are complaining > about nulls that aren't simple literals in the first place. I wonder > whether a training-wheels feature that whined *at runtime* about null > WHERE-qual or case-test results would be more useful than a parser > check. Am I understanding correctly that this would be looking for bogus conditions in the plan tree? 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
Hello, > Yeah, I was wondering about that too. But Fabien brought up a completely new > use-case for this: people learning SQL. Indeed. This year, about 30% of my students wrote "= NULL" in a query at least once. Probably I or a colleague were called to the rescue for help. So this warning would save me time, which explains why I reviewed the patch with such enthousiasm:-) I'm fine with keeping the current behavior as a default. -- Fabien.
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"?
>> transform_null_equals_options[] = { [...]
>>
>> 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.
>> Or anything in better English.
>
> I've changed this, and hope it's clearer.
Yep, and more elegant than my proposal.
>> * 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?
>> 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.
--
Fabien.
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
Attachment
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. 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. 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. 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. 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". -- Fabien.
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
Attachment
> [...] Done.
All looks well, but I just noticed a warning:
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation-O2 -I. -I. -I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c
guc.c:4090:3: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
&transform_null_equals,
^
guc.c:4090:3: note: (near initialization for ‘ConfigureNamesEnum[16].variable’)
Too bad, gcc does not like an (enum*) there. Maybe cast to (int *), or
declare the variable as an int (which seems to be what is done for
others)?
--
Fabien.
On Wed, Jul 18, 2018 at 04:15:30PM -0400, Fabien COELHO wrote: > > >[...] Done. > > All looks well, but I just noticed a warning: > > gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation-O2 -I. -I. -I../../../../src/include -D_GNU_SOURCE -c -o guc.o guc.c > guc.c:4090:3: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types] > &transform_null_equals, > ^ > guc.c:4090:3: note: (near initialization for ‘ConfigureNamesEnum[16].variable’) > > Too bad, gcc does not like an (enum*) there. Maybe cast to (int *), or > declare the variable as an int (which seems to be what is done for others)? Done that way. 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
Attachment
Hello David, All is nearly well, although "make docs" found a typo. I should have tested doc building before, sorry for this new round which should have been avoided. "<replacable>null_expression</replaceable>" s/<replacable>/<replaceable>/; Also, while reading the full documentation about the setting: I find this sentence a bit strange: """ But new users are frequently confused about the semantics of expressions involving null values, so this option is off by default. """ It appears to justify why the option is off by default, but this is not the actual reason, which was already provided at the end of the preceding paragraph. I would suggest to remove this "But..." sentence, or turn it differently, maybe something like: """ New users are frequently confused about the semantics of expressions involving null values: beware that turning this option on might bring even more confusion. """ -- Fabien.
On Thu, Jul 19, 2018 at 06:37:39AM -0400, Fabien COELHO wrote: > > Hello David, > > All is nearly well, although "make docs" found a typo. I should have tested > doc building before, sorry for this new round which should have been > avoided. > > "<replacable>null_expression</replaceable>" > > s/<replacable>/<replaceable>/; Fixed. > Also, while reading the full documentation about the setting: I find this > sentence a bit strange: > > """ > But new users are frequently confused about the semantics of expressions > involving null values, so this option is off by default. > """ > > It appears to justify why the option is off by default, but this is not the > actual reason, which was already provided at the end of the preceding > paragraph. > > I would suggest to remove this "But..." sentence, or turn it differently, > maybe something like: > > """ New users are frequently confused about the semantics of expressions > involving null values: beware that turning this option on might bring even > more confusion. """ I've changed it to something that makes more sense to me. Hope it makes more sense to others. 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
Attachment
Hello David,
>> [...]
>
> Fixed.
>
>> [...]
>
> I've changed it to something that makes more sense to me. Hope it
> makes more sense to others.
Ok, all is fine for me.
I could not find a CF entry, so I created one:
https://commitfest.postgresql.org/19/1728/
--
Fabien.
On Sat, Jul 21, 2018 at 04:23:14PM -0400, Fabien COELHO wrote: > > Hello David, > > >>[...] > > > >Fixed. > > > >>[...] > > > >I've changed it to something that makes more sense to me. Hope it > >makes more sense to others. > > Ok, all is fine for me. > > I could not find a CF entry, so I created one: > > https://commitfest.postgresql.org/19/1728/ Thanks! 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
hOn Mon, Jul 16, 2018 at 11:37:28AM -0400, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > On 16/07/18 18:10, Tom Lane wrote: > >> TBH I'm not really excited about investing any work in this area at all. > >> Considering how seldom we hear any questions about transform_null_equals > >> anymore[1], I'm wondering if we couldn't just rip the "feature" out > >> entirely. > > > Yeah, I was wondering about that too. But Fabien brought up a completely > > new use-case for this: people learning SQL. For beginners who don't > > understand the behavior of NULLs yet, I can see a warning or error being > > useful training wheels. Perhaps a completely new "training_wheels=on" > > option, which could check may for many other beginner errors, too, would > > be better for that. > > Agreed --- but what we'd want that to do seems only vaguely related to > the existing behavior of transform_null_equals. As an example, we > intentionally made transform_null_equals *not* trigger on > > CASE x WHEN NULL THEN ... > > but a training-wheels warning for that would likely be reasonable. > > For that matter, many of the old threads about this are complaining > about nulls that aren't simple literals in the first place. I wonder > whether a training-wheels feature that whined *at runtime* about null > WHERE-qual or case-test results would be more useful than a parser > check. I will again say I would love to see this as part of a wholesale "novice" mode which warns of generally bad SQL practices. I don't see this one item alone as sufficiently useful. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
> On Tue, Aug 7, 2018 at 11:19 PM Bruce Momjian <bruce@momjian.us> wrote: > > On Mon, Jul 16, 2018 at 11:37:28AM -0400, Tom Lane wrote: > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > > On 16/07/18 18:10, Tom Lane wrote: > > >> TBH I'm not really excited about investing any work in this area at all. > > >> Considering how seldom we hear any questions about transform_null_equals > > >> anymore[1], I'm wondering if we couldn't just rip the "feature" out > > >> entirely. > > > > > Yeah, I was wondering about that too. But Fabien brought up a completely > > > new use-case for this: people learning SQL. For beginners who don't > > > understand the behavior of NULLs yet, I can see a warning or error being > > > useful training wheels. Perhaps a completely new "training_wheels=on" > > > option, which could check may for many other beginner errors, too, would > > > be better for that. > > > > Agreed --- but what we'd want that to do seems only vaguely related to > > the existing behavior of transform_null_equals. As an example, we > > intentionally made transform_null_equals *not* trigger on > > > > CASE x WHEN NULL THEN ... > > > > but a training-wheels warning for that would likely be reasonable. > > > > For that matter, many of the old threads about this are complaining > > about nulls that aren't simple literals in the first place. I wonder > > whether a training-wheels feature that whined *at runtime* about null > > WHERE-qual or case-test results would be more useful than a parser > > check. > > I will again say I would love to see this as part of a wholesale > "novice" mode which warns of generally bad SQL practices. I don't see > this one item alone as sufficiently useful. Valid point. Maybe then at least we can outline what kind of bad SQL practices could be included into this "novice mode" to make it sufficiently useful? Otherwise this sounds like a boundless problem.
> On Mon, Nov 26, 2018 at 9:19 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote: > > > On Tue, Aug 7, 2018 at 11:19 PM Bruce Momjian <bruce@momjian.us> wrote: > > > > On Mon, Jul 16, 2018 at 11:37:28AM -0400, Tom Lane wrote: > > > Heikki Linnakangas <hlinnaka@iki.fi> writes: > > > > On 16/07/18 18:10, Tom Lane wrote: > > > >> TBH I'm not really excited about investing any work in this area at all. > > > >> Considering how seldom we hear any questions about transform_null_equals > > > >> anymore[1], I'm wondering if we couldn't just rip the "feature" out > > > >> entirely. > > > > > > > Yeah, I was wondering about that too. But Fabien brought up a completely > > > > new use-case for this: people learning SQL. For beginners who don't > > > > understand the behavior of NULLs yet, I can see a warning or error being > > > > useful training wheels. Perhaps a completely new "training_wheels=on" > > > > option, which could check may for many other beginner errors, too, would > > > > be better for that. > > > > > > Agreed --- but what we'd want that to do seems only vaguely related to > > > the existing behavior of transform_null_equals. As an example, we > > > intentionally made transform_null_equals *not* trigger on > > > > > > CASE x WHEN NULL THEN ... > > > > > > but a training-wheels warning for that would likely be reasonable. > > > > > > For that matter, many of the old threads about this are complaining > > > about nulls that aren't simple literals in the first place. I wonder > > > whether a training-wheels feature that whined *at runtime* about null > > > WHERE-qual or case-test results would be more useful than a parser > > > check. > > > > I will again say I would love to see this as part of a wholesale > > "novice" mode which warns of generally bad SQL practices. I don't see > > this one item alone as sufficiently useful. > > Valid point. Maybe then at least we can outline what kind of bad SQL practices > could be included into this "novice mode" to make it sufficiently useful? > Otherwise this sounds like a boundless problem. Due to lack of response I'm marking this as returned with feedback. Of course feel free to resubmit it.