Thread: Precedence of standard comparison operators
My Salesforce colleagues have been bugging me about this topic, and
since I see in a nearby thread that we may be about to break backwards
compatibility on "=>", maybe it's time to do something about this too.
To wit, that the precedence of <= >= and <> is neither sane nor standards
compliant.
Up to now, Postgres has had special precedence rules for = < and >,
but their multi-character brethren just get treated as general Op
tokens. This has assorted surprising consequences; for example you
can do this:
regression=# select * from testjsonb where j->>'space' < j->>'node';
but not this:
regression=# select * from testjsonb where j->>'space' <= j->>'node';
ERROR: operator does not exist: text <= jsonb
LINE 1: select * from testjsonb where j->>'space' <= j->>'node'; ^
HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts.
Of course the latter happens because ->> and <= have identical
precedence so the construct is parsed as((j ->> 'space') <= j) ->> 'node'
whereas < has lower precedence than user-defined operators so
the first case parses in the expected fashion.
I claim that this behavior is contrary to spec as well as being
unintuitive. Following the grammar productions in SQL99:
<where clause> ::= WHERE <search condition>
<search condition> ::= <boolean value expression>
<boolean value expression> ::= <boolean term> | <boolean value expression> OR
<booleanterm>
<boolean term> ::= <boolean factor> | <boolean term> AND <boolean factor>
<boolean factor> ::= [ NOT ] <boolean test>
<boolean test> ::= <boolean primary> [ IS [ NOT ] <truth value> ]
<truth value> ::= TRUE | FALSE | UNKNOWN
<boolean primary> ::= <predicate> | <parenthesized boolean value expression>
| <nonparenthesized value expression primary>
<parenthesized boolean value expression> ::= <left paren> <boolean value expression> <right paren>
<predicate> ::= <comparison predicate> | <between predicate> | <in
predicate> | <like predicate> | <null predicate> | <quantified comparison predicate>
| <exists predicate> | <unique predicate> | <match predicate> |
<overlapspredicate> | <similar predicate> | <distinct predicate> | <type
predicate>
<comparison predicate> ::= <row value expression> <comp op> <row value expression>
<comp op> ::= <equals operator> | <not equals operator> | <less than
operator> | <greater than operator> | <less than or equals operator> | <greater than
orequals operator>
<row value expression> ::= <row value special case> | <row value constructor>
<contextually typed row value expression> ::= <row value special case> |
<contextuallytyped row value constructor>
<row value special case> ::= <value specification> | <value expression>
So both the examples I gave should be understood as <row value special
case> value expressions related by <comp op>s. This line of reasoning
says that any non-boolean operator should bind tighter than the six
standard comparison operators, because it will necessarily be part of a
<value expression> component of a boolean expression.
We have that right for = < > but not for the other three standard-mandated
comparison operators. I think we should change the grammar so that all
six act like < > do now, that is, they should have %nonassoc precedence
just above NOT.
Another thought, looking at this closely, is that we have the precedence
of IS tests (IS NOT NULL etc) wrong as well: they should bind less tightly
than user-defined ops, not more so. I'm less excited about changing that,
but there's certainly room to argue that it's wrong per spec. It's
definitely weird that the IS tests bind more tightly than multicharacter
Ops but less tightly than + - * /.
I've not really experimented with this at all; it would be useful for
example to see how many regression tests break as a gauge for how
troublesome such changes would be. I thought I'd ask whether there's
any chance at all of such a change getting accepted before doing any
serious work on it.
Thoughts?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > the precedence of <= >= and <> is neither sane nor standards compliant. +1 That was a bit of a pain when I migrated a lot of code from Sybase ASE to PostgreSQL; I think we should conform to the standard on this, even if it breaks backward compatibility. (Of course, the release notes must warn about this in a conspicuous way.) > We have that right for = < > but not for the other three standard-mandated > comparison operators. I think we should change the grammar so that all > six act like < > do now, that is, they should have %nonassoc precedence > just above NOT. +1 > Another thought, looking at this closely, is that we have the precedence > of IS tests (IS NOT NULL etc) wrong as well: they should bind less tightly > than user-defined ops, not more so. I'm less excited about changing that, > but there's certainly room to argue that it's wrong per spec. It's > definitely weird that the IS tests bind more tightly than multicharacter > Ops but less tightly than + - * /. Again, I think it is worth it to conform to the standard. > I've not really experimented with this at all; it would be useful for > example to see how many regression tests break as a gauge for how > troublesome such changes would be. I thought I'd ask whether there's > any chance at all of such a change getting accepted before doing any > serious work on it. You will have my vote. I wonder whether it would be feasible to have an option to generate warnings (or maybe just LOG level messages?) for queries where the results could differ. (I seem to remember some queries that, when written to standard and run on PostgreSQL, silently returned results incompatible with the standard. We changed the code to generate SQL to emit extra layers of parentheses to get the same behavior on both databases, but we had to notice the problem first.) -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> the precedence of <= >= and <> is neither sane nor standards compliant.
> I wonder whether it would be feasible to have an option to generate
> warnings (or maybe just LOG level messages?) for queries where the
> results could differ.
My guess (admittedly not yet based on much) is that warnings won't be too
necessary. If a construction is parsed differently than before, you'll
get no-such-operator gripes. The case of interest is something like
a <= b %% c
which was formerly
(a <= b) %% c
and would become
a <= (b %% c)
Now, if it worked before, %% must expect a boolean left input; but the
odds are pretty good that b is not boolean.
This argument does get a lot weaker when you consider operators that
take nearly anything, such as ||; for instance if a b c are all text
then both parsings of
a <= b || c
are type-wise acceptable. But that's something that I hope most people
would've parenthesized to begin with, because (a <= b) || c is not exactly
the intuitive expectation for what you'll get.
Anyway, to answer your question, I think that Bison knows somewhere inside
when it's making a precedence-driven choice like this, but I don't believe
it's exposed in any way that we could get at easily. Perhaps there would
be a way to produce a warning if we hand-hacked the C-code bison output,
but we're not gonna do that.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kevin Grittner <kgrittn@ymail.com> writes: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> the precedence of <= >= and <> is neither sane nor standards compliant. >> I wonder whether it would be feasible to have an option to generate >> warnings (or maybe just LOG level messages?) for queries where the >> results could differ. > > My guess (admittedly not yet based on much) is that warnings won't be too > necessary. If a construction is parsed differently than before, you'll > get no-such-operator gripes. I have a memory of running into this in real-world production code and that it involved booleans. I'll see whether I posted something to the community lists about it, but it didn't take long to produce an (admittedly artificial) case where incorrect results are silently returned: test=# select 'f'::boolean = 'f'::boolean >= 'f'::boolean; ?column? ---------- f (1 row) test=# select 'f'::boolean >= 'f'::boolean >= 'f'::boolean; ?column? ---------- t (1 row) -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> I have a memory of running into this in real-world production code
> and that it involved booleans. I'll see whether I posted something
> to the community lists about it, but it didn't take long to produce
> an (admittedly artificial) case where incorrect results are
> silently returned:
> test=# select 'f'::boolean = 'f'::boolean >= 'f'::boolean;
> ?column?
> ----------
> f
> (1 row)
> test=# select 'f'::boolean >= 'f'::boolean >= 'f'::boolean;
> ?column?
> ----------
> t
> (1 row)
One of the reasons I want to make these operators %nonassoc is
so you get an error on cases like these --- if you actually meant
this, you'll be forced to parenthesize one way or the other.
regards, tom lane
Kevin Grittner <kgrittn@ymail.com> wrote: > Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Kevin Grittner <kgrittn@ymail.com> writes: >>> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> the precedence of <= >= and <> is neither sane nor standards compliant. >>> >>> I wonder whether it would be feasible to have an option to generate >>> warnings (or maybe just LOG level messages?) for queries where the >>> results could differ. >> >> My guess (admittedly not yet based on much) is that warnings won't be too >> necessary. If a construction is parsed differently than before, you'll >> get no-such-operator gripes. > > I have a memory of running into this in real-world production code > and that it involved booleans. I'll see whether I posted something > to the community lists about it [...] Here's what I posted when I ran into it in real-world code, although I posted simplified test cases rather than the (probably very complex) production code: http://www.postgresql.org/message-id/200712171958.lBHJwOBb037317@wwwmaster.postgresql.org -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Tom Lane <tgl@sss.pgh.pa.us> wrote: > One of the reasons I want to make these operators %nonassoc is > so you get an error on cases like these --- if you actually meant > this, you'll be forced to parenthesize one way or the other. I could live with that versus a configurable warning. It's simpler and makes it less likely that someone will accidentally get incorrect results without realizing it. If we confirm that the standard specifies a left-to-right evaluation (which I seem to recall, but wouldn't trust that memory without confirmation), we could consider loosening it up five or ten years down the road, once everyone has code that works with this stricter implementation. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/19/15 10:48 AM, Tom Lane wrote: > I've not really experimented with this at all; it would be useful for > example to see how many regression tests break as a gauge for how > troublesome such changes would be. I thought I'd ask whether there's > any chance at all of such a change getting accepted before doing any > serious work on it. I think we should try to do it, but we need a way for users to see what is going on. If we just put into the release notes, "the precedences of >= and <= have been changed, but we don't expect this to cause many problems", there might be wide-spread panic. One way would be to have a knob that warns/logs/debugs when it sees an <= or >= call in a place that would change meaning. Perhaps in transformAExprOp(). This might be an expensive check, but it wouldn't have to be on all the time. We could also add a flag to the A_Expr node that remember whether the expression was parenthesized, so that users could update their code with parentheses to shut the warning up. I think this would be a standard_conforming_strings-like transition.
Peter Eisentraut <peter_e@gmx.net> writes:
> I think we should try to do it, but we need a way for users to see what
> is going on. If we just put into the release notes, "the precedences of
>> = and <= have been changed, but we don't expect this to cause many
> problems", there might be wide-spread panic.
> One way would be to have a knob that warns/logs/debugs when it sees an
> <= or >= call in a place that would change meaning. Perhaps in
> transformAExprOp(). This might be an expensive check, but it wouldn't
> have to be on all the time. We could also add a flag to the A_Expr node
> that remember whether the expression was parenthesized, so that users
> could update their code with parentheses to shut the warning up.
> I think this would be a standard_conforming_strings-like transition.
We had this discussion back in 2007 :-(.
I don't believe there is any practical way for us to generate useful
warnings here; as I said to Kevin, I don't think that Bison exposes
sufficient information to detect when a parsing decision was made
differently than before because of precedence. If there's going to be
an insistence on that then I suspect we'll spend another 8 years not
conforming to spec.
regards, tom lane
On 2/20/15 2:41 PM, Tom Lane wrote: > I don't believe there is any practical way for us to generate useful > warnings here; as I said to Kevin, I don't think that Bison exposes > sufficient information to detect when a parsing decision was made > differently than before because of precedence. We could check if there is a >= or <= as a child of another general operator. That is already quite unlikely to begin with (except for the obvious common case I am forgetting right now). We could even do this in an external module with a hook. Or to be more precise, check whether the >= or <= was in parentheses, which we could record in the parser. Neither might be absolutely accurate, but it would at least give users a list of things to check. The above would imply that we add these checks before changing the precedence. Creating a check under the new precendence would be much harder.
Peter Eisentraut <peter_e@gmx.net> writes:
> We could check if there is a >= or <= as a child of another general
> operator. That is already quite unlikely to begin with (except for the
> obvious common case I am forgetting right now). We could even do this
> in an external module with a hook. Or to be more precise, check whether
> the >= or <= was in parentheses, which we could record in the parser.
> Neither might be absolutely accurate, but it would at least give users a
> list of things to check.
> The above would imply that we add these checks before changing the
> precedence. Creating a check under the new precendence would be much
> harder.
Hm. Well, assuming that we're satisfied with just having a way to warn
when the behavior changed (and not, in particular, a switch that can
select old or new behavior), I think it might not be that hard. The point
is that we're going to reduce the precedence of <= and friends, which
means that some other operator that might formerly have bound less tightly
might now bind more tightly and thereby be underneath the <= instead of
above it. So it seems like we could adapt your idea: in transformAExprOp,
if the opname is <= etc, then check to see if the righthand argument is
another A_Expr with a multicharacter Op name. If so warn. As you said,
we'd need to mark parenthesized subexpressions but that might not be
horridly painful.
regards, tom lane
Attached is a draft patch to bring the precedence of comparison operators
and IS tests into line with the SQL standard. I have not yet looked into
producing warnings for changes in parsing decisions; but I was gratified
to discover that this patch results in none, nada, zero changes in any
of our regression tests. So that's at least some evidence that it may
not be a huge problem in practice. Pending writing some code for warnings,
I thought I'd throw this up in case anyone wants to try it on applications
they have handy.
Credit where credit is due dept: this is mostly the work of Serge Rielau
of Salesforce.
regards, tom lane
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 4b81b08..c88e7d9 100644
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*************** CAST ( '<replaceable>string</replaceable
*** 984,993 ****
associativity of the operators in <productname>PostgreSQL</>.
Most operators have the same precedence and are left-associative.
The precedence and associativity of the operators is hard-wired
! into the parser. This can lead to non-intuitive behavior; for
! example the Boolean operators <literal><</> and
! <literal>></> have a different precedence than the Boolean
! operators <literal><=</> and <literal>>=</>. Also, you will
sometimes need to add parentheses when using combinations of
binary and unary operators. For instance:
<programlisting>
--- 984,994 ----
associativity of the operators in <productname>PostgreSQL</>.
Most operators have the same precedence and are left-associative.
The precedence and associativity of the operators is hard-wired
! into the parser.
! </para>
!
! <para>
! You will
sometimes need to add parentheses when using combinations of
binary and unary operators. For instance:
<programlisting>
*************** SELECT (5 !) - 6;
*** 1063,1087 ****
</row>
<row>
! <entry><token>IS</token></entry>
! <entry></entry>
! <entry><literal>IS TRUE</>, <literal>IS FALSE</>, <literal>IS NULL</>, etc</entry>
! </row>
!
! <row>
! <entry><token>ISNULL</token></entry>
! <entry></entry>
! <entry>test for null</entry>
! </row>
!
! <row>
! <entry><token>NOTNULL</token></entry>
! <entry></entry>
! <entry>test for not null</entry>
! </row>
!
! <row>
! <entry>(any other)</entry>
<entry>left</entry>
<entry>all other native and user-defined operators</entry>
</row>
--- 1064,1070 ----
</row>
<row>
! <entry>(any other operator)</entry>
<entry>left</entry>
<entry>all other native and user-defined operators</entry>
</row>
*************** SELECT (5 !) - 6;
*** 1111,1125 ****
</row>
<row>
! <entry><token><</token> <token>></token></entry>
<entry></entry>
! <entry>less than, greater than</entry>
</row>
<row>
! <entry><token>=</token></entry>
! <entry>right</entry>
! <entry>equality, assignment</entry>
</row>
<row>
--- 1094,1109 ----
</row>
<row>
! <entry><token><</token> <token>></token> <token>=</token> <token><=</token> <token>>=</token>
<token><></token>
! </entry>
<entry></entry>
! <entry>comparison operators</entry>
</row>
<row>
! <entry><token>IS</token> <token>ISNULL</token> <token>NOTNULL</token></entry>
! <entry></entry>
! <entry><literal>IS TRUE</>, <literal>IS FALSE</>, <literal>IS NULL</>, etc</entry>
</row>
<row>
*************** SELECT (5 !) - 6;
*** 1159,1165 ****
SELECT 3 OPERATOR(pg_catalog.+) 4;
</programlisting>
the <literal>OPERATOR</> construct is taken to have the default precedence
! shown in <xref linkend="sql-precedence-table"> for <quote>any other</> operator. This is true no matter
which specific operator appears inside <literal>OPERATOR()</>.
</para>
</sect2>
--- 1143,1150 ----
SELECT 3 OPERATOR(pg_catalog.+) 4;
</programlisting>
the <literal>OPERATOR</> construct is taken to have the default precedence
! shown in <xref linkend="sql-precedence-table"> for
! <quote>any other operator</>. This is true no matter
which specific operator appears inside <literal>OPERATOR()</>.
</para>
</sect2>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 36dac29..f98158c 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** static Node *makeRecursiveViewSelect(cha
*** 532,537 ****
--- 532,538 ----
%token <str> IDENT FCONST SCONST BCONST XCONST Op
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS
+ %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
/*
* If you want to make any keyword changes, update the keyword table in
*************** static Node *makeRecursiveViewSelect(cha
*** 645,652 ****
%left OR
%left AND
%right NOT
! %right '='
! %nonassoc '<' '>'
%nonassoc LIKE ILIKE SIMILAR
%nonassoc ESCAPE
%nonassoc OVERLAPS
--- 646,653 ----
%left OR
%left AND
%right NOT
! %nonassoc IS ISNULL NOTNULL /* IS sets precedence for IS NULL, etc */
! %nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
%nonassoc LIKE ILIKE SIMILAR
%nonassoc ESCAPE
%nonassoc OVERLAPS
*************** static Node *makeRecursiveViewSelect(cha
*** 676,684 ****
%nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */
%nonassoc IDENT NULL_P PARTITION RANGE ROWS PRECEDING FOLLOWING
%left Op OPERATOR /* multi-character ops and user-defined operators */
- %nonassoc NOTNULL
- %nonassoc ISNULL
- %nonassoc IS /* sets precedence for IS NULL, etc */
%left '+' '-'
%left '*' '/' '%'
%left '^'
--- 677,682 ----
*************** a_expr: c_expr { $$ = $1; }
*** 11202,11207 ****
--- 11200,11211 ----
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">", $1, $3, @2); }
| a_expr '=' a_expr
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "=", $1, $3, @2); }
+ | a_expr LESS_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<=", $1, $3, @2); }
+ | a_expr GREATER_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">=", $1, $3, @2); }
+ | a_expr NOT_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<>", $1, $3, @2); }
| a_expr qual_Op a_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
*************** b_expr: c_expr
*** 11566,11571 ****
--- 11570,11581 ----
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">", $1, $3, @2); }
| b_expr '=' b_expr
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "=", $1, $3, @2); }
+ | b_expr LESS_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<=", $1, $3, @2); }
+ | b_expr GREATER_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">=", $1, $3, @2); }
+ | b_expr NOT_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<>", $1, $3, @2); }
| b_expr qual_Op b_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
| qual_Op b_expr %prec Op
*************** MathOp: '+' { $$ = "+"; }
*** 12485,12490 ****
--- 12495,12503 ----
| '<' { $$ = "<"; }
| '>' { $$ = ">"; }
| '=' { $$ = "="; }
+ | LESS_EQUALS { $$ = "<="; }
+ | GREATER_EQUALS { $$ = ">="; }
+ | NOT_EQUALS { $$ = "<>"; }
;
qual_Op: Op
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index a78ce03..7ce7a47 100644
*** a/src/backend/parser/scan.l
--- b/src/backend/parser/scan.l
*************** ident_cont [A-Za-z\200-\377_0-9\$]
*** 331,339 ****
--- 331,344 ----
identifier {ident_start}{ident_cont}*
+ /* Assorted special-case operators and operator-like tokens */
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** other .
*** 808,813 ****
--- 813,840 ----
return COLON_EQUALS;
}
+ {less_equals} {
+ SET_YYLLOC();
+ return LESS_EQUALS;
+ }
+
+ {greater_equals} {
+ SET_YYLLOC();
+ return GREATER_EQUALS;
+ }
+
+ {less_greater} {
+ /* We accept both "<>" and "!=" as meaning NOT_EQUALS */
+ SET_YYLLOC();
+ return NOT_EQUALS;
+ }
+
+ {not_equals} {
+ /* We accept both "<>" and "!=" as meaning NOT_EQUALS */
+ SET_YYLLOC();
+ return NOT_EQUALS;
+ }
+
{self} {
SET_YYLLOC();
return yytext[0];
*************** other .
*** 885,895 ****
if (nchars >= NAMEDATALEN)
yyerror("operator too long");
! /* Convert "!=" operator to "<>" for compatibility */
! if (strcmp(yytext, "!=") == 0)
! yylval->str = pstrdup("<>");
! else
! yylval->str = pstrdup(yytext);
return Op;
}
--- 912,918 ----
if (nchars >= NAMEDATALEN)
yyerror("operator too long");
! yylval->str = pstrdup(yytext);
return Op;
}
diff --git a/src/bin/psql/psqlscan.l b/src/bin/psql/psqlscan.l
index fb3fa11..a37cd2c 100644
*** a/src/bin/psql/psqlscan.l
--- b/src/bin/psql/psqlscan.l
*************** ident_cont [A-Za-z\200-\377_0-9\$]
*** 355,363 ****
--- 355,368 ----
identifier {ident_start}{ident_cont}*
+ /* Assorted special-case operators and operator-like tokens */
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** other .
*** 669,674 ****
--- 674,695 ----
ECHO;
}
+ {less_equals} {
+ ECHO;
+ }
+
+ {greater_equals} {
+ ECHO;
+ }
+
+ {less_greater} {
+ ECHO;
+ }
+
+ {not_equals} {
+ ECHO;
+ }
+
/*
* These rules are specific to psql --- they implement parenthesis
* counting and detection of command-ending semicolon. These must
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 506a313..761cfab 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*************** static void check_raise_parameters(PLp
*** 227,232 ****
--- 227,233 ----
%token <str> IDENT FCONST SCONST BCONST XCONST Op
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS
+ %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
/*
* Other tokens recognized by plpgsql's lexer interface layer (pl_scanner.c).
I wrote:
> Attached is a draft patch to bring the precedence of comparison operators
> and IS tests into line with the SQL standard. I have not yet looked into
> producing warnings for changes in parsing decisions ...
I've made some progress on getting parse_expr.c to produce warnings by
after-the-fact analysis of the raw parse tree, and I think it can be made
to work. However, I've run into two stumbling blocks that greatly limit
the quality of the warnings, and will need to be fixed as separate
patches:
1. BooleanTest and NullTest node types don't carry location pointers,
so there's no way to give an error cursor position referencing them.
Simple enough addition. I think we left these out because there were
no post-grammar error reports involving them, but now we need some.
2. gram.y expands BETWEEN operations into complex AND/OR nests on sight,
losing the information that there was a BETWEEN there, which is important
if we're to detect possible precedence changes involving BETWEEN.
What we should do about #2 is preserve BETWEEN as a distinct construct
in the output of raw parsing. This is a good thing anyway, because in
the long run we are going to want to fix BETWEEN's semantic issues
(such as multiple evaluation of possibly-volatile input expressions)
and fixing what the grammar does with it is an essential first step.
Barring objection I'll go do both of those things as separate patches,
and then come back to the precedence warnings.
regards, tom lane
Attached is an improved patch that includes optional warnings for
constructs that changed parsing. It's not quite 100% but I think it's
about 90% correct; the difference in size between this and the previous
patch should be a pretty fair indication of what it's going to cost us
to have a warning capability.
What's missing from this version is that it can't tell the difference
between LIKE/ILIKE/SIMILAR TO and the underlying operators, that is,
it sees "a LIKE b" as "a ~~ b" because that's what the grammar emits.
However, those inputs have different operator-precedence behavior.
Likewise, we can't tell the difference between
xmlexpr IS NOT DOCUMENT
NOT (xmlexpr IS DOCUMENT)
because the grammar converts the former into the latter --- and again,
those two things have different precedence behavior.
It wouldn't take very much additional code to fix these things by changing
what the grammar emits; but I'm running out of energy for today. In any
case, I thought I should put this up and see if this general approach is
going to satisfy people's concerns about making such a change.
regards, tom lane
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6bcb106..22a2d32 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** dynamic_library_path = 'C:\tools\postgre
*** 6752,6757 ****
--- 6752,6780 ----
</listitem>
</varlistentry>
+ <varlistentry id="guc-operator-precedence-warning" xreflabel="operator_precedence_warning">
+ <term><varname>operator_precedence_warning</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>operator_precedence_warning</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ When on, the parser will emit a warning for any construct that might
+ have changed meanings since <productname>PostgreSQL</> 9.4 as a result
+ of changes in operator precedence. This is useful for auditing
+ applications to see if precedence changes have broken anything; but it
+ is not meant to be left turned on in production, since it will warn
+ about some perfectly valid, standard-compliant SQL code.
+ The default is <literal>off</>.
+ </para>
+
+ <para>
+ See <xref linkend="sql-precedence"> for more information.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-quote-all-identifiers" xreflabel="quote-all-identifiers">
<term><varname>quote_all_identifiers</varname> (<type>boolean</type>)
<indexterm>
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 4b81b08..e7484c4 100644
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*************** CAST ( '<replaceable>string</replaceable
*** 984,993 ****
associativity of the operators in <productname>PostgreSQL</>.
Most operators have the same precedence and are left-associative.
The precedence and associativity of the operators is hard-wired
! into the parser. This can lead to non-intuitive behavior; for
! example the Boolean operators <literal><</> and
! <literal>></> have a different precedence than the Boolean
! operators <literal><=</> and <literal>>=</>. Also, you will
sometimes need to add parentheses when using combinations of
binary and unary operators. For instance:
<programlisting>
--- 984,994 ----
associativity of the operators in <productname>PostgreSQL</>.
Most operators have the same precedence and are left-associative.
The precedence and associativity of the operators is hard-wired
! into the parser.
! </para>
!
! <para>
! You will
sometimes need to add parentheses when using combinations of
binary and unary operators. For instance:
<programlisting>
*************** SELECT (5 !) - 6;
*** 1008,1014 ****
</para>
<table id="sql-precedence-table">
! <title>Operator Precedence (decreasing)</title>
<tgroup cols="3">
<thead>
--- 1009,1015 ----
</para>
<table id="sql-precedence-table">
! <title>Operator Precedence (highest to lowest)</title>
<tgroup cols="3">
<thead>
*************** SELECT (5 !) - 6;
*** 1063,1087 ****
</row>
<row>
! <entry><token>IS</token></entry>
! <entry></entry>
! <entry><literal>IS TRUE</>, <literal>IS FALSE</>, <literal>IS NULL</>, etc</entry>
! </row>
!
! <row>
! <entry><token>ISNULL</token></entry>
! <entry></entry>
! <entry>test for null</entry>
! </row>
!
! <row>
! <entry><token>NOTNULL</token></entry>
! <entry></entry>
! <entry>test for not null</entry>
! </row>
!
! <row>
! <entry>(any other)</entry>
<entry>left</entry>
<entry>all other native and user-defined operators</entry>
</row>
--- 1064,1070 ----
</row>
<row>
! <entry>(any other operator)</entry>
<entry>left</entry>
<entry>all other native and user-defined operators</entry>
</row>
*************** SELECT (5 !) - 6;
*** 1111,1125 ****
</row>
<row>
! <entry><token><</token> <token>></token></entry>
<entry></entry>
! <entry>less than, greater than</entry>
</row>
<row>
! <entry><token>=</token></entry>
! <entry>right</entry>
! <entry>equality, assignment</entry>
</row>
<row>
--- 1094,1110 ----
</row>
<row>
! <entry><token><</token> <token>></token> <token>=</token> <token><=</token> <token>>=</token>
<token><></token>
! </entry>
<entry></entry>
! <entry>comparison operators</entry>
</row>
<row>
! <entry><token>IS</token> <token>ISNULL</token> <token>NOTNULL</token></entry>
! <entry></entry>
! <entry><literal>IS TRUE</>, <literal>IS FALSE</>, <literal>IS
! NULL</>, <literal>IS DISTINCT FROM</>, etc</entry>
</row>
<row>
*************** SELECT (5 !) - 6;
*** 1159,1167 ****
SELECT 3 OPERATOR(pg_catalog.+) 4;
</programlisting>
the <literal>OPERATOR</> construct is taken to have the default precedence
! shown in <xref linkend="sql-precedence-table"> for <quote>any other</> operator. This is true no matter
which specific operator appears inside <literal>OPERATOR()</>.
</para>
</sect2>
</sect1>
--- 1144,1172 ----
SELECT 3 OPERATOR(pg_catalog.+) 4;
</programlisting>
the <literal>OPERATOR</> construct is taken to have the default precedence
! shown in <xref linkend="sql-precedence-table"> for
! <quote>any other operator</>. This is true no matter
which specific operator appears inside <literal>OPERATOR()</>.
</para>
+
+ <note>
+ <para>
+ <productname>PostgreSQL</> versions prior to 9.5 used a slightly
+ different operator precedence rule; in particular, <token><=</token>
+ <token>>=</token> and <token><></token> used to be treated as
+ generic operators, and <literal>IS</> tests used to have higher priority.
+ This was changed for better compliance with the SQL standard, and because
+ treating these operators differently from the other comparison operators
+ <token><</token> <token>></token> and <token>=</token> was
+ confusing. In most cases, this change will result in no behavioral
+ change or obvious <quote>no such operator</> failures; however there are
+ corner cases in which a query might change behavior without any parsing
+ error being reported. If you are concerned about whether this change has
+ silently broken something, you can test your application with the
+ configuration parameter <xref linkend="guc-operator-precedence-warning">
+ turned on to see if any warnings are logged.
+ </para>
+ </note>
</sect2>
</sect1>
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 98aa5f0..a6d0c5e 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outAExpr(StringInfo str, const A_Expr *
*** 2532,2537 ****
--- 2532,2540 ----
appendStringInfoString(str, " NOT_BETWEEN_SYM ");
WRITE_NODE_FIELD(name);
break;
+ case AEXPR_PAREN:
+ appendStringInfoString(str, " PAREN");
+ break;
default:
appendStringInfoString(str, " ??");
break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 76b0aff..3c28349 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 58,63 ****
--- 58,64 ----
#include "nodes/nodeFuncs.h"
#include "parser/gramparse.h"
#include "parser/parser.h"
+ #include "parser/parse_expr.h"
#include "storage/lmgr.h"
#include "utils/date.h"
#include "utils/datetime.h"
*************** static Node *makeRecursiveViewSelect(cha
*** 532,537 ****
--- 533,539 ----
%token <str> IDENT FCONST SCONST BCONST XCONST Op
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS
+ %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
/*
* If you want to make any keyword changes, update the keyword table in
*************** static Node *makeRecursiveViewSelect(cha
*** 645,652 ****
%left OR
%left AND
%right NOT
! %right '='
! %nonassoc '<' '>'
%nonassoc LIKE ILIKE SIMILAR
%nonassoc ESCAPE
%nonassoc OVERLAPS
--- 647,654 ----
%left OR
%left AND
%right NOT
! %nonassoc IS ISNULL NOTNULL /* IS sets precedence for IS NULL, etc */
! %nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
%nonassoc LIKE ILIKE SIMILAR
%nonassoc ESCAPE
%nonassoc OVERLAPS
*************** static Node *makeRecursiveViewSelect(cha
*** 676,684 ****
%nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */
%nonassoc IDENT NULL_P PARTITION RANGE ROWS PRECEDING FOLLOWING
%left Op OPERATOR /* multi-character ops and user-defined operators */
- %nonassoc NOTNULL
- %nonassoc ISNULL
- %nonassoc IS /* sets precedence for IS NULL, etc */
%left '+' '-'
%left '*' '/' '%'
%left '^'
--- 678,683 ----
*************** a_expr: c_expr { $$ = $1; }
*** 11204,11209 ****
--- 11203,11214 ----
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">", $1, $3, @2); }
| a_expr '=' a_expr
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "=", $1, $3, @2); }
+ | a_expr LESS_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<=", $1, $3, @2); }
+ | a_expr GREATER_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">=", $1, $3, @2); }
+ | a_expr NOT_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<>", $1, $3, @2); }
| a_expr qual_Op a_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
*************** b_expr: c_expr
*** 11564,11569 ****
--- 11569,11580 ----
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">", $1, $3, @2); }
| b_expr '=' b_expr
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "=", $1, $3, @2); }
+ | b_expr LESS_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<=", $1, $3, @2); }
+ | b_expr GREATER_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">=", $1, $3, @2); }
+ | b_expr NOT_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<>", $1, $3, @2); }
| b_expr qual_Op b_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
| qual_Op b_expr %prec Op
*************** c_expr: columnref { $$ = $1; }
*** 11635,11640 ****
--- 11646,11669 ----
n->indirection = check_indirection($4, yyscanner);
$$ = (Node *)n;
}
+ else if (operator_precedence_warning)
+ {
+ /*
+ * If precedence warnings are enabled, insert
+ * AEXPR_PAREN nodes wrapping all explicitly
+ * parenthesized subexpressions; this prevents bogus
+ * warnings from being issued when the ordering has
+ * been forced by parentheses.
+ *
+ * In principle we should not be relying on a GUC to
+ * decide whether to insert AEXPR_PAREN nodes.
+ * However, since they have no effect except to
+ * suppress warnings, it's probably safe enough; and
+ * we'd just as soon not waste cycles on dummy parse
+ * nodes if we don't have to.
+ */
+ $$ = (Node *) makeA_Expr(AEXPR_PAREN, NIL, $2, NULL, @1);
+ }
else
$$ = $2;
}
*************** MathOp: '+' { $$ = "+"; }
*** 12483,12488 ****
--- 12512,12520 ----
| '<' { $$ = "<"; }
| '>' { $$ = ">"; }
| '=' { $$ = "="; }
+ | LESS_EQUALS { $$ = "<="; }
+ | GREATER_EQUALS { $$ = ">="; }
+ | NOT_EQUALS { $$ = "<>"; }
;
qual_Op: Op
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index f314745..de19502 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
***************
*** 37,42 ****
--- 37,44 ----
#include "utils/xml.h"
+ /* GUC parameters */
+ bool operator_precedence_warning = false;
bool Transform_null_equals = false;
static Node *transformExprRecurse(ParseState *pstate, Node *expr);
*************** static Node *make_row_distinct_op(ParseS
*** 76,81 ****
--- 78,88 ----
RowExpr *lrow, RowExpr *rrow, int location);
static Expr *make_distinct_op(ParseState *pstate, List *opname,
Node *ltree, Node *rtree, int location);
+ static int operator_precedence_group(Node *node, const char **nodename);
+ static void emit_precedence_warnings(ParseState *pstate,
+ int opgroup, const char *opname,
+ Node *lchild, Node *rchild,
+ int location);
/*
*************** transformExprRecurse(ParseState *pstate,
*** 188,193 ****
--- 195,203 ----
case AEXPR_NOT_BETWEEN_SYM:
result = transformAExprBetween(pstate, a);
break;
+ case AEXPR_PAREN:
+ result = transformExprRecurse(pstate, a->lexpr);
+ break;
default:
elog(ERROR, "unrecognized A_Expr kind: %d", a->kind);
result = NULL; /* keep compiler quiet */
*************** transformExprRecurse(ParseState *pstate,
*** 249,254 ****
--- 259,269 ----
{
NullTest *n = (NullTest *) expr;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate, 1, "IS",
+ (Node *) n->arg, NULL,
+ n->location);
+
n->arg = (Expr *) transformExprRecurse(pstate, (Node *) n->arg);
/* the argument can be any type, so don't coerce it */
n->argisrow = type_is_rowtype(exprType((Node *) n->arg));
*************** transformAExprOp(ParseState *pstate, A_E
*** 773,778 ****
--- 788,805 ----
Node *rexpr = a->rexpr;
Node *result;
+ if (operator_precedence_warning)
+ {
+ int opgroup;
+ const char *opname;
+
+ opgroup = operator_precedence_group((Node *) a, &opname);
+ if (opgroup > 0)
+ emit_precedence_warnings(pstate, opgroup, opname,
+ lexpr, rexpr,
+ a->location);
+ }
+
/*
* Special-case "foo = NULL" and "NULL = foo" for compatibility with
* standards-broken products (like Microsoft's). Turn these into IS NULL
*************** transformAExprOpAll(ParseState *pstate,
*** 877,884 ****
static Node *
transformAExprDistinct(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = transformExprRecurse(pstate, a->lexpr);
! Node *rexpr = transformExprRecurse(pstate, a->rexpr);
if (lexpr && IsA(lexpr, RowExpr) &&
rexpr && IsA(rexpr, RowExpr))
--- 904,919 ----
static Node *
transformAExprDistinct(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = a->lexpr;
! Node *rexpr = a->rexpr;
!
! if (operator_precedence_warning)
! emit_precedence_warnings(pstate, 1, "IS",
! lexpr, rexpr,
! a->location);
!
! lexpr = transformExprRecurse(pstate, lexpr);
! rexpr = transformExprRecurse(pstate, rexpr);
if (lexpr && IsA(lexpr, RowExpr) &&
rexpr && IsA(rexpr, RowExpr))
*************** transformAExprNullIf(ParseState *pstate,
*** 935,954 ****
return (Node *) result;
}
static Node *
transformAExprOf(ParseState *pstate, A_Expr *a)
{
! /*
! * Checking an expression for match to a list of type names. Will result
! * in a boolean constant node.
! */
! Node *lexpr = transformExprRecurse(pstate, a->lexpr);
Const *result;
ListCell *telem;
Oid ltype,
rtype;
bool matched = false;
ltype = exprType(lexpr);
foreach(telem, (List *) a->rexpr)
{
--- 970,996 ----
return (Node *) result;
}
+ /*
+ * Checking an expression for match to a list of type names. Will result
+ * in a boolean constant node.
+ */
static Node *
transformAExprOf(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = a->lexpr;
Const *result;
ListCell *telem;
Oid ltype,
rtype;
bool matched = false;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate, 1, "IS",
+ lexpr, NULL,
+ a->location);
+
+ lexpr = transformExprRecurse(pstate, lexpr);
+
ltype = exprType(lexpr);
foreach(telem, (List *) a->rexpr)
{
*************** transformBooleanTest(ParseState *pstate,
*** 2157,2162 ****
--- 2199,2209 ----
{
const char *clausename;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate, 1, "IS",
+ (Node *) b->arg, NULL,
+ b->location);
+
switch (b->booltesttype)
{
case IS_TRUE:
*************** make_distinct_op(ParseState *pstate, Lis
*** 2674,2679 ****
--- 2721,2904 ----
}
/*
+ * Identify node's group for operator precedence warnings
+ *
+ * Groups are:
+ * 0: everything not classified below
+ * 1: IS tests (NullTest, BooleanTest, etc)
+ * 2: < > =
+ * 3: <= => <>
+ * 4: LIKE ILIKE SIMILAR BETWEEN IN
+ * 5: generic Op
+ *
+ * For items in nonzero groups, also return a suitable node name into *nodename
+ *
+ * Note: group zero is used for nodes that are higher or lower precedence
+ * than everything that changed precedence; we need never issue warnings
+ * related to such nodes. Also, nodes in group 4 do have precedences
+ * relative to each other, but we don't care since those did not change.
+ */
+ static int
+ operator_precedence_group(Node *node, const char **nodename)
+ {
+ int group = 0;
+
+ *nodename = NULL;
+ if (node == NULL)
+ return 0;
+
+ if (IsA(node, A_Expr))
+ {
+ A_Expr *aexpr = (A_Expr *) node;
+
+ if (aexpr->kind == AEXPR_OP &&
+ aexpr->lexpr != NULL &&
+ aexpr->rexpr != NULL)
+ {
+ /* binary operator */
+ if (list_length(aexpr->name) == 1)
+ {
+ *nodename = strVal(linitial(aexpr->name));
+ /* Ignore if op was always higher priority than IS-tests */
+ if (strcmp(*nodename, "+") == 0 ||
+ strcmp(*nodename, "-") == 0 ||
+ strcmp(*nodename, "*") == 0 ||
+ strcmp(*nodename, "/") == 0 ||
+ strcmp(*nodename, "%") == 0 ||
+ strcmp(*nodename, "^") == 0)
+ group = 0;
+ else if (strcmp(*nodename, "~") == 0 ||
+ strcmp(*nodename, "!~") == 0 ||
+ strcmp(*nodename, "~~") == 0 ||
+ strcmp(*nodename, "!~~") == 0 ||
+ strcmp(*nodename, "~~*") == 0 ||
+ strcmp(*nodename, "!~~*") == 0)
+ group = 4; /* LIKE, ILIKE, SIMILAR */
+ else if (strcmp(*nodename, "<=") == 0 ||
+ strcmp(*nodename, ">=") == 0 ||
+ strcmp(*nodename, "<>") == 0)
+ group = 3;
+ else if (strcmp(*nodename, "<") == 0 ||
+ strcmp(*nodename, ">") == 0 ||
+ strcmp(*nodename, "=") == 0)
+ group = 2;
+ else
+ group = 5;
+ }
+ else
+ {
+ /* schema-qualified operator syntax */
+ *nodename = "OPERATOR()";
+ group = 5;
+ }
+ }
+ else if (aexpr->kind == AEXPR_DISTINCT ||
+ aexpr->kind == AEXPR_OF)
+ {
+ *nodename = "IS";
+ group = 1;
+ }
+ else if (aexpr->kind == AEXPR_IN)
+ {
+ *nodename = "IN";
+ group = 4;
+ }
+ else if (aexpr->kind == AEXPR_BETWEEN ||
+ aexpr->kind == AEXPR_NOT_BETWEEN ||
+ aexpr->kind == AEXPR_BETWEEN_SYM ||
+ aexpr->kind == AEXPR_NOT_BETWEEN_SYM)
+ {
+ Assert(list_length(aexpr->name) == 1);
+ *nodename = strVal(linitial(aexpr->name));
+ group = 4;
+ }
+ }
+ else if (IsA(node, NullTest) ||IsA(node, BooleanTest))
+ {
+ *nodename = "IS";
+ group = 1;
+ }
+ else if (IsA(node, XmlExpr))
+ {
+ XmlExpr *x = (XmlExpr *) node;
+
+ if (x->op == IS_DOCUMENT)
+ {
+ *nodename = "IS";
+ group = 1;
+ }
+ /* XXX what of IS NOT DOCUMENT? */
+ }
+
+ return group;
+ }
+
+ /*
+ * helper routine for delivering 9.4-to-9.5 operator precedence warnings
+ *
+ * opgroup/opname/location represent some parent node
+ * lchild, rchild are its left and right children (either could be NULL)
+ *
+ * This should be called before transforming the child nodes, since if a
+ * precedence-driven parsing change has occurred in a query that used to work,
+ * it's quite possible that we'll get a semantic failure while analyzing the
+ * child expression. We want to produce the warning before that happens.
+ * In any case, operator_precedence_group() expects untransformed input.
+ */
+ static void
+ emit_precedence_warnings(ParseState *pstate,
+ int opgroup, const char *opname,
+ Node *lchild, Node *rchild,
+ int location)
+ {
+ /*----------
+ * Map precedence groupings to old precedence ordering
+ *
+ * Old precedence order:
+ * 2: < > =
+ * 4: LIKE ILIKE SIMILAR BETWEEN IN
+ * 5: generic Op, inclding 3: <= => <>
+ * 1: IS tests (NullTest, BooleanTest, etc)
+ *----------
+ */
+ static const int oldprecedence[] = {0, 4, 1, 3, 2, 3};
+ int cgroup;
+ const char *copname;
+
+ Assert(opgroup > 0);
+
+ /*
+ * Complain if left child, which should be same or higher precedence
+ * according to current rules, used to be lower precedence.
+ */
+ cgroup = operator_precedence_group(lchild, &copname);
+ if (cgroup > 0)
+ {
+ Assert(opgroup <= cgroup);
+ if (oldprecedence[cgroup] < oldprecedence[opgroup])
+ ereport(WARNING,
+ (errmsg("operator precedence change: %s is now lower precedence than %s",
+ opname, copname),
+ parser_errposition(pstate, location)));
+ }
+
+ /*
+ * Complain if right child, which should be higher precedence according to
+ * current rules, used to be same or lower precedence.
+ */
+ cgroup = operator_precedence_group(rchild, &copname);
+ if (cgroup > 0)
+ {
+ Assert(opgroup < cgroup);
+ if (oldprecedence[cgroup] <= oldprecedence[opgroup])
+ ereport(WARNING,
+ (errmsg("operator precedence change: %s is now lower precedence than %s",
+ opname, copname),
+ parser_errposition(pstate, location)));
+ }
+ }
+
+ /*
* Produce a string identifying an expression by kind.
*
* Note: when practical, use a simple SQL keyword for the result. If that
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 3724330..2d85cf0 100644
*** a/src/backend/parser/parse_target.c
--- b/src/backend/parser/parse_target.c
*************** FigureColnameInternal(Node *node, char *
*** 1654,1665 ****
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
case T_A_Expr:
- /* make nullif() act like a regular function */
if (((A_Expr *) node)->kind == AEXPR_NULLIF)
{
*name = "nullif";
return 2;
}
break;
case T_TypeCast:
strength = FigureColnameInternal(((TypeCast *) node)->arg,
--- 1654,1670 ----
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
case T_A_Expr:
if (((A_Expr *) node)->kind == AEXPR_NULLIF)
{
+ /* make nullif() act like a regular function */
*name = "nullif";
return 2;
}
+ if (((A_Expr *) node)->kind == AEXPR_PAREN)
+ {
+ /* look through dummy parenthesis node */
+ return FigureColnameInternal(((A_Expr *) node)->lexpr, name);
+ }
break;
case T_TypeCast:
strength = FigureColnameInternal(((TypeCast *) node)->arg,
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index a78ce03..7ce7a47 100644
*** a/src/backend/parser/scan.l
--- b/src/backend/parser/scan.l
*************** ident_cont [A-Za-z\200-\377_0-9\$]
*** 331,339 ****
--- 331,344 ----
identifier {ident_start}{ident_cont}*
+ /* Assorted special-case operators and operator-like tokens */
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** other .
*** 808,813 ****
--- 813,840 ----
return COLON_EQUALS;
}
+ {less_equals} {
+ SET_YYLLOC();
+ return LESS_EQUALS;
+ }
+
+ {greater_equals} {
+ SET_YYLLOC();
+ return GREATER_EQUALS;
+ }
+
+ {less_greater} {
+ /* We accept both "<>" and "!=" as meaning NOT_EQUALS */
+ SET_YYLLOC();
+ return NOT_EQUALS;
+ }
+
+ {not_equals} {
+ /* We accept both "<>" and "!=" as meaning NOT_EQUALS */
+ SET_YYLLOC();
+ return NOT_EQUALS;
+ }
+
{self} {
SET_YYLLOC();
return yytext[0];
*************** other .
*** 885,895 ****
if (nchars >= NAMEDATALEN)
yyerror("operator too long");
! /* Convert "!=" operator to "<>" for compatibility */
! if (strcmp(yytext, "!=") == 0)
! yylval->str = pstrdup("<>");
! else
! yylval->str = pstrdup(yytext);
return Op;
}
--- 912,918 ----
if (nchars >= NAMEDATALEN)
yyerror("operator too long");
! yylval->str = pstrdup(yytext);
return Op;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9572777..360dad9 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_bool ConfigureNames
*** 1514,1519 ****
--- 1514,1529 ----
},
{
+ {"operator_precedence_warning", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
+ gettext_noop("Emit a warning for constructs that changed meaning since PostgreSQL 9.4."),
+ NULL,
+ },
+ &operator_precedence_warning,
+ false,
+ NULL, NULL, NULL
+ },
+
+ {
{"quote_all_identifiers", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
gettext_noop("When generating SQL fragments, quote all identifiers."),
NULL,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index b053659..9b2ca28 100644
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 582,587 ****
--- 582,588 ----
#default_with_oids = off
#escape_string_warning = on
#lo_compat_privileges = off
+ #operator_precedence_warning = off
#quote_all_identifiers = off
#sql_inheritance = on
#standard_conforming_strings = on
diff --git a/src/bin/psql/psqlscan.l b/src/bin/psql/psqlscan.l
index fb3fa11..a37cd2c 100644
*** a/src/bin/psql/psqlscan.l
--- b/src/bin/psql/psqlscan.l
*************** ident_cont [A-Za-z\200-\377_0-9\$]
*** 355,363 ****
--- 355,368 ----
identifier {ident_start}{ident_cont}*
+ /* Assorted special-case operators and operator-like tokens */
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** other .
*** 669,674 ****
--- 674,695 ----
ECHO;
}
+ {less_equals} {
+ ECHO;
+ }
+
+ {greater_equals} {
+ ECHO;
+ }
+
+ {less_greater} {
+ ECHO;
+ }
+
+ {not_equals} {
+ ECHO;
+ }
+
/*
* These rules are specific to psql --- they implement parenthesis
* counting and detection of command-ending semicolon. These must
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index d7b6148..1987313 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef enum A_Expr_Kind
*** 236,242 ****
AEXPR_BETWEEN, /* name must be "BETWEEN" */
AEXPR_NOT_BETWEEN, /* name must be "NOT BETWEEN" */
AEXPR_BETWEEN_SYM, /* name must be "BETWEEN SYMMETRIC" */
! AEXPR_NOT_BETWEEN_SYM /* name must be "NOT BETWEEN SYMMETRIC" */
} A_Expr_Kind;
typedef struct A_Expr
--- 236,243 ----
AEXPR_BETWEEN, /* name must be "BETWEEN" */
AEXPR_NOT_BETWEEN, /* name must be "NOT BETWEEN" */
AEXPR_BETWEEN_SYM, /* name must be "BETWEEN SYMMETRIC" */
! AEXPR_NOT_BETWEEN_SYM, /* name must be "NOT BETWEEN SYMMETRIC" */
! AEXPR_PAREN /* nameless dummy node for parentheses */
} A_Expr_Kind;
typedef struct A_Expr
diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h
index 66391df..fbc3f17 100644
*** a/src/include/parser/parse_expr.h
--- b/src/include/parser/parse_expr.h
***************
*** 16,21 ****
--- 16,22 ----
#include "parser/parse_node.h"
/* GUC parameters */
+ extern bool operator_precedence_warning;
extern bool Transform_null_equals;
extern Node *transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind);
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 506a313..761cfab 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*************** static void check_raise_parameters(PLp
*** 227,232 ****
--- 227,233 ----
%token <str> IDENT FCONST SCONST BCONST XCONST Op
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS
+ %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
/*
* Other tokens recognized by plpgsql's lexer interface layer (pl_scanner.c).
Here's a completed patch for this. This includes fixing the NOT LIKE
problem as discussed in the other thread.
I've done more-or-less-exhaustive testing on this to verify that it
produces warnings whenever necessary. It generates a few false-positive
warnings in corner cases that seem too complicated to model more precisely.
An example is that the production for "= ANY (sub-select)" clauses looks
like
a_expr subquery_Op sub_type select_with_parens %prec Op
but in point of fact its precedence against operators to its left is
not necessarily Op, but whatever actual operator appears --- for
example "= ANY" has the precedence of "=". This is because of the same
point noted in the other thread that Bison really implements that by
comparison to the lookahead token's precedence, not the rule's declared
precedence. The patch treats this as having precedence Op, which is
the highest possibility, so it will sometimes warn unnecessarily.
regards, tom lane
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 9261e7f..cd89819 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** dynamic_library_path = 'C:\tools\postgre
*** 6792,6797 ****
--- 6792,6820 ----
</listitem>
</varlistentry>
+ <varlistentry id="guc-operator-precedence-warning" xreflabel="operator_precedence_warning">
+ <term><varname>operator_precedence_warning</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>operator_precedence_warning</> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ When on, the parser will emit a warning for any construct that might
+ have changed meanings since <productname>PostgreSQL</> 9.4 as a result
+ of changes in operator precedence. This is useful for auditing
+ applications to see if precedence changes have broken anything; but it
+ is not meant to be kept turned on in production, since it will warn
+ about some perfectly valid, standard-compliant SQL code.
+ The default is <literal>off</>.
+ </para>
+
+ <para>
+ See <xref linkend="sql-precedence"> for more information.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-quote-all-identifiers" xreflabel="quote-all-identifiers">
<term><varname>quote_all_identifiers</varname> (<type>boolean</type>)
<indexterm>
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 4b81b08..e492684 100644
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*************** CAST ( '<replaceable>string</replaceable
*** 984,993 ****
associativity of the operators in <productname>PostgreSQL</>.
Most operators have the same precedence and are left-associative.
The precedence and associativity of the operators is hard-wired
! into the parser. This can lead to non-intuitive behavior; for
! example the Boolean operators <literal><</> and
! <literal>></> have a different precedence than the Boolean
! operators <literal><=</> and <literal>>=</>. Also, you will
sometimes need to add parentheses when using combinations of
binary and unary operators. For instance:
<programlisting>
--- 984,994 ----
associativity of the operators in <productname>PostgreSQL</>.
Most operators have the same precedence and are left-associative.
The precedence and associativity of the operators is hard-wired
! into the parser.
! </para>
!
! <para>
! You will
sometimes need to add parentheses when using combinations of
binary and unary operators. For instance:
<programlisting>
*************** SELECT (5 !) - 6;
*** 1008,1014 ****
</para>
<table id="sql-precedence-table">
! <title>Operator Precedence (decreasing)</title>
<tgroup cols="3">
<thead>
--- 1009,1015 ----
</para>
<table id="sql-precedence-table">
! <title>Operator Precedence (highest to lowest)</title>
<tgroup cols="3">
<thead>
*************** SELECT (5 !) - 6;
*** 1063,1125 ****
</row>
<row>
! <entry><token>IS</token></entry>
! <entry></entry>
! <entry><literal>IS TRUE</>, <literal>IS FALSE</>, <literal>IS NULL</>, etc</entry>
! </row>
!
! <row>
! <entry><token>ISNULL</token></entry>
! <entry></entry>
! <entry>test for null</entry>
! </row>
!
! <row>
! <entry><token>NOTNULL</token></entry>
! <entry></entry>
! <entry>test for not null</entry>
! </row>
!
! <row>
! <entry>(any other)</entry>
<entry>left</entry>
<entry>all other native and user-defined operators</entry>
</row>
<row>
- <entry><token>IN</token></entry>
- <entry></entry>
- <entry>set membership</entry>
- </row>
-
- <row>
- <entry><token>BETWEEN</token></entry>
- <entry></entry>
- <entry>range containment</entry>
- </row>
-
- <row>
<entry><token>OVERLAPS</token></entry>
<entry></entry>
<entry>time interval overlap</entry>
</row>
<row>
! <entry><token>LIKE</token> <token>ILIKE</token> <token>SIMILAR</token></entry>
<entry></entry>
! <entry>string pattern matching</entry>
</row>
<row>
! <entry><token><</token> <token>></token></entry>
<entry></entry>
! <entry>less than, greater than</entry>
</row>
<row>
! <entry><token>=</token></entry>
! <entry>right</entry>
! <entry>equality, assignment</entry>
</row>
<row>
--- 1064,1098 ----
</row>
<row>
! <entry>(any other operator)</entry>
<entry>left</entry>
<entry>all other native and user-defined operators</entry>
</row>
<row>
<entry><token>OVERLAPS</token></entry>
<entry></entry>
<entry>time interval overlap</entry>
</row>
<row>
! <entry><token>BETWEEN</token> <token>IN</token> <token>LIKE</token> <token>ILIKE</token>
<token>SIMILAR</token></entry>
<entry></entry>
! <entry>range containment, set membership, string matching</entry>
</row>
<row>
! <entry><token><</token> <token>></token> <token>=</token> <token><=</token> <token>>=</token>
<token><></token>
! </entry>
<entry></entry>
! <entry>comparison operators</entry>
</row>
<row>
! <entry><token>IS</token> <token>ISNULL</token> <token>NOTNULL</token></entry>
! <entry></entry>
! <entry><literal>IS TRUE</>, <literal>IS FALSE</>, <literal>IS
! NULL</>, <literal>IS DISTINCT FROM</>, etc</entry>
</row>
<row>
*************** SELECT (5 !) - 6;
*** 1159,1167 ****
SELECT 3 OPERATOR(pg_catalog.+) 4;
</programlisting>
the <literal>OPERATOR</> construct is taken to have the default precedence
! shown in <xref linkend="sql-precedence-table"> for <quote>any other</> operator. This is true no matter
which specific operator appears inside <literal>OPERATOR()</>.
</para>
</sect2>
</sect1>
--- 1132,1163 ----
SELECT 3 OPERATOR(pg_catalog.+) 4;
</programlisting>
the <literal>OPERATOR</> construct is taken to have the default precedence
! shown in <xref linkend="sql-precedence-table"> for
! <quote>any other operator</>. This is true no matter
which specific operator appears inside <literal>OPERATOR()</>.
</para>
+
+ <note>
+ <para>
+ <productname>PostgreSQL</> versions before 9.5 used slightly different
+ operator precedence rules. In particular, <token><=</token>
+ <token>>=</token> and <token><></token> used to be treated as
+ generic operators; <literal>IS</> tests used to have higher priority;
+ and <literal>NOT BETWEEN</> and related constructs acted inconsistently,
+ being taken in some cases as having the precedence of <literal>NOT</>
+ rather than <literal>BETWEEN</>. These rules were changed for better
+ compliance with the SQL standard and to reduce confusion from
+ inconsistent treatment of logically equivalent constructs. In most
+ cases, these changes will result in no behavioral change, or perhaps
+ in <quote>no such operator</> failures which can be resolved by adding
+ parentheses. However there are corner cases in which a query might
+ change behavior without any parsing error being reported. If you are
+ concerned about whether these changes have silently broken something,
+ you can test your application with the configuration
+ parameter <xref linkend="guc-operator-precedence-warning"> turned on
+ to see if any warnings are logged.
+ </para>
+ </note>
</sect2>
</sect1>
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 2f417fe..f5e9f48 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outAExpr(StringInfo str, const A_Expr *
*** 2544,2549 ****
--- 2544,2552 ----
appendStringInfoString(str, " NOT_BETWEEN_SYM ");
WRITE_NODE_FIELD(name);
break;
+ case AEXPR_PAREN:
+ appendStringInfoString(str, " PAREN");
+ break;
default:
appendStringInfoString(str, " ??");
break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 581f7a1..4e82ef5 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 58,63 ****
--- 58,64 ----
#include "nodes/nodeFuncs.h"
#include "parser/gramparse.h"
#include "parser/parser.h"
+ #include "parser/parse_expr.h"
#include "storage/lmgr.h"
#include "utils/date.h"
#include "utils/datetime.h"
*************** static Node *makeRecursiveViewSelect(cha
*** 532,537 ****
--- 533,539 ----
%token <str> IDENT FCONST SCONST BCONST XCONST Op
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS
+ %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
/*
* If you want to make any keyword changes, update the keyword table in
*************** static Node *makeRecursiveViewSelect(cha
*** 634,641 ****
* The grammar thinks these are keywords, but they are not in the kwlist.h
* list and so can never be entered directly. The filter in parser.c
* creates these tokens when required (based on looking one token ahead).
*/
! %token NULLS_LA WITH_LA
/* Precedence: lowest to highest */
--- 636,648 ----
* The grammar thinks these are keywords, but they are not in the kwlist.h
* list and so can never be entered directly. The filter in parser.c
* creates these tokens when required (based on looking one token ahead).
+ *
+ * NOT_LA exists so that productions such as NOT LIKE can be given the same
+ * precedence as LIKE; otherwise they'd effectively have the same precedence
+ * as NOT, at least with respect to their left-hand subexpression.
+ * NULLS_LA and WITH_LA are needed to make the grammar LALR(1).
*/
! %token NOT_LA NULLS_LA WITH_LA
/* Precedence: lowest to highest */
*************** static Node *makeRecursiveViewSelect(cha
*** 645,657 ****
%left OR
%left AND
%right NOT
! %right '='
! %nonassoc '<' '>'
! %nonassoc LIKE ILIKE SIMILAR
! %nonassoc ESCAPE
%nonassoc OVERLAPS
- %nonassoc BETWEEN
- %nonassoc IN_P
%left POSTFIXOP /* dummy for postfix Op rules */
/*
* To support target_el without AS, we must give IDENT an explicit priority
--- 652,662 ----
%left OR
%left AND
%right NOT
! %nonassoc IS ISNULL NOTNULL /* IS sets precedence for IS NULL, etc */
! %nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
! %nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
! %nonassoc ESCAPE /* ESCAPE must be just above LIKE/ILIKE/SIMILAR */
%nonassoc OVERLAPS
%left POSTFIXOP /* dummy for postfix Op rules */
/*
* To support target_el without AS, we must give IDENT an explicit priority
*************** static Node *makeRecursiveViewSelect(cha
*** 676,684 ****
%nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */
%nonassoc IDENT NULL_P PARTITION RANGE ROWS PRECEDING FOLLOWING
%left Op OPERATOR /* multi-character ops and user-defined operators */
- %nonassoc NOTNULL
- %nonassoc ISNULL
- %nonassoc IS /* sets precedence for IS NULL, etc */
%left '+' '-'
%left '*' '/' '%'
%left '^'
--- 681,686 ----
*************** interval_second:
*** 11170,11175 ****
--- 11172,11183 ----
*
* c_expr is all the productions that are common to a_expr and b_expr;
* it's factored out just to eliminate redundant coding.
+ *
+ * Be careful of productions involving more than one terminal token.
+ * By default, bison will assign such productions the precedence of their
+ * last terminal, but in nearly all cases you want it to be the precedence
+ * of the first terminal instead; otherwise you will not get the behavior
+ * you expect! So we use %prec annotations freely to set precedences.
*/
a_expr: c_expr { $$ = $1; }
| a_expr TYPECAST Typename
*************** a_expr: c_expr { $$ = $1; }
*** 11219,11224 ****
--- 11227,11238 ----
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">", $1, $3, @2); }
| a_expr '=' a_expr
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "=", $1, $3, @2); }
+ | a_expr LESS_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<=", $1, $3, @2); }
+ | a_expr GREATER_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">=", $1, $3, @2); }
+ | a_expr NOT_EQUALS a_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<>", $1, $3, @2); }
| a_expr qual_Op a_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
*************** a_expr: c_expr { $$ = $1; }
*** 11233,11245 ****
{ $$ = makeOrExpr($1, $3, @2); }
| NOT a_expr
{ $$ = makeNotExpr($2, @1); }
| a_expr LIKE a_expr
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_LIKE, "~~",
$1, $3, @2);
}
! | a_expr LIKE a_expr ESCAPE a_expr
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($3, $5),
--- 11247,11261 ----
{ $$ = makeOrExpr($1, $3, @2); }
| NOT a_expr
{ $$ = makeNotExpr($2, @1); }
+ | NOT_LA a_expr %prec NOT
+ { $$ = makeNotExpr($2, @1); }
| a_expr LIKE a_expr
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_LIKE, "~~",
$1, $3, @2);
}
! | a_expr LIKE a_expr ESCAPE a_expr %prec LIKE
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($3, $5),
*************** a_expr: c_expr { $$ = $1; }
*** 11247,11258 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_LIKE, "~~",
$1, (Node *) n, @2);
}
! | a_expr NOT LIKE a_expr
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_LIKE, "!~~",
$1, $4, @2);
}
! | a_expr NOT LIKE a_expr ESCAPE a_expr
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($4, $6),
--- 11263,11274 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_LIKE, "~~",
$1, (Node *) n, @2);
}
! | a_expr NOT_LA LIKE a_expr %prec NOT_LA
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_LIKE, "!~~",
$1, $4, @2);
}
! | a_expr NOT_LA LIKE a_expr ESCAPE a_expr %prec NOT_LA
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($4, $6),
*************** a_expr: c_expr { $$ = $1; }
*** 11265,11271 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_ILIKE, "~~*",
$1, $3, @2);
}
! | a_expr ILIKE a_expr ESCAPE a_expr
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($3, $5),
--- 11281,11287 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_ILIKE, "~~*",
$1, $3, @2);
}
! | a_expr ILIKE a_expr ESCAPE a_expr %prec ILIKE
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($3, $5),
*************** a_expr: c_expr { $$ = $1; }
*** 11273,11284 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_ILIKE, "~~*",
$1, (Node *) n, @2);
}
! | a_expr NOT ILIKE a_expr
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_ILIKE, "!~~*",
$1, $4, @2);
}
! | a_expr NOT ILIKE a_expr ESCAPE a_expr
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($4, $6),
--- 11289,11300 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_ILIKE, "~~*",
$1, (Node *) n, @2);
}
! | a_expr NOT_LA ILIKE a_expr %prec NOT_LA
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_ILIKE, "!~~*",
$1, $4, @2);
}
! | a_expr NOT_LA ILIKE a_expr ESCAPE a_expr %prec NOT_LA
{
FuncCall *n = makeFuncCall(SystemFuncName("like_escape"),
list_make2($4, $6),
*************** a_expr: c_expr { $$ = $1; }
*** 11287,11293 ****
$1, (Node *) n, @2);
}
! | a_expr SIMILAR TO a_expr %prec SIMILAR
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($4, makeNullAConst(-1)),
--- 11303,11309 ----
$1, (Node *) n, @2);
}
! | a_expr SIMILAR TO a_expr %prec SIMILAR
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($4, makeNullAConst(-1)),
*************** a_expr: c_expr { $$ = $1; }
*** 11295,11301 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_SIMILAR, "~",
$1, (Node *) n, @2);
}
! | a_expr SIMILAR TO a_expr ESCAPE a_expr
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($4, $6),
--- 11311,11317 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_SIMILAR, "~",
$1, (Node *) n, @2);
}
! | a_expr SIMILAR TO a_expr ESCAPE a_expr %prec SIMILAR
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($4, $6),
*************** a_expr: c_expr { $$ = $1; }
*** 11303,11309 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_SIMILAR, "~",
$1, (Node *) n, @2);
}
! | a_expr NOT SIMILAR TO a_expr %prec SIMILAR
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($5, makeNullAConst(-1)),
--- 11319,11325 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_SIMILAR, "~",
$1, (Node *) n, @2);
}
! | a_expr NOT_LA SIMILAR TO a_expr %prec NOT_LA
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($5, makeNullAConst(-1)),
*************** a_expr: c_expr { $$ = $1; }
*** 11311,11317 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_SIMILAR, "!~",
$1, (Node *) n, @2);
}
! | a_expr NOT SIMILAR TO a_expr ESCAPE a_expr
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($5, $7),
--- 11327,11333 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_SIMILAR, "!~",
$1, (Node *) n, @2);
}
! | a_expr NOT_LA SIMILAR TO a_expr ESCAPE a_expr %prec NOT_LA
{
FuncCall *n = makeFuncCall(SystemFuncName("similar_escape"),
list_make2($5, $7),
*************** a_expr: c_expr { $$ = $1; }
*** 11443,11449 ****
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_OF, "<>", $1, (Node *) $6, @2);
}
! | a_expr BETWEEN opt_asymmetric b_expr AND b_expr %prec BETWEEN
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_BETWEEN,
"BETWEEN",
--- 11459,11465 ----
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_OF, "<>", $1, (Node *) $6, @2);
}
! | a_expr BETWEEN opt_asymmetric b_expr AND a_expr %prec BETWEEN
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_BETWEEN,
"BETWEEN",
*************** a_expr: c_expr { $$ = $1; }
*** 11451,11457 ****
(Node *) list_make2($4, $6),
@2);
}
! | a_expr NOT BETWEEN opt_asymmetric b_expr AND b_expr %prec BETWEEN
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_NOT_BETWEEN,
"NOT BETWEEN",
--- 11467,11473 ----
(Node *) list_make2($4, $6),
@2);
}
! | a_expr NOT_LA BETWEEN opt_asymmetric b_expr AND a_expr %prec NOT_LA
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_NOT_BETWEEN,
"NOT BETWEEN",
*************** a_expr: c_expr { $$ = $1; }
*** 11459,11465 ****
(Node *) list_make2($5, $7),
@2);
}
! | a_expr BETWEEN SYMMETRIC b_expr AND b_expr %prec BETWEEN
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_BETWEEN_SYM,
"BETWEEN SYMMETRIC",
--- 11475,11481 ----
(Node *) list_make2($5, $7),
@2);
}
! | a_expr BETWEEN SYMMETRIC b_expr AND a_expr %prec BETWEEN
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_BETWEEN_SYM,
"BETWEEN SYMMETRIC",
*************** a_expr: c_expr { $$ = $1; }
*** 11467,11473 ****
(Node *) list_make2($4, $6),
@2);
}
! | a_expr NOT BETWEEN SYMMETRIC b_expr AND b_expr %prec BETWEEN
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_NOT_BETWEEN_SYM,
"NOT BETWEEN SYMMETRIC",
--- 11483,11489 ----
(Node *) list_make2($4, $6),
@2);
}
! | a_expr NOT_LA BETWEEN SYMMETRIC b_expr AND a_expr %prec NOT_LA
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_NOT_BETWEEN_SYM,
"NOT BETWEEN SYMMETRIC",
*************** a_expr: c_expr { $$ = $1; }
*** 11495,11501 ****
$$ = (Node *) makeSimpleA_Expr(AEXPR_IN, "=", $1, $3, @2);
}
}
! | a_expr NOT IN_P in_expr
{
/* in_expr returns a SubLink or a list of a_exprs */
if (IsA($4, SubLink))
--- 11511,11517 ----
$$ = (Node *) makeSimpleA_Expr(AEXPR_IN, "=", $1, $3, @2);
}
}
! | a_expr NOT_LA IN_P in_expr %prec NOT_LA
{
/* in_expr returns a SubLink or a list of a_exprs */
if (IsA($4, SubLink))
*************** b_expr: c_expr
*** 11599,11604 ****
--- 11615,11626 ----
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">", $1, $3, @2); }
| b_expr '=' b_expr
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "=", $1, $3, @2); }
+ | b_expr LESS_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<=", $1, $3, @2); }
+ | b_expr GREATER_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ">=", $1, $3, @2); }
+ | b_expr NOT_EQUALS b_expr
+ { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "<>", $1, $3, @2); }
| b_expr qual_Op b_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
| qual_Op b_expr %prec Op
*************** c_expr: columnref { $$ = $1; }
*** 11670,11675 ****
--- 11692,11715 ----
n->indirection = check_indirection($4, yyscanner);
$$ = (Node *)n;
}
+ else if (operator_precedence_warning)
+ {
+ /*
+ * If precedence warnings are enabled, insert
+ * AEXPR_PAREN nodes wrapping all explicitly
+ * parenthesized subexpressions; this prevents bogus
+ * warnings from being issued when the ordering has
+ * been forced by parentheses.
+ *
+ * In principle we should not be relying on a GUC to
+ * decide whether to insert AEXPR_PAREN nodes.
+ * However, since they have no effect except to
+ * suppress warnings, it's probably safe enough; and
+ * we'd just as soon not waste cycles on dummy parse
+ * nodes if we don't have to.
+ */
+ $$ = (Node *) makeA_Expr(AEXPR_PAREN, NIL, $2, NULL, @1);
+ }
else
$$ = $2;
}
*************** MathOp: '+' { $$ = "+"; }
*** 12518,12523 ****
--- 12558,12566 ----
| '<' { $$ = "<"; }
| '>' { $$ = ">"; }
| '=' { $$ = "="; }
+ | LESS_EQUALS { $$ = "<="; }
+ | GREATER_EQUALS { $$ = ">="; }
+ | NOT_EQUALS { $$ = "<>"; }
;
qual_Op: Op
*************** subquery_Op:
*** 12540,12550 ****
{ $$ = $3; }
| LIKE
{ $$ = list_make1(makeString("~~")); }
! | NOT LIKE
{ $$ = list_make1(makeString("!~~")); }
| ILIKE
{ $$ = list_make1(makeString("~~*")); }
! | NOT ILIKE
{ $$ = list_make1(makeString("!~~*")); }
/* cannot put SIMILAR TO here, because SIMILAR TO is a hack.
* the regular expression is preprocessed by a function (similar_escape),
--- 12583,12593 ----
{ $$ = $3; }
| LIKE
{ $$ = list_make1(makeString("~~")); }
! | NOT_LA LIKE
{ $$ = list_make1(makeString("!~~")); }
| ILIKE
{ $$ = list_make1(makeString("~~*")); }
! | NOT_LA ILIKE
{ $$ = list_make1(makeString("!~~*")); }
/* cannot put SIMILAR TO here, because SIMILAR TO is a hack.
* the regular expression is preprocessed by a function (similar_escape),
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 7829bcb..cf09d65 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
***************
*** 37,44 ****
--- 37,91 ----
#include "utils/xml.h"
+ /* GUC parameters */
+ bool operator_precedence_warning = false;
bool Transform_null_equals = false;
+ /*
+ * Node-type groups for operator precedence warnings
+ * We use zero for everything not otherwise classified
+ */
+ #define PREC_GROUP_POSTFIX_IS 1 /* postfix IS tests (NullTest, etc) */
+ #define PREC_GROUP_INFIX_IS 2 /* infix IS (IS DISTINCT FROM, etc) */
+ #define PREC_GROUP_LESS 3 /* < > */
+ #define PREC_GROUP_EQUAL 4 /* = */
+ #define PREC_GROUP_LESS_EQUAL 5 /* <= >= <> */
+ #define PREC_GROUP_LIKE 6 /* LIKE ILIKE SIMILAR */
+ #define PREC_GROUP_BETWEEN 7 /* BETWEEN */
+ #define PREC_GROUP_IN 8 /* IN */
+ #define PREC_GROUP_NOT_LIKE 9 /* NOT LIKE/ILIKE/SIMILAR */
+ #define PREC_GROUP_NOT_BETWEEN 10 /* NOT BETWEEN */
+ #define PREC_GROUP_NOT_IN 11 /* NOT IN */
+ #define PREC_GROUP_POSTFIX_OP 12 /* generic postfix operators */
+ #define PREC_GROUP_INFIX_OP 13 /* generic infix operators */
+ #define PREC_GROUP_PREFIX_OP 14 /* generic prefix operators */
+
+ /*
+ * Map precedence groupings to old precedence ordering
+ *
+ * Old precedence order:
+ * 1. NOT
+ * 2. =
+ * 3. < >
+ * 4. LIKE ILIKE SIMILAR
+ * 5. BETWEEN
+ * 6. IN
+ * 7. generic postfix Op
+ * 8. generic Op, including <= => <>
+ * 9. generic prefix Op
+ * 10. IS tests (NullTest, BooleanTest, etc)
+ *
+ * NOT BETWEEN etc map to BETWEEN etc when considered as being on the left,
+ * but to NOT when considered as being on the right, because of the buggy
+ * precedence handling of those productions in the old grammar.
+ */
+ static const int oldprecedence_l[] = {
+ 0, 10, 10, 3, 2, 8, 4, 5, 6, 4, 5, 6, 7, 8, 9
+ };
+ static const int oldprecedence_r[] = {
+ 0, 10, 10, 3, 2, 8, 4, 5, 6, 1, 1, 1, 7, 8, 9
+ };
+
static Node *transformExprRecurse(ParseState *pstate, Node *expr);
static Node *transformParamRef(ParseState *pstate, ParamRef *pref);
static Node *transformAExprOp(ParseState *pstate, A_Expr *a);
*************** static Node *make_row_distinct_op(ParseS
*** 76,81 ****
--- 123,133 ----
RowExpr *lrow, RowExpr *rrow, int location);
static Expr *make_distinct_op(ParseState *pstate, List *opname,
Node *ltree, Node *rtree, int location);
+ static int operator_precedence_group(Node *node, const char **nodename);
+ static void emit_precedence_warnings(ParseState *pstate,
+ int opgroup, const char *opname,
+ Node *lchild, Node *rchild,
+ int location);
/*
*************** transformExprRecurse(ParseState *pstate,
*** 194,199 ****
--- 246,254 ----
case AEXPR_NOT_BETWEEN_SYM:
result = transformAExprBetween(pstate, a);
break;
+ case AEXPR_PAREN:
+ result = transformExprRecurse(pstate, a->lexpr);
+ break;
default:
elog(ERROR, "unrecognized A_Expr kind: %d", a->kind);
result = NULL; /* keep compiler quiet */
*************** transformExprRecurse(ParseState *pstate,
*** 255,260 ****
--- 310,320 ----
{
NullTest *n = (NullTest *) expr;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_IS, "IS",
+ (Node *) n->arg, NULL,
+ n->location);
+
n->arg = (Expr *) transformExprRecurse(pstate, (Node *) n->arg);
/* the argument can be any type, so don't coerce it */
n->argisrow = type_is_rowtype(exprType((Node *) n->arg));
*************** transformAExprOp(ParseState *pstate, A_E
*** 779,784 ****
--- 839,856 ----
Node *rexpr = a->rexpr;
Node *result;
+ if (operator_precedence_warning)
+ {
+ int opgroup;
+ const char *opname;
+
+ opgroup = operator_precedence_group((Node *) a, &opname);
+ if (opgroup > 0)
+ emit_precedence_warnings(pstate, opgroup, opname,
+ lexpr, rexpr,
+ a->location);
+ }
+
/*
* Special-case "foo = NULL" and "NULL = foo" for compatibility with
* standards-broken products (like Microsoft's). Turn these into IS NULL
*************** transformAExprOp(ParseState *pstate, A_E
*** 855,862 ****
static Node *
transformAExprOpAny(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = transformExprRecurse(pstate, a->lexpr);
! Node *rexpr = transformExprRecurse(pstate, a->rexpr);
return (Node *) make_scalar_array_op(pstate,
a->name,
--- 927,943 ----
static Node *
transformAExprOpAny(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = a->lexpr;
! Node *rexpr = a->rexpr;
!
! if (operator_precedence_warning)
! emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_OP,
! strVal(llast(a->name)),
! lexpr, NULL,
! a->location);
!
! lexpr = transformExprRecurse(pstate, lexpr);
! rexpr = transformExprRecurse(pstate, rexpr);
return (Node *) make_scalar_array_op(pstate,
a->name,
*************** transformAExprOpAny(ParseState *pstate,
*** 869,876 ****
static Node *
transformAExprOpAll(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = transformExprRecurse(pstate, a->lexpr);
! Node *rexpr = transformExprRecurse(pstate, a->rexpr);
return (Node *) make_scalar_array_op(pstate,
a->name,
--- 950,966 ----
static Node *
transformAExprOpAll(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = a->lexpr;
! Node *rexpr = a->rexpr;
!
! if (operator_precedence_warning)
! emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_OP,
! strVal(llast(a->name)),
! lexpr, NULL,
! a->location);
!
! lexpr = transformExprRecurse(pstate, lexpr);
! rexpr = transformExprRecurse(pstate, rexpr);
return (Node *) make_scalar_array_op(pstate,
a->name,
*************** transformAExprOpAll(ParseState *pstate,
*** 883,890 ****
static Node *
transformAExprDistinct(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = transformExprRecurse(pstate, a->lexpr);
! Node *rexpr = transformExprRecurse(pstate, a->rexpr);
if (lexpr && IsA(lexpr, RowExpr) &&
rexpr && IsA(rexpr, RowExpr))
--- 973,988 ----
static Node *
transformAExprDistinct(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = a->lexpr;
! Node *rexpr = a->rexpr;
!
! if (operator_precedence_warning)
! emit_precedence_warnings(pstate, PREC_GROUP_INFIX_IS, "IS",
! lexpr, rexpr,
! a->location);
!
! lexpr = transformExprRecurse(pstate, lexpr);
! rexpr = transformExprRecurse(pstate, rexpr);
if (lexpr && IsA(lexpr, RowExpr) &&
rexpr && IsA(rexpr, RowExpr))
*************** transformAExprNullIf(ParseState *pstate,
*** 941,960 ****
return (Node *) result;
}
static Node *
transformAExprOf(ParseState *pstate, A_Expr *a)
{
! /*
! * Checking an expression for match to a list of type names. Will result
! * in a boolean constant node.
! */
! Node *lexpr = transformExprRecurse(pstate, a->lexpr);
Const *result;
ListCell *telem;
Oid ltype,
rtype;
bool matched = false;
ltype = exprType(lexpr);
foreach(telem, (List *) a->rexpr)
{
--- 1039,1065 ----
return (Node *) result;
}
+ /*
+ * Checking an expression for match to a list of type names. Will result
+ * in a boolean constant node.
+ */
static Node *
transformAExprOf(ParseState *pstate, A_Expr *a)
{
! Node *lexpr = a->lexpr;
Const *result;
ListCell *telem;
Oid ltype,
rtype;
bool matched = false;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_IS, "IS",
+ lexpr, NULL,
+ a->location);
+
+ lexpr = transformExprRecurse(pstate, lexpr);
+
ltype = exprType(lexpr);
foreach(telem, (List *) a->rexpr)
{
*************** transformAExprIn(ParseState *pstate, A_E
*** 998,1003 ****
--- 1103,1115 ----
else
useOr = true;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate,
+ useOr ? PREC_GROUP_IN : PREC_GROUP_NOT_IN,
+ "IN",
+ a->lexpr, NULL,
+ a->location);
+
/*
* We try to generate a ScalarArrayOpExpr from IN/NOT IN, but this is only
* possible if there is a suitable array type available. If not, we fall
*************** transformAExprBetween(ParseState *pstate
*** 1150,1155 ****
--- 1262,1283 ----
bexpr = (Node *) linitial(args);
cexpr = (Node *) lsecond(args);
+ if (operator_precedence_warning)
+ {
+ int opgroup;
+ const char *opname;
+
+ opgroup = operator_precedence_group((Node *) a, &opname);
+ emit_precedence_warnings(pstate, opgroup, opname,
+ aexpr, cexpr,
+ a->location);
+ /* We can ignore bexpr thanks to syntactic restrictions */
+ /* Wrap subexpressions to prevent extra warnings */
+ aexpr = (Node *) makeA_Expr(AEXPR_PAREN, NIL, aexpr, NULL, -1);
+ bexpr = (Node *) makeA_Expr(AEXPR_PAREN, NIL, bexpr, NULL, -1);
+ cexpr = (Node *) makeA_Expr(AEXPR_PAREN, NIL, cexpr, NULL, -1);
+ }
+
/*
* Build the equivalent comparison expression. Make copies of
* multiply-referenced subexpressions for safety. (XXX this is really
*************** transformSubLink(ParseState *pstate, Sub
*** 1654,1659 ****
--- 1782,1800 ----
List *right_list;
ListCell *l;
+ if (operator_precedence_warning)
+ {
+ if (sublink->operName == NIL)
+ emit_precedence_warnings(pstate, PREC_GROUP_IN, "IN",
+ sublink->testexpr, NULL,
+ sublink->location);
+ else
+ emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_OP,
+ strVal(llast(sublink->operName)),
+ sublink->testexpr, NULL,
+ sublink->location);
+ }
+
/*
* If the source was "x IN (select)", convert to "x = ANY (select)".
*/
*************** transformXmlExpr(ParseState *pstate, Xml
*** 1997,2002 ****
--- 2138,2148 ----
ListCell *lc;
int i;
+ if (operator_precedence_warning && x->op == IS_DOCUMENT)
+ emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_IS, "IS",
+ (Node *) linitial(x->args), NULL,
+ x->location);
+
newx = makeNode(XmlExpr);
newx->op = x->op;
if (x->name)
*************** transformBooleanTest(ParseState *pstate,
*** 2169,2174 ****
--- 2315,2325 ----
{
const char *clausename;
+ if (operator_precedence_warning)
+ emit_precedence_warnings(pstate, PREC_GROUP_POSTFIX_IS, "IS",
+ (Node *) b->arg, NULL,
+ b->location);
+
switch (b->booltesttype)
{
case IS_TRUE:
*************** make_distinct_op(ParseState *pstate, Lis
*** 2686,2691 ****
--- 2837,3145 ----
}
/*
+ * Identify node's group for operator precedence warnings
+ *
+ * For items in nonzero groups, also return a suitable node name into *nodename
+ *
+ * Note: group zero is used for nodes that are higher or lower precedence
+ * than everything that changed precedence; we need never issue warnings
+ * related to such nodes.
+ */
+ static int
+ operator_precedence_group(Node *node, const char **nodename)
+ {
+ int group = 0;
+
+ *nodename = NULL;
+ if (node == NULL)
+ return 0;
+
+ if (IsA(node, A_Expr))
+ {
+ A_Expr *aexpr = (A_Expr *) node;
+
+ if (aexpr->kind == AEXPR_OP &&
+ aexpr->lexpr != NULL &&
+ aexpr->rexpr != NULL)
+ {
+ /* binary operator */
+ if (list_length(aexpr->name) == 1)
+ {
+ *nodename = strVal(linitial(aexpr->name));
+ /* Ignore if op was always higher priority than IS-tests */
+ if (strcmp(*nodename, "+") == 0 ||
+ strcmp(*nodename, "-") == 0 ||
+ strcmp(*nodename, "*") == 0 ||
+ strcmp(*nodename, "/") == 0 ||
+ strcmp(*nodename, "%") == 0 ||
+ strcmp(*nodename, "^") == 0)
+ group = 0;
+ else if (strcmp(*nodename, "<") == 0 ||
+ strcmp(*nodename, ">") == 0)
+ group = PREC_GROUP_LESS;
+ else if (strcmp(*nodename, "=") == 0)
+ group = PREC_GROUP_EQUAL;
+ else if (strcmp(*nodename, "<=") == 0 ||
+ strcmp(*nodename, ">=") == 0 ||
+ strcmp(*nodename, "<>") == 0)
+ group = PREC_GROUP_LESS_EQUAL;
+ else
+ group = PREC_GROUP_INFIX_OP;
+ }
+ else
+ {
+ /* schema-qualified operator syntax */
+ *nodename = "OPERATOR()";
+ group = PREC_GROUP_INFIX_OP;
+ }
+ }
+ else if (aexpr->kind == AEXPR_OP &&
+ aexpr->lexpr == NULL &&
+ aexpr->rexpr != NULL)
+ {
+ /* prefix operator */
+ if (list_length(aexpr->name) == 1)
+ {
+ *nodename = strVal(linitial(aexpr->name));
+ /* Ignore if op was always higher priority than IS-tests */
+ if (strcmp(*nodename, "+") == 0 ||
+ strcmp(*nodename, "-"))
+ group = 0;
+ else
+ group = PREC_GROUP_PREFIX_OP;
+ }
+ else
+ {
+ /* schema-qualified operator syntax */
+ *nodename = "OPERATOR()";
+ group = PREC_GROUP_PREFIX_OP;
+ }
+ }
+ else if (aexpr->kind == AEXPR_OP &&
+ aexpr->lexpr != NULL &&
+ aexpr->rexpr == NULL)
+ {
+ /* postfix operator */
+ if (list_length(aexpr->name) == 1)
+ {
+ *nodename = strVal(linitial(aexpr->name));
+ group = PREC_GROUP_POSTFIX_OP;
+ }
+ else
+ {
+ /* schema-qualified operator syntax */
+ *nodename = "OPERATOR()";
+ group = PREC_GROUP_POSTFIX_OP;
+ }
+ }
+ else if (aexpr->kind == AEXPR_OP_ANY ||
+ aexpr->kind == AEXPR_OP_ALL)
+ {
+ *nodename = strVal(llast(aexpr->name));
+ group = PREC_GROUP_POSTFIX_OP;
+ }
+ else if (aexpr->kind == AEXPR_DISTINCT)
+ {
+ *nodename = "IS";
+ group = PREC_GROUP_INFIX_IS;
+ }
+ else if (aexpr->kind == AEXPR_OF)
+ {
+ *nodename = "IS";
+ group = PREC_GROUP_POSTFIX_IS;
+ }
+ else if (aexpr->kind == AEXPR_IN)
+ {
+ *nodename = "IN";
+ if (strcmp(strVal(linitial(aexpr->name)), "=") == 0)
+ group = PREC_GROUP_IN;
+ else
+ group = PREC_GROUP_NOT_IN;
+ }
+ else if (aexpr->kind == AEXPR_LIKE)
+ {
+ *nodename = "LIKE";
+ if (strcmp(strVal(linitial(aexpr->name)), "~~") == 0)
+ group = PREC_GROUP_LIKE;
+ else
+ group = PREC_GROUP_NOT_LIKE;
+ }
+ else if (aexpr->kind == AEXPR_ILIKE)
+ {
+ *nodename = "ILIKE";
+ if (strcmp(strVal(linitial(aexpr->name)), "~~*") == 0)
+ group = PREC_GROUP_LIKE;
+ else
+ group = PREC_GROUP_NOT_LIKE;
+ }
+ else if (aexpr->kind == AEXPR_SIMILAR)
+ {
+ *nodename = "SIMILAR";
+ if (strcmp(strVal(linitial(aexpr->name)), "~") == 0)
+ group = PREC_GROUP_LIKE;
+ else
+ group = PREC_GROUP_NOT_LIKE;
+ }
+ else if (aexpr->kind == AEXPR_BETWEEN ||
+ aexpr->kind == AEXPR_BETWEEN_SYM)
+ {
+ Assert(list_length(aexpr->name) == 1);
+ *nodename = strVal(linitial(aexpr->name));
+ group = PREC_GROUP_BETWEEN;
+ }
+ else if (aexpr->kind == AEXPR_NOT_BETWEEN ||
+ aexpr->kind == AEXPR_NOT_BETWEEN_SYM)
+ {
+ Assert(list_length(aexpr->name) == 1);
+ *nodename = strVal(linitial(aexpr->name));
+ group = PREC_GROUP_NOT_BETWEEN;
+ }
+ }
+ else if (IsA(node, NullTest) ||
+ IsA(node, BooleanTest))
+ {
+ *nodename = "IS";
+ group = PREC_GROUP_POSTFIX_IS;
+ }
+ else if (IsA(node, XmlExpr))
+ {
+ XmlExpr *x = (XmlExpr *) node;
+
+ if (x->op == IS_DOCUMENT)
+ {
+ *nodename = "IS";
+ group = PREC_GROUP_POSTFIX_IS;
+ }
+ }
+ else if (IsA(node, SubLink))
+ {
+ SubLink *s = (SubLink *) node;
+
+ if (s->subLinkType == ANY_SUBLINK ||
+ s->subLinkType == ALL_SUBLINK)
+ {
+ if (s->operName == NIL)
+ {
+ *nodename = "IN";
+ group = PREC_GROUP_IN;
+ }
+ else
+ {
+ *nodename = strVal(llast(s->operName));
+ group = PREC_GROUP_POSTFIX_OP;
+ }
+ }
+ }
+ else if (IsA(node, BoolExpr))
+ {
+ /*
+ * Must dig into NOTs to see if it's IS NOT DOCUMENT or NOT IN. This
+ * opens us to possibly misrecognizing, eg, NOT (x IS DOCUMENT) as a
+ * problematic construct. We can tell the difference by checking
+ * whether the parse locations of the two nodes are identical.
+ *
+ * Note that when we are comparing the child node to its own children,
+ * we will not know that it was a NOT. Fortunately, that doesn't
+ * matter for these cases.
+ */
+ BoolExpr *b = (BoolExpr *) node;
+
+ if (b->boolop == NOT_EXPR)
+ {
+ Node *child = (Node *) linitial(b->args);
+
+ if (IsA(child, XmlExpr))
+ {
+ XmlExpr *x = (XmlExpr *) child;
+
+ if (x->op == IS_DOCUMENT &&
+ x->location == b->location)
+ {
+ *nodename = "IS";
+ group = PREC_GROUP_POSTFIX_IS;
+ }
+ }
+ else if (IsA(child, SubLink))
+ {
+ SubLink *s = (SubLink *) child;
+
+ if (s->subLinkType == ANY_SUBLINK && s->operName == NIL &&
+ s->location == b->location)
+ {
+ *nodename = "IN";
+ group = PREC_GROUP_NOT_IN;
+ }
+ }
+ }
+ }
+ return group;
+ }
+
+ /*
+ * helper routine for delivering 9.4-to-9.5 operator precedence warnings
+ *
+ * opgroup/opname/location represent some parent node
+ * lchild, rchild are its left and right children (either could be NULL)
+ *
+ * This should be called before transforming the child nodes, since if a
+ * precedence-driven parsing change has occurred in a query that used to work,
+ * it's quite possible that we'll get a semantic failure while analyzing the
+ * child expression. We want to produce the warning before that happens.
+ * In any case, operator_precedence_group() expects untransformed input.
+ */
+ static void
+ emit_precedence_warnings(ParseState *pstate,
+ int opgroup, const char *opname,
+ Node *lchild, Node *rchild,
+ int location)
+ {
+ int cgroup;
+ const char *copname;
+
+ Assert(opgroup > 0);
+
+ /*
+ * Complain if left child, which should be same or higher precedence
+ * according to current rules, used to be lower precedence.
+ *
+ * Exception to precedence rules: if left child is IN or NOT IN or a
+ * postfix operator, the grouping is syntactically forced regardless of
+ * precedence.
+ */
+ cgroup = operator_precedence_group(lchild, &copname);
+ if (cgroup > 0)
+ {
+ if (oldprecedence_l[cgroup] < oldprecedence_r[opgroup] &&
+ cgroup != PREC_GROUP_IN &&
+ cgroup != PREC_GROUP_NOT_IN &&
+ cgroup != PREC_GROUP_POSTFIX_OP &&
+ cgroup != PREC_GROUP_POSTFIX_IS)
+ ereport(WARNING,
+ (errmsg("operator precedence change: %s is now lower precedence than %s",
+ opname, copname),
+ parser_errposition(pstate, location)));
+ }
+
+ /*
+ * Complain if right child, which should be higher precedence according to
+ * current rules, used to be same or lower precedence.
+ *
+ * Exception to precedence rules: if right child is a prefix operator, the
+ * grouping is syntactically forced regardless of precedence.
+ */
+ cgroup = operator_precedence_group(rchild, &copname);
+ if (cgroup > 0)
+ {
+ if (oldprecedence_r[cgroup] <= oldprecedence_l[opgroup] &&
+ cgroup != PREC_GROUP_PREFIX_OP)
+ ereport(WARNING,
+ (errmsg("operator precedence change: %s is now lower precedence than %s",
+ opname, copname),
+ parser_errposition(pstate, location)));
+ }
+ }
+
+ /*
* Produce a string identifying an expression by kind.
*
* Note: when practical, use a simple SQL keyword for the result. If that
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 3724330..2d85cf0 100644
*** a/src/backend/parser/parse_target.c
--- b/src/backend/parser/parse_target.c
*************** FigureColnameInternal(Node *node, char *
*** 1654,1665 ****
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
case T_A_Expr:
- /* make nullif() act like a regular function */
if (((A_Expr *) node)->kind == AEXPR_NULLIF)
{
*name = "nullif";
return 2;
}
break;
case T_TypeCast:
strength = FigureColnameInternal(((TypeCast *) node)->arg,
--- 1654,1670 ----
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
case T_A_Expr:
if (((A_Expr *) node)->kind == AEXPR_NULLIF)
{
+ /* make nullif() act like a regular function */
*name = "nullif";
return 2;
}
+ if (((A_Expr *) node)->kind == AEXPR_PAREN)
+ {
+ /* look through dummy parenthesis node */
+ return FigureColnameInternal(((A_Expr *) node)->lexpr, name);
+ }
break;
case T_TypeCast:
strength = FigureColnameInternal(((TypeCast *) node)->arg,
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index b17771d..fdf5a6a 100644
*** a/src/backend/parser/parser.c
--- b/src/backend/parser/parser.c
*************** base_yylex(YYSTYPE *lvalp, YYLTYPE *lloc
*** 107,112 ****
--- 107,115 ----
*/
switch (cur_token)
{
+ case NOT:
+ cur_token_length = 3;
+ break;
case NULLS_P:
cur_token_length = 5;
break;
*************** base_yylex(YYSTYPE *lvalp, YYLTYPE *lloc
*** 151,156 ****
--- 154,173 ----
/* Replace cur_token if needed, based on lookahead */
switch (cur_token)
{
+ case NOT:
+ /* Replace NOT by NOT_LA if it's followed by BETWEEN, IN, etc */
+ switch (next_token)
+ {
+ case BETWEEN:
+ case IN_P:
+ case LIKE:
+ case ILIKE:
+ case SIMILAR:
+ cur_token = NOT_LA;
+ break;
+ }
+ break;
+
case NULLS_P:
/* Replace NULLS_P by NULLS_LA if it's followed by FIRST or LAST */
switch (next_token)
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index a78ce03..7ce7a47 100644
*** a/src/backend/parser/scan.l
--- b/src/backend/parser/scan.l
*************** ident_cont [A-Za-z\200-\377_0-9\$]
*** 331,339 ****
--- 331,344 ----
identifier {ident_start}{ident_cont}*
+ /* Assorted special-case operators and operator-like tokens */
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** other .
*** 808,813 ****
--- 813,840 ----
return COLON_EQUALS;
}
+ {less_equals} {
+ SET_YYLLOC();
+ return LESS_EQUALS;
+ }
+
+ {greater_equals} {
+ SET_YYLLOC();
+ return GREATER_EQUALS;
+ }
+
+ {less_greater} {
+ /* We accept both "<>" and "!=" as meaning NOT_EQUALS */
+ SET_YYLLOC();
+ return NOT_EQUALS;
+ }
+
+ {not_equals} {
+ /* We accept both "<>" and "!=" as meaning NOT_EQUALS */
+ SET_YYLLOC();
+ return NOT_EQUALS;
+ }
+
{self} {
SET_YYLLOC();
return yytext[0];
*************** other .
*** 885,895 ****
if (nchars >= NAMEDATALEN)
yyerror("operator too long");
! /* Convert "!=" operator to "<>" for compatibility */
! if (strcmp(yytext, "!=") == 0)
! yylval->str = pstrdup("<>");
! else
! yylval->str = pstrdup(yytext);
return Op;
}
--- 912,918 ----
if (nchars >= NAMEDATALEN)
yyerror("operator too long");
! yylval->str = pstrdup(yytext);
return Op;
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d84dba7..feaa0c4 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_bool ConfigureNames
*** 1590,1595 ****
--- 1590,1605 ----
},
{
+ {"operator_precedence_warning", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
+ gettext_noop("Emit a warning for constructs that changed meaning since PostgreSQL 9.4."),
+ NULL,
+ },
+ &operator_precedence_warning,
+ false,
+ NULL, NULL, NULL
+ },
+
+ {
{"quote_all_identifiers", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
gettext_noop("When generating SQL fragments, quote all identifiers."),
NULL,
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index f8f9ce1..26f3a53 100644
*** a/src/backend/utils/misc/postgresql.conf.sample
--- b/src/backend/utils/misc/postgresql.conf.sample
***************
*** 585,590 ****
--- 585,591 ----
#default_with_oids = off
#escape_string_warning = on
#lo_compat_privileges = off
+ #operator_precedence_warning = off
#quote_all_identifiers = off
#sql_inheritance = on
#standard_conforming_strings = on
diff --git a/src/bin/psql/psqlscan.l b/src/bin/psql/psqlscan.l
index fb3fa11..a37cd2c 100644
*** a/src/bin/psql/psqlscan.l
--- b/src/bin/psql/psqlscan.l
*************** ident_cont [A-Za-z\200-\377_0-9\$]
*** 355,363 ****
--- 355,368 ----
identifier {ident_start}{ident_cont}*
+ /* Assorted special-case operators and operator-like tokens */
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** other .
*** 669,674 ****
--- 674,695 ----
ECHO;
}
+ {less_equals} {
+ ECHO;
+ }
+
+ {greater_equals} {
+ ECHO;
+ }
+
+ {less_greater} {
+ ECHO;
+ }
+
+ {not_equals} {
+ ECHO;
+ }
+
/*
* These rules are specific to psql --- they implement parenthesis
* counting and detection of command-ending semicolon. These must
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac13302..8cda8d6 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
*************** typedef enum A_Expr_Kind
*** 239,245 ****
AEXPR_BETWEEN, /* name must be "BETWEEN" */
AEXPR_NOT_BETWEEN, /* name must be "NOT BETWEEN" */
AEXPR_BETWEEN_SYM, /* name must be "BETWEEN SYMMETRIC" */
! AEXPR_NOT_BETWEEN_SYM /* name must be "NOT BETWEEN SYMMETRIC" */
} A_Expr_Kind;
typedef struct A_Expr
--- 239,246 ----
AEXPR_BETWEEN, /* name must be "BETWEEN" */
AEXPR_NOT_BETWEEN, /* name must be "NOT BETWEEN" */
AEXPR_BETWEEN_SYM, /* name must be "BETWEEN SYMMETRIC" */
! AEXPR_NOT_BETWEEN_SYM, /* name must be "NOT BETWEEN SYMMETRIC" */
! AEXPR_PAREN /* nameless dummy node for parentheses */
} A_Expr_Kind;
typedef struct A_Expr
diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h
index 66391df..fbc3f17 100644
*** a/src/include/parser/parse_expr.h
--- b/src/include/parser/parse_expr.h
***************
*** 16,21 ****
--- 16,22 ----
#include "parser/parse_node.h"
/* GUC parameters */
+ extern bool operator_precedence_warning;
extern bool Transform_null_equals;
extern Node *transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind);
diff --git a/src/include/parser/scanner.h b/src/include/parser/scanner.h
index e6724bc..ac645b5 100644
*** a/src/include/parser/scanner.h
--- b/src/include/parser/scanner.h
*************** typedef union core_YYSTYPE
*** 51,56 ****
--- 51,57 ----
* %token <str> IDENT FCONST SCONST BCONST XCONST Op
* %token <ival> ICONST PARAM
* %token TYPECAST DOT_DOT COLON_EQUALS
+ * %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
* The above token definitions *must* be the first ones declared in any
* bison parser built atop this scanner, so that they will have consistent
* numbers assigned to them (specifically, IDENT = 258 and so on).
diff --git a/src/interfaces/ecpg/preproc/parse.pl b/src/interfaces/ecpg/preproc/parse.pl
index 36dce80..d715afd 100644
*** a/src/interfaces/ecpg/preproc/parse.pl
--- b/src/interfaces/ecpg/preproc/parse.pl
*************** my %replace_token = (
*** 42,52 ****
# or in the block
my %replace_string = (
'NULLS_LA' => 'nulls',
'WITH_LA' => 'with',
'TYPECAST' => '::',
'DOT_DOT' => '..',
! 'COLON_EQUALS' => ':=',);
# specific replace_types for specific non-terminals - never include the ':'
# ECPG-only replace_types are defined in ecpg-replace_types
--- 42,57 ----
# or in the block
my %replace_string = (
+ 'NOT_LA' => 'not',
'NULLS_LA' => 'nulls',
'WITH_LA' => 'with',
'TYPECAST' => '::',
'DOT_DOT' => '..',
! 'COLON_EQUALS' => ':=',
! 'LESS_EQUALS' => '<=',
! 'GREATER_EQUALS' => '>=',
! 'NOT_EQUALS' => '<>',
! );
# specific replace_types for specific non-terminals - never include the ':'
# ECPG-only replace_types are defined in ecpg-replace_types
diff --git a/src/interfaces/ecpg/preproc/parser.c b/src/interfaces/ecpg/preproc/parser.c
index 099a213..662a90a 100644
*** a/src/interfaces/ecpg/preproc/parser.c
--- b/src/interfaces/ecpg/preproc/parser.c
*************** filtered_base_yylex(void)
*** 75,80 ****
--- 75,83 ----
*/
switch (cur_token)
{
+ case NOT:
+ cur_token_length = 3;
+ break;
case NULLS_P:
cur_token_length = 5;
break;
*************** filtered_base_yylex(void)
*** 119,124 ****
--- 122,141 ----
/* Replace cur_token if needed, based on lookahead */
switch (cur_token)
{
+ case NOT:
+ /* Replace NOT by NOT_LA if it's followed by BETWEEN, IN, etc */
+ switch (next_token)
+ {
+ case BETWEEN:
+ case IN_P:
+ case LIKE:
+ case ILIKE:
+ case SIMILAR:
+ cur_token = NOT_LA;
+ break;
+ }
+ break;
+
case NULLS_P:
/* Replace NULLS_P by NULLS_LA if it's followed by FIRST or LAST */
switch (next_token)
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index 530712e..4760ddd 100644
*** a/src/interfaces/ecpg/preproc/pgc.l
--- b/src/interfaces/ecpg/preproc/pgc.l
*************** array ({ident_cont}|{whitespace}|[\[\]
*** 236,241 ****
--- 236,245 ----
typecast "::"
dot_dot \.\.
colon_equals ":="
+ less_equals "<="
+ greater_equals ">="
+ less_greater "<>"
+ not_equals "!="
/*
* "self" is the set of chars that should be returned as single-character
*************** cppline {space}*#([^i][A-Za-z]*|{if}|{
*** 620,625 ****
--- 624,633 ----
<SQL>{typecast} { return TYPECAST; }
<SQL>{dot_dot} { return DOT_DOT; }
<SQL>{colon_equals} { return COLON_EQUALS; }
+ <SQL>{less_equals} { return LESS_EQUALS; }
+ <SQL>{greater_equals} { return GREATER_EQUALS; }
+ <SQL>{less_greater} { return NOT_EQUALS; }
+ <SQL>{not_equals} { return NOT_EQUALS; }
<SQL>{informix_special} {
/* are we simulating Informix? */
if (INFORMIX_MODE)
*************** cppline {space}*#([^i][A-Za-z]*|{if}|{
*** 699,709 ****
return yytext[0];
}
! /* Convert "!=" operator to "<>" for compatibility */
! if (strcmp(yytext, "!=") == 0)
! yylval.str = mm_strdup("<>");
! else
! yylval.str = mm_strdup(yytext);
return Op;
}
<SQL>{param} {
--- 707,713 ----
return yytext[0];
}
! yylval.str = mm_strdup(yytext);
return Op;
}
<SQL>{param} {
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 506a313..761cfab 100644
*** a/src/pl/plpgsql/src/pl_gram.y
--- b/src/pl/plpgsql/src/pl_gram.y
*************** static void check_raise_parameters(PLp
*** 227,232 ****
--- 227,233 ----
%token <str> IDENT FCONST SCONST BCONST XCONST Op
%token <ival> ICONST PARAM
%token TYPECAST DOT_DOT COLON_EQUALS
+ %token LESS_EQUALS GREATER_EQUALS NOT_EQUALS
/*
* Other tokens recognized by plpgsql's lexer interface layer (pl_scanner.c).
On 20 February 2015 at 20:44, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Well, assuming that we're satisfied with just having a way to warn > when the behavior changed (and not, in particular, a switch that can > select old or new behavior) I'm in favour of your proposed improvements, but I'm having a problem thinking about random application breakage that would result. Having a warn_if_screwed parameter that we disable by default won't help much because if you are affected you can't change that situation. There are too many applications to test all of them and not all applications can be edited, even if they were tested. I think the way to do this is to have a pluggable parser, so users can choose 1) old parser, 2) new, better parser, 3) any other parser they fancy that they maintain to ensure application compatibility. We've got a pluggable executor and optimizer, so why not a parser too? Implementing that in the same way we have done for executor and optimizer is a 1 day patch, so easily achievable for 9.5. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
> On 20 February 2015 at 20:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, assuming that we're satisfied with just having a way to warn
>> when the behavior changed (and not, in particular, a switch that can
>> select old or new behavior)
> I'm in favour of your proposed improvements, but I'm having a problem
> thinking about random application breakage that would result.
> Having a warn_if_screwed parameter that we disable by default won't
> help much because if you are affected you can't change that situation.
> There are too many applications to test all of them and not all
> applications can be edited, even if they were tested.
I find this argument to be unhelpful, because it could be made in exactly
the same words against any non-backwards-compatible change whatsoever.
Nonetheless, we do make non-backwards-compatible changes all the time.
> I think the way to do this is to have a pluggable parser, so users can
> choose 1) old parser, 2) new, better parser, 3) any other parser they
> fancy that they maintain to ensure application compatibility. We've
> got a pluggable executor and optimizer, so why not a parser too?
> Implementing that in the same way we have done for executor and
> optimizer is a 1 day patch, so easily achievable for 9.5.
I don't find that particularly attractive either. It seems quite unlikely
to me that anyone would actually use such a hook; replacing the whole
parser would be essentially unmaintainable. Nobody uses the hooks you
mention to replace the whole planner or whole executor --- there are
wrappers out there, which is a different thing altogether. But you could
not undo the issue at hand here without at the very least a whole new
copy of gram.y, which would need to be updated constantly if you wanted
it to keep working across more than one release.
regards, tom lane
On 02/26/2015 10:56 AM, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> On 20 February 2015 at 20:44, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Well, assuming that we're satisfied with just having a way to warn >>> when the behavior changed (and not, in particular, a switch that can >>> select old or new behavior) >> I'm in favour of your proposed improvements, but I'm having a problem >> thinking about random application breakage that would result. >> Having a warn_if_screwed parameter that we disable by default won't >> help much because if you are affected you can't change that situation. >> There are too many applications to test all of them and not all >> applications can be edited, even if they were tested. > I find this argument to be unhelpful, because it could be made in exactly > the same words against any non-backwards-compatible change whatsoever. > Nonetheless, we do make non-backwards-compatible changes all the time. That's true, we do. But finding out where apps are going to break is not going to be easy. Reviewing a million lines of code to examine where changes in operator precendence might affect you could be an enormous undertaking for many users. I understand the need, but the whole prospect makes me very, very nervous, TBH. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 02/26/2015 10:56 AM, Tom Lane wrote:
>> I find this argument to be unhelpful, because it could be made in exactly
>> the same words against any non-backwards-compatible change whatsoever.
>> Nonetheless, we do make non-backwards-compatible changes all the time.
> That's true, we do. But finding out where apps are going to break is not
> going to be easy. Reviewing a million lines of code to examine where
> changes in operator precendence might affect you could be an enormous
> undertaking for many users. I understand the need, but the whole
> prospect makes me very, very nervous, TBH.
Well, again, it's not apparent to me why such arguments can't be used
to prevent us from ever again making any user-visible semantics change.
In this particular case, given I've gone to the trouble of creating a
warning mode, I think it would be easier to find potential problems than
it is for many of the compatibility changes we've made in the past.
A not-terribly-far-away example is your own recent change in casting
timestamp values to JSON; if someone had demanded a way to audit their
applications to find places where that might break things, could that
patch have been accepted? Indeed, could *any* of the backwards
compatibility breaks called out in the 9.4 notes have passed such a test?
I'm not honestly sure why we're holding this particular change to such
a high standard.
(Also, I think that most cases in practice are going to end up as parse
errors, not silently different query results, though I admit I haven't
got much evidence one way or the other on that. It'd be useful perhaps
if someone tried some large existing application against the patch to
see what happens. How many parse failures come up, and how many
warnings?)
regards, tom lane
On 26 February 2015 at 15:56, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I think the way to do this is to have a pluggable parser, so users can >> choose 1) old parser, 2) new, better parser, 3) any other parser they >> fancy that they maintain to ensure application compatibility. We've >> got a pluggable executor and optimizer, so why not a parser too? >> Implementing that in the same way we have done for executor and >> optimizer is a 1 day patch, so easily achievable for 9.5. > > I don't find that particularly attractive either. It seems quite unlikely > to me that anyone would actually use such a hook; replacing the whole > parser would be essentially unmaintainable. Nobody uses the hooks you > mention to replace the whole planner or whole executor --- there are > wrappers out there, which is a different thing altogether. But you could > not undo the issue at hand here without at the very least a whole new > copy of gram.y, which would need to be updated constantly if you wanted > it to keep working across more than one release. That's not what I see. Whole planner replacement is used for extensions by CitusData and NTT, and will definitely be used in the future for other apps. Whole executor replacement is also used by one extension to produce DDL triggers. In any case, whole planner replacement would be very desirable for people trying to minimize code churn in their applications. As soon as it exists, I will use to make some MySQL-only apps work seamlessly against PostgreSQL, such as WordPress. It doesn't need to be 100% perfect MySQL, it just needs to be enough to make the app work. Maintaining a code base that can work against multiple databases is hard. Writing it against one main database and maintaining a parser layer would be much easier. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
> On 26 February 2015 at 15:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't find that particularly attractive either. It seems quite unlikely
>> to me that anyone would actually use such a hook; replacing the whole
>> parser would be essentially unmaintainable. Nobody uses the hooks you
>> mention to replace the whole planner or whole executor --- there are
>> wrappers out there, which is a different thing altogether. But you could
>> not undo the issue at hand here without at the very least a whole new
>> copy of gram.y, which would need to be updated constantly if you wanted
>> it to keep working across more than one release.
> That's not what I see.
> Whole planner replacement is used for extensions by CitusData and NTT,
> and will definitely be used in the future for other apps.
> Whole executor replacement is also used by one extension to produce
> DDL triggers.
That sounds completely silly from here. You'd be better off maintaining
your code as a set of patches on the core code and shipping your own
binaries. It would be substantially less work to maintain, I'd think,
because otherwise you have to track every single patch made to what are
very large swaths of code. The costs of merge resolution (when your
changes specifically touch something also updated by the community) would
be about the same, but the gruntwork aspect would be considerably better.
And you'd have to be certifiably insane to ship such things as extensions
not tied to specific core binaries, anyway, so I'm not seeing an upside
in doing it like that.
Having said that, I won't stand in the way of somebody else proposing
parser hooks. I don't intend to do it myself though.
regards, tom lane
On 2/25/15 5:15 PM, Simon Riggs wrote: > Having a warn_if_screwed parameter that we disable by default won't > help much because if you are affected you can't change that situation. > There are too many applications to test all of them and not all > applications can be edited, even if they were tested. My suggestion was to treat this like the standard_conforming_string change. That is, warn for many years before changing.
On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e@gmx.net> wrote: >On 2/25/15 5:15 PM, Simon Riggs wrote: >> Having a warn_if_screwed parameter that we disable by default won't >> help much because if you are affected you can't change that >situation. >> There are too many applications to test all of them and not all >> applications can be edited, even if they were tested. > >My suggestion was to treat this like the standard_conforming_string >change. That is, warn for many years before changing. I don't think scs is a good example to follow. For one it didn't work well. For another the impact of scs was imo biggerand had security implications. Also people didn't really switch because of it until the default changed. At the veryleast there should be an option to error out, not warn. --- Please excuse brevity and formatting - I am writing this on my mobile phone.
Andres Freund <andres@anarazel.de> writes:
> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e@gmx.net> wrote:
>> My suggestion was to treat this like the standard_conforming_string
>> change. That is, warn for many years before changing.
> I don't think scs is a good example to follow.
Yeah. For one thing, there wouldn't be any way to suppress the warning
other than to parenthesize your code, which I would find problematic
because it would penalize standard-conforming queries. I'd prefer an
arrangement whereby once you fix your code to be standard-conforming,
you're done.
A possible point of compromise would be to leave the warning turned on
by default, at least until we get a better sense of how this would
play out in the real world. I continue to suspect that we're making
a mountain out of, if not a molehill, at least a hillock. I think most
sane people would have parenthesized their queries to start with rather
than go look up whether IS DISTINCT FROM binds tighter than <= ...
regards, tom lane
On 2/26/15 4:09 PM, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e@gmx.net> wrote: >>> My suggestion was to treat this like the standard_conforming_string >>> change. That is, warn for many years before changing. > >> I don't think scs is a good example to follow. > > Yeah. For one thing, there wouldn't be any way to suppress the warning > other than to parenthesize your code, which I would find problematic > because it would penalize standard-conforming queries. I'd prefer an > arrangement whereby once you fix your code to be standard-conforming, > you're done. > > A possible point of compromise would be to leave the warning turned on > by default, at least until we get a better sense of how this would > play out in the real world. I continue to suspect that we're making > a mountain out of, if not a molehill, at least a hillock. I think most > sane people would have parenthesized their queries to start with rather > than go look up whether IS DISTINCT FROM binds tighter than <= ... Question of sanity aside, I can tell you that many business queries are written with little more sophistication than monkeys with typewriters, where you keep the monkeys fed with bananas and paper until your query results (not the SQL) looks like what someone expects it to look like. Then the output of that version is held as sacrosanct, and any future SQL changes are wrong unless they produce the expected result changes. In my experience this happens because some poor business person just needs to get their job done and either isn't allowed to bother the data people or the data people are just too busy, so they're stuck doing it themselves. From what I've seen, SQL written by developers is often a bit better than this... but not a lot. And part of that salvation is because application queries tend to be a lot less complex than reporting/BI ones. I don't have a great feel for how much of an impact this specific change would have on that... the examples so far have all been pretty esoteric. I suspect most people writing such "wonderful" SQL don't know about IS DISTINCT FROM nor would they try doing things like bool_a > bool_b >= bool_c. So there may actually be very little code to fix, but I think we at least need a way for users to verify that. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On 2015-02-26 20:13:34 +0000, Simon Riggs wrote: > On 26 February 2015 at 15:56, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > >> I think the way to do this is to have a pluggable parser, so users can > >> choose 1) old parser, 2) new, better parser, 3) any other parser they > >> fancy that they maintain to ensure application compatibility. We've > >> got a pluggable executor and optimizer, so why not a parser too? > >> Implementing that in the same way we have done for executor and > >> optimizer is a 1 day patch, so easily achievable for 9.5. > > > > I don't find that particularly attractive either. It seems quite unlikely > > to me that anyone would actually use such a hook; replacing the whole > > parser would be essentially unmaintainable. Nobody uses the hooks you > > mention to replace the whole planner or whole executor --- there are > > wrappers out there, which is a different thing altogether. But you could > > not undo the issue at hand here without at the very least a whole new > > copy of gram.y, which would need to be updated constantly if you wanted > > it to keep working across more than one release. I can see a point in having the ability to plug in a parser - I just don't think it has much to do with this topic. It'd be a nightmare to maintain two versions of our parser; I don't think anybody wants to patch more than one. > Whole planner replacement is used for extensions by CitusData and NTT, > and will definitely be used in the future for other apps. s/planner/parser/? Because replacing the planner can already be done as a whole without much problems. > Whole executor replacement is also used by one extension to produce > DDL triggers. Hm? > In any case, whole planner replacement would be very desirable for > people trying to minimize code churn in their applications. As soon as > it exists, I will use to make some MySQL-only apps work seamlessly > against PostgreSQL, such as WordPress. It doesn't need to be 100% > perfect MySQL, it just needs to be enough to make the app work. > Maintaining a code base that can work against multiple databases is > hard. Writing it against one main database and maintaining a parser > layer would be much easier. Assuming you meant parser: Maybe. I have my doubt somebody will actually invest the significant amount of time to develop something like that. But I don't have a problem providing the capability; there seems little point in allowing to replace the optimizer but not the planner. I just don't see that it has much to do with this discussion. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e@gmx.net> wrote:
>>> My suggestion was to treat this like the standard_conforming_string
>>> change. That is, warn for many years before changing.
>> I don't think scs is a good example to follow.
> Yeah. For one thing, there wouldn't be any way to suppress the warning
> other than to parenthesize your code, which I would find problematic
> because it would penalize standard-conforming queries. I'd prefer an
> arrangement whereby once you fix your code to be standard-conforming,
> you're done.
> A possible point of compromise would be to leave the warning turned on
> by default, at least until we get a better sense of how this would
> play out in the real world. I continue to suspect that we're making
> a mountain out of, if not a molehill, at least a hillock. I think most
> sane people would have parenthesized their queries to start with rather
> than go look up whether IS DISTINCT FROM binds tighter than <= ...
This thread seems to have died off without any clear resolution. I'd
hoped somebody would try the patch on some nontrivial application to
see if it broke anything or caused any warnings, but it doesn't seem
like that is happening.
Do we have consensus on doing this? Should we have the warning on
by default, or off?
regards, tom lane
Tom Lane wrote: > Do we have consensus on doing this? Should we have the warning on > by default, or off? This is the tough decision, isn't it. I had thought it would default to off and people would only turn it on in their testing procedure prior to the actual upgrade of the production systems, but how are they going to find out they need to turn it on in the first place? We could have a big fat red blinking warning in the release notes and a picture of a dancing elephant in a tutu next to it, and we can be certain that many will miss it anyway. I think we should have an "expires" value: it means ON unless the system's initdb is older than one month from the current date, in which case it turns itself off. This is of course just a silly joke, but as you said there are probably valid constructs that are going to raise warnings for no reason, so having it default to ON would be pointlessly noisy in systems that have been developed with the new rules. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 10, 2015 at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Andres Freund <andres@anarazel.de> writes: >>> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e@gmx.net> wrote: >>>> My suggestion was to treat this like the standard_conforming_string >>>> change. That is, warn for many years before changing. > >>> I don't think scs is a good example to follow. > >> Yeah. For one thing, there wouldn't be any way to suppress the warning >> other than to parenthesize your code, which I would find problematic >> because it would penalize standard-conforming queries. I'd prefer an >> arrangement whereby once you fix your code to be standard-conforming, >> you're done. > >> A possible point of compromise would be to leave the warning turned on >> by default, at least until we get a better sense of how this would >> play out in the real world. I continue to suspect that we're making >> a mountain out of, if not a molehill, at least a hillock. I think most >> sane people would have parenthesized their queries to start with rather >> than go look up whether IS DISTINCT FROM binds tighter than <= ... > > This thread seems to have died off without any clear resolution. I'd > hoped somebody would try the patch on some nontrivial application to > see if it broke anything or caused any warnings, but it doesn't seem > like that is happening. > > Do we have consensus on doing this? Should we have the warning on > by default, or off? I vote for defaulting the warning to off. If that proves to be too problematic, I'd take that as a sign that this whole change is not as low-impact as we're hoping, and maybe consider a rethink. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 10, 2015 at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Do we have consensus on doing this? Should we have the warning on
> by default, or off?
I vote for defaulting the warning to off. If that proves to be too
problematic, I'd take that as a sign that this whole change is not as
low-impact as we're hoping, and maybe consider a rethink.
Do we want to have three states? On, Off, and Auto? We can then change what Auto means in a point-release while letting people who have chosen On or Off have their wish.
Auto could also consider some other data - like how long ago the database was initialized...
I would vote for Auto meaning On in the .0 release.
David J.
On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston <david.g.johnston@gmail.com> wrote: > On Tue, Mar 10, 2015 at 9:37 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Tue, Mar 10, 2015 at 12:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > Do we have consensus on doing this? Should we have the warning on >> > by default, or off? >> >> I vote for defaulting the warning to off. If that proves to be too >> problematic, I'd take that as a sign that this whole change is not as >> low-impact as we're hoping, and maybe consider a rethink. > > Do we want to have three states? On, Off, and Auto? We can then change > what Auto means in a point-release while letting people who have chosen On > or Off have their wish. > > Auto could also consider some other data - like how long ago the database > was initialized... > > I would vote for Auto meaning On in the .0 release. I don't think users will appreciate an auto value whose meaning might change at some point, and I doubt we've have much luck identifying the correct point, either. Users will upgrade over the course of years, not months, and will not necessarily complete their application retesting within any particular period of time thereafter. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston
> <david.g.johnston@gmail.com> wrote:
>> I would vote for Auto meaning On in the .0 release.
> I don't think users will appreciate an auto value whose meaning might
> change at some point, and I doubt we've have much luck identifying the
> correct point, either. Users will upgrade over the course of years,
> not months, and will not necessarily complete their application
> retesting within any particular period of time thereafter.
Yeah, I think that's too cute by far. And people do not like things like
this changing in point releases. If we do this, I envision it as being
on by default in 9.5 and then changing the default in 9.6 or 9.7 or so.
Another possibility is to leave it on through beta testing with the intent
to turn it off before 9.5 final; that would give us more data about
whether there are real issues than we're likely to get otherwise.
regards, tom lane
On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston >> <david.g.johnston@gmail.com> wrote: >>> I would vote for Auto meaning On in the .0 release. > >> I don't think users will appreciate an auto value whose meaning might >> change at some point, and I doubt we've have much luck identifying the >> correct point, either. Users will upgrade over the course of years, >> not months, and will not necessarily complete their application >> retesting within any particular period of time thereafter. > > Yeah, I think that's too cute by far. And people do not like things like > this changing in point releases. If we do this, I envision it as being > on by default in 9.5 and then changing the default in 9.6 or 9.7 or so. > > Another possibility is to leave it on through beta testing with the intent > to turn it off before 9.5 final; that would give us more data about > whether there are real issues than we're likely to get otherwise. To my mind, the fact that we're doing this at all is largely predicated on the fact that there won't be many real issues. So I think the goal of the debugging messages ought to be to let those people who discover that they do have issues track them down more easily, not to warn people. Warning is sort of closing the barn door after the horse has got out: hey, by the way, I just broke your app. Another thing to consider is that if it becomes common to run with these warnings on, then everybody will have to pretty much write their code with full parenthesization anyway, at least if they plan to publish their code on PGXN or anywhere that it might get run on some system other than the one it was written for. That seems like an annoying gotcha for an issue that we're not expecting to be common. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Another possibility is to leave it on through beta testing with the intent
>> to turn it off before 9.5 final; that would give us more data about
>> whether there are real issues than we're likely to get otherwise.
> To my mind, the fact that we're doing this at all is largely
> predicated on the fact that there won't be many real issues. So I
> think the goal of the debugging messages ought to be to let those
> people who discover that they do have issues track them down more
> easily, not to warn people. Warning is sort of closing the barn door
> after the horse has got out: hey, by the way, I just broke your app.
Agreed, but in the near term we need to *find out* whether there will
be many real issues. Perhaps having the warnings on by default would
help that, or perhaps not; I'm not sure.
> Another thing to consider is that if it becomes common to run with
> these warnings on, then everybody will have to pretty much write their
> code with full parenthesization anyway, at least if they plan to
> publish their code on PGXN or anywhere that it might get run on some
> system other than the one it was written for. That seems like an
> annoying gotcha for an issue that we're not expecting to be common.
Hm, well, people who are publishing code will likely want it to work
on both old and new PG releases, so I suspect they'd need to make it
run warning-free anyway.
regards, tom lane
On Tue, Mar 10, 2015 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
This thread seems to have died off without any clear resolution. I'd
hoped somebody would try the patch on some nontrivial application to
see if it broke anything or caused any warnings, but it doesn't seem
like that is happening.
I tried it on a fairly typical web application. Around 5000 or so distinct statements according to pg_stat_statements. With operator_precedence_warning = on, no warnings yet.
Alex Hunsaker <badalex@gmail.com> writes:
> On Tue, Mar 10, 2015 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This thread seems to have died off without any clear resolution. I'd
>> hoped somebody would try the patch on some nontrivial application to
>> see if it broke anything or caused any warnings, but it doesn't seem
>> like that is happening.
> I tried it on a fairly typical web application. Around 5000 or so distinct
> statements according to pg_stat_statements. With
> operator_precedence_warning = on, no warnings yet.
Thanks! Much appreciated.
regards, tom lane
On 3/10/15 1:12 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston >> <david.g.johnston@gmail.com> wrote: >>> I would vote for Auto meaning On in the .0 release. > >> I don't think users will appreciate an auto value whose meaning might >> change at some point, and I doubt we've have much luck identifying the >> correct point, either. Users will upgrade over the course of years, >> not months, and will not necessarily complete their application >> retesting within any particular period of time thereafter. > > Yeah, I think that's too cute by far. And people do not like things like > this changing in point releases. If we do this, I envision it as being > on by default in 9.5 and then changing the default in 9.6 or 9.7 or so. Well, I point again to standards_conforming_strings: Leave the warning off for one release (or more), then default to on for one (or more), then change the behavior. We can change the timeline, but I don't think the approach was unsound.
Peter Eisentraut <peter_e@gmx.net> writes:
> Well, I point again to standards_conforming_strings: Leave the warning
> off for one release (or more), then default to on for one (or more),
> then change the behavior.
> We can change the timeline, but I don't think the approach was unsound.
I'm not excited about that approach, for the reasons that were stated
upthread, mainly that there is little reason to think that anybody
paid any attention to the s_c_s transition till they were forced to.
Waiting to make the change will just allow more non-spec-compliant
SQL code to accumulate in the wild, without significantly reducing
the pain involved.
There's one more reason, too: the code I have is designed to give correct
warnings within the context of a parser that parses according to the
spec-compliant rules. It's possible that a similar approach could be used
to generate correct warnings within a parsetree built according to the old
rules, but it would be entirely different in detail and would need a lot
of additional work to develop. I don't particularly want to do that
additional work.
regards, tom lane
I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Another possibility is to leave it on through beta testing with the intent
>>> to turn it off before 9.5 final; that would give us more data about
>>> whether there are real issues than we're likely to get otherwise.
>> To my mind, the fact that we're doing this at all is largely
>> predicated on the fact that there won't be many real issues. So I
>> think the goal of the debugging messages ought to be to let those
>> people who discover that they do have issues track them down more
>> easily, not to warn people. Warning is sort of closing the barn door
>> after the horse has got out: hey, by the way, I just broke your app.
> Agreed, but in the near term we need to *find out* whether there will
> be many real issues. Perhaps having the warnings on by default would
> help that, or perhaps not; I'm not sure.
Given the lack of enthusiasm about that argument, I've pushed the patch
with warnings off by default. It's certainly easy enough to change that
aspect later if we choose to.
regards, tom lane
On 3/10/15 4:34 PM, Tom Lane wrote: > There's one more reason, too: the code I have is designed to give correct > warnings within the context of a parser that parses according to the > spec-compliant rules. It's possible that a similar approach could be used > to generate correct warnings within a parsetree built according to the old > rules, but it would be entirely different in detail and would need a lot > of additional work to develop. I don't particularly want to do that > additional work. So you want to change the precedence behavior in the next release and have a warning about "this code would have worked differently before". My understanding was that we would keep the precedence behavior for a while but warn about "this code would work differently in the future".
Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> Another possibility is to leave it on through beta testing >>>> with the intent to turn it off before 9.5 final; that would >>>> give us more data about whether there are real issues than >>>> we're likely to get otherwise. >>> >>> To my mind, the fact that we're doing this at all is largely >>> predicated on the fact that there won't be many real issues. >>> So I think the goal of the debugging messages ought to be to >>> let those people who discover that they do have issues track >>> them down more easily, not to warn people. Warning is sort of >>> closing the barn door after the horse has got out: hey, by the >>> way, I just broke your app. I don't get this argument at all. If the new code doesn't cause your queries to return different results (or update or delete different rows), you don't get a warning, right? Or are there false positives? If there are no false positives, turning it on is zero impact (except for any performance impact involved in detecting the condition) for those who have no problems. That will probably be the vast majority of users. The question is, do we want to quietly do something new and different for the small percentage of users who will have a problem, and leave it to them to find out about this setting and turn on the feature that tells them where the problems are? Or would it be more friendly to show the issues so they can resolve them, and then turn off the warnings once they are satisfied? Once users have established that they have no code that has different semantics under the standard operator precedence, they may want to turn it off to avoid the need for extra parentheses where pre-9.5 behavior would be different; but unless there is a significant performance hit for the check, they may want to leave it on just as insurance against mistakes. > Given the lack of enthusiasm about that argument, I've pushed the > patch with warnings off by default. It's certainly easy enough > to change that aspect later if we choose to. I think it should default to on for several major releases. Once the standard behavior is well-established we can default to off. I see that as a three to five year interval. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 11, 2015 at 8:00 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
If there are no false positives, turning it on is zero impact
(except for any performance impact involved in detecting the
condition) for those who have no problems.
Think of this as a bug fix. Hopefully nobody was using the syntax before because it didn't work right and if it did they were depending on the broken behaviour. But once it's fixed it would be frustrating to have it be fixed but have a warning saying "WARNING this query works fine now but didn't work in previous versions". Especially since warnings in DML are effectively fatal errors due to the frequency that they'll fire.
The warning can be useful for people testing code written in old versions or writing code intended to be multi-version compatible. But it's not something someone would want if they're writing code for 9.5.
greg
Greg Stark <stark@mit.edu> wrote: > On Wed, Mar 11, 2015 at 8:00 PM, Kevin Grittner <kgrittn@ymail.com> wrote: >> If there are no false positives, turning it on is zero impact >> (except for any performance impact involved in detecting the >> condition) for those who have no problems. > Think of this as a bug fix. I do. :-) > Hopefully nobody was using the syntax before because it didn't > work right and if it did they were depending on the broken > behaviour. Right; and they may have millions of lines of code which have been carefully tested and in production for years, and which may suddenly start to generate incorrect results on queries *or mangle existing data* with the fix unless they change their SQL code. > But once it's fixed it would be frustrating to have it be fixed > but have a warning saying "WARNING this query works fine now but > didn't work in previous versions". That's getting into subjective territory. What you say "works fine" may be completely unintended an unexpected, and may cause serious disruption of their business. You seem to be arguing that they deserve it for counting on our old, buggy implementation, and coming to accept that as expected behavior. > Especially since warnings in DML are effectively fatal errors due > to the frequency that they'll fire. In what situation? Code which was running under a previous major release of PostgreSQL? If they are getting so many of these warnings in that case, they would probably prefer to have this caught in pre-upgrade testing or at least as soon as possible. If it is new code written under 9.5 one would hope they have a testing phase where this would be detected so they could choose to add the parentheses needed to maintain portable code or turn off the warning. > The warning can be useful for people testing code written in old > versions or writing code intended to be multi-version compatible. We agree there... > But it's not something someone would want if they're writing code > for 9.5. ... and we also agree here; but I'm saying that in that case it will still rarely come up and if it does it's easy enough to disable. That inconvenience is not sufficient to not want to, by default, mangle the data or query results of some small percentage of existing users. If we ship with this off the results are entirely predictable. It will be somewhat surprising not to see any negative headlines about it. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 11, 2015 at 4:36 PM, Kevin Grittner <kgrittn@ymail.com> wrote: > If we ship with this off the results are entirely predictable. It > will be somewhat surprising not to see any negative headlines about > it. Can you, or can anyone, show a plausible example of something that would work under the old rules and work under the new rules but with a different meaning? I have to admit that I'm having some difficulty imagining exactly when that happens. Tom's examples upthread were not things that seemed all that likely. The most plausible example was probably a <= b || c, but the *old* interpretation of that is (a <= b) || c, so I'm having a little trouble taking that seriously as an example of where this would cause a problem. If the old interpretation had been a <= (b || c) and we were changing that to (a <= b) || c, then, yeah, that could break things for a lot of people, but not so many in this direction. Are there better examples of how this is going to be bite people? I really don't want to have another implicit-casting-changes type debacle here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: > Can you, or can anyone, show a plausible example of something > that would work under the old rules and work under the new rules > but with a different meaning? I have to admit that I'm having > some difficulty imagining exactly when that happens. Tom's > examples upthread were not things that seemed all that likely. I think very few will see any problems. At Wisconsin Courts we had millions of lines of code when we moved to PostgreSQL and saw one or two queries which had problems with this. The example I gave earlier on this thread was: | test=# select 'f'::boolean = 'f'::boolean >= 'f'::boolean; | ?column? | ---------- | f | (1 row) Now, that was a simple case for purposes of illustration, but imagine that the first two literals are column names in a complex query and it becomes slightly less ridiculous. Imagine they are being passed in to some plpgsql function which accepts different types, and the statement is run through EXECUTE and it may be somewhat reasonable. The upshot is that for at least 99% of shops turning this on will not generate any warnings at all. So, the question is what is better for the cases where it will actually matter? Either way it is like leaving the barn door open so that horses are capable of running out. We have an alarm that lets you know when something is going through the barn door; the question is whether to default that alarm on or off. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> Robert Haas <robertmhaas@gmail.com> wrote:
>> Can you, or can anyone, show a plausible example of something
>> that would work under the old rules and work under the new rules
>> but with a different meaning? I have to admit that I'm having
>> some difficulty imagining exactly when that happens. Tom's
>> examples upthread were not things that seemed all that likely.
> I think very few will see any problems. At Wisconsin Courts we had
> millions of lines of code when we moved to PostgreSQL and saw one
> or two queries which had problems with this. The example I gave
> earlier on this thread was:
> | test=# select 'f'::boolean = 'f'::boolean >= 'f'::boolean;
> | ?column?
> | ----------
> | f
> | (1 row)
> Now, that was a simple case for purposes of illustration, but
> imagine that the first two literals are column names in a complex
> query and it becomes slightly less ridiculous. Imagine they are
> being passed in to some plpgsql function which accepts different
> types, and the statement is run through EXECUTE and it may be
> somewhat reasonable.
Note that that example now *fails*, because = and => now belong
to the same precedence level which is %nonassoc:
regression=# select 'f'::boolean = 'f'::boolean >= 'f'::boolean;
ERROR: syntax error at or near ">="
LINE 1: select 'f'::boolean = 'f'::boolean >= 'f'::boolean; ^
(It'd be nice if it gave a more specific error message than just
"syntax error", but I'm not sure if there's any practical way to
persuade bison to do something different than that.)
I think that the set of practical examples that produce a different
answer without outright failing (either due to %nonassoc or due to
lack of any matching operator) is really going to be darn small.
> Either way it is like leaving the barn door open so that horses are
> capable of running out. We have an alarm that lets you know when
> something is going through the barn door; the question is whether
> to default that alarm on or off.
If we believe that the set of affected apps really is small, then
it seems like there should not be much downside to having the
warning on by default: by hypothesis, few people will ever see it
at all. OTOH, there is some small run-time cost to having it on...
The %nonassoc syntax error problem is likely to be a bigger problem
for user-friendliness than anything else about this patch, IMO.
Now that we're committed to this course, I will do some research
and see if that behavior can be improved at all.
regards, tom lane
Kevin Grittner <kgrittn@ymail.com> writes:
> If there are no false positives, turning it on is zero impact
> (except for any performance impact involved in detecting the
> condition) for those who have no problems. That will probably be
> the vast majority of users. The question is, do we want to quietly
> do something new and different for the small percentage of users
> who will have a problem, and leave it to them to find out about
> this setting and turn on the feature that tells them where the
> problems are? Or would it be more friendly to show the issues so
> they can resolve them, and then turn off the warnings once they are
> satisfied?
FWIW, there *are* some especially-corner-casey false positives,
as I noted upthread. I think the odds of people hitting them
are remarkably low, which is why I wasn't willing to invest the
additional code needed to try to make them all go away. I doubt
that this consideration is worth worrying about as we decide
whether the warnings should be on by default ... but if you're
going to make an argument from an assumption of no false positives,
it's wrong.
regards, tom lane
On Wed, Mar 11, 2015 at 6:19 PM, Kevin Grittner <kgrittn@ymail.com> wrote: > Either way it is like leaving the barn door open so that horses are > capable of running out. We have an alarm that lets you know when > something is going through the barn door; the question is whether > to default that alarm on or off. What we have an alarm that lets you know that your perfectly legitimate code might have meant something different in a release you aren't running. If that helps you catch a bug in code you are porting from a previous version, great. But otherwise it's simply a nuisance. I don't think this is like leaving the barn door open so that horses are capable of running out. I think it's more like insisting on wiring an alarm to every barn door in the county whether there are any animals currently housed in that barn or not. Now there is nothing wrong with giving away free alarm systems, but insisting on turning them all on except for the people who explicitly turn them off seems a little pushy. Also, given Tom's remarks downthread, we seem to still lack a plausible use case where the same code is legal in both versions but works differently. We should really keep trying to find one of those, because I think it would shed a lot of light on this debate. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 11, 2015 at 7:49 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kevin Grittner <kgrittn@ymail.com> writes: >> If there are no false positives, turning it on is zero impact >> (except for any performance impact involved in detecting the >> condition) for those who have no problems. That will probably be >> the vast majority of users. The question is, do we want to quietly >> do something new and different for the small percentage of users >> who will have a problem, and leave it to them to find out about >> this setting and turn on the feature that tells them where the >> problems are? Or would it be more friendly to show the issues so >> they can resolve them, and then turn off the warnings once they are >> satisfied? > > FWIW, there *are* some especially-corner-casey false positives, > as I noted upthread. I think the odds of people hitting them > are remarkably low, which is why I wasn't willing to invest the > additional code needed to try to make them all go away. I doubt > that this consideration is worth worrying about as we decide > whether the warnings should be on by default ... but if you're > going to make an argument from an assumption of no false positives, > it's wrong. Just out of curiosity, does this change create a dump-and-reload hazard? Like if I pg_upgrade my cluster, will the output of pg_dump potentially be sufficiently under-parenthesized that reload will create a non-equivalent database? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Just out of curiosity, does this change create a dump-and-reload
> hazard? Like if I pg_upgrade my cluster, will the output of pg_dump
> potentially be sufficiently under-parenthesized that reload will
> create a non-equivalent database?
No. Had there been any such hazard, I would not have been nearly
as enthused about this project.
In the first place, pg_dump disables ruleutils' pretty-printing
heuristics, so that you always get fully parenthesized expressions
no matter what. I insisted on that years ago, in the expectation
that we might someday need to do things like this patch; it's
always acted that way, in every release that had any pretty-printing
ability at all. For example,
regression=# create view vv as select unique1 + unique2*four as x from tenk1;
CREATE VIEW
regression=# \d+ vv View "public.vv"Column | Type | Modifiers | Storage | Description
--------+---------+-----------+---------+-------------x | integer | | plain |
View definition:SELECT tenk1.unique1 + tenk1.unique2 * tenk1.four AS x FROM tenk1;
regression=# \q
$ pg_dump -t vv regression
...
CREATE VIEW vv ASSELECT (tenk1.unique1 + (tenk1.unique2 * tenk1.four)) AS x FROM tenk1;
In the second place, AFAICS ruleutils' heuristics do not know anything
about any of the constructs affected by this patch, and so they'll be
fully parenthesized anyway, even in pretty-printed output.
For example:
regression=# create view vvv as select 1+2 is distinct from 42 as q;
CREATE VIEW
regression=# \d+ vvv View "public.vvv"Column | Type | Modifiers | Storage | Description
--------+---------+-----------+---------+-------------q | boolean | | plain |
View definition:SELECT (1 + 2) IS DISTINCT FROM 42 AS q;
The parens are unnecessary, but ruleutils doesn't know that, so
it puts them in.
regards, tom lane
On Wed, Mar 11, 2015 at 8:36 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Right; and they may have millions of lines of code which have been
carefully tested and in production for years, and which may
suddenly start to generate incorrect results on queries *or mangle
existing data* with the fix unless they change their SQL code.
Well not suddenly. When doing a major upgrade. And we provide a tool to help them identify the problems.
But having a warning enabled by default means the problem is effectively not fixed at all. People will not be able to write code that's valid according to the docs and the spec without extra parentheses or disabling the warning.
greg
On Tue, Mar 10, 2015 at 04:10:01PM -0400, Peter Eisentraut wrote: > On 3/10/15 1:12 PM, Tom Lane wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston > >> <david.g.johnston@gmail.com> wrote: > >>> I would vote for Auto meaning On in the .0 release. > > > >> I don't think users will appreciate an auto value whose meaning might > >> change at some point, and I doubt we've have much luck identifying the > >> correct point, either. Users will upgrade over the course of years, > >> not months, and will not necessarily complete their application > >> retesting within any particular period of time thereafter. > > > > Yeah, I think that's too cute by far. And people do not like things like > > this changing in point releases. If we do this, I envision it as being > > on by default in 9.5 and then changing the default in 9.6 or 9.7 or so. > > Well, I point again to standards_conforming_strings: Leave the warning > off for one release (or more), then default to on for one (or more), > then change the behavior. > > We can change the timeline, but I don't think the approach was unsound. Sorry to be replying late (but when has that ever stopped me :-) ), but for standards_conforming_strings, there were security concerns about the change, and we are forcing people to move to a new necessary syntax when using backslash escapes, i.e. E''. Once they moved to E'', the warnings went away. In this case, the way to avoid the warnings is to add _unnecessary_ parentheses, and sometimes in cases that don't need them, and never will. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Feb 19, 2015 at 10:48:34AM -0500, Tom Lane wrote:
> To wit, that the precedence of <= >= and <> is neither sane nor standards
> compliant.
> I claim that this behavior is contrary to spec as well as being
> unintuitive. Following the grammar productions in SQL99:
Between 1999 and 2006, SQL changed its representation of the grammar in this
area; I have appended to this message some of the key productions as of 2013.
I did not notice a semantic change, though.
> We have that right for = < > but not for the other three standard-mandated
> comparison operators. I think we should change the grammar so that all
> six act like < > do now, that is, they should have %nonassoc precedence
> just above NOT.
>
> Another thought, looking at this closely, is that we have the precedence
> of IS tests (IS NOT NULL etc) wrong as well: they should bind less tightly
> than user-defined ops, not more so.
SQL has two groups of IS tests with different precedence. The <boolean test>
productions IS [NOT] {TRUE | FALSE | UNKNOWN} have precedence just lower than
"<", and the <null predicate> productions IS [NOT] NULL have precedence equal
to "<". (An implementation giving them the same precedence can conform,
because conforming queries cannot notice the difference.)
I attempted to catalog the diverse precedence changes in commit c6b3c93:
> @@ -647,13 +654,11 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
> %left OR
> %left AND
> %right NOT
> -%right '='
> -%nonassoc '<' '>'
> -%nonassoc LIKE ILIKE SIMILAR
> -%nonassoc ESCAPE
> +%nonassoc IS ISNULL NOTNULL /* IS sets precedence for IS NULL, etc */
> +%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
> +%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
> +%nonassoc ESCAPE /* ESCAPE must be just above LIKE/ILIKE/SIMILAR */
> %nonassoc OVERLAPS
> -%nonassoc BETWEEN
> -%nonassoc IN_P
> %left POSTFIXOP /* dummy for postfix Op rules */
> /*
> * To support target_el without AS, we must give IDENT an explicit priority
> @@ -678,9 +683,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
> %nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */
> %nonassoc IDENT NULL_P PARTITION RANGE ROWS PRECEDING FOLLOWING
> %left Op OPERATOR /* multi-character ops and user-defined operators */
> -%nonassoc NOTNULL
> -%nonassoc ISNULL
> -%nonassoc IS /* sets precedence for IS NULL, etc */
> %left '+' '-'
> %left '*' '/' '%'
> %left '^'
1. Decrease precedence of "<=", ">=" and "<>" to match "<".
2. Increase precedence of, for example, "BETWEEN x AND Y" to match precedence with "BETWEEN" keyword instead of "AND"
keyword. Make similar precedence changes to other multiple-keyword productions involving "AND", "NOT", etc.
3. Decrease precedence of IS [NOT] {TRUE | FALSE | UNKNOWN} to fall between NOT and "<".
4. Decrease precedence of IS [NOT] NULL and IS[NOT]NULL to match IS [NOT] {TRUE | FALSE | UNKNOWN}.
5. Forbid chains of "=" (make it nonassoc), and increase its precedence to match "<".
6. Decrease precedence of BETWEEN and IN keywords to match "LIKE".
> It's
> definitely weird that the IS tests bind more tightly than multicharacter
> Ops but less tightly than + - * /.
(1), (2) and (3) improve SQL conformance, and that last sentence seems to
explain your rationale for (4). I've been unable to explain (5) and (6). Why
in particular the following three precedence groups instead of combining them
as in SQL or subdividing further as in PostgreSQL 9.4?
> +%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
> +%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
> %nonassoc OVERLAPS
Thanks,
nm
<comparison predicate> ::= <row value predicand> <comparison predicate part 2>
<comparison predicate part 2> ::= <comp op> <row value predicand>
<row value predicand> ::= <row value special case> | <row value constructor predicand>
# Syntax Rules
# 1) The declared type of a <row value special case> shall be a row type.
<row value special case> ::= <nonparenthesized value expression primary>
# Things with precedence higher than comparison.
<row value constructor predicand> ::= <common value expression> | <boolean predicand> | <explicit row value
constructor>
# numeric: addition, multiplication
# string: concat, collate clause
# datetime: addition, AT TIME ZONE
# interval: addition, division
# UDT: <value expression primary>
# reference: <value expression primary>
# collection: array/multiset
<common value expression> ::= <numeric value expression> | <string value expression> | <datetime value expression> |
<intervalvalue expression> | <user-defined type value expression> | <reference value expression> | <collection value
expression>
<boolean predicand> ::= <parenthesized boolean value expression> | <nonparenthesized value expression primary>
<parenthesized boolean value expression> ::= <left paren> <boolean value expression> <right paren>
# Things unambiguous without parens.
<nonparenthesized value expression primary> ::= <unsigned value specification> | <column reference> | <set function
specification>| <window function> | <scalar subquery> | <case expression> | <cast specification> | <field reference> |
<subtypetreatment> | <method invocation> | <static method invocation> | <new specification> | <attribute or method
reference>| <reference resolution> | <collection value constructor> | <array element reference> | <multiset element
reference>| <next value expression> | <routine invocation> | <row pattern navigation operation>
<boolean value expression> ::= <boolean term> | <boolean value expression> OR <boolean term>
<boolean term> ::= <boolean factor> | <boolean term> AND <boolean factor>
<boolean factor> ::= [ NOT ] <boolean test>
<boolean test> ::= <boolean primary> [ IS [ NOT ] <truth value> ]
<boolean primary> ::= <predicate> | <boolean predicand>
<truth value> ::= TRUE | FALSE | UNKNOWN
# Things with precedence equal to comparison.
<predicate> ::= <comparison predicate> | <between predicate> | <in predicate> | <like predicate> | <similar
predicate>| <regex like predicate> | <null predicate> | <quantified comparison predicate> | <exists predicate> |
<uniquepredicate> | <normalized predicate> | <match predicate> | <overlaps predicate> | <distinct predicate> | <member
predicate>| <submultiset predicate> | <set predicate> | <type predicate> | <period predicate>
<null predicate> ::= <row value predicand> <null predicate part 2>
<null predicate part 2> ::= IS [ NOT ] NULL
Noah Misch <noah@leadboat.com> writes:
> SQL has two groups of IS tests with different precedence. The <boolean test>
> productions IS [NOT] {TRUE | FALSE | UNKNOWN} have precedence just lower than
> "<", and the <null predicate> productions IS [NOT] NULL have precedence equal
> to "<". (An implementation giving them the same precedence can conform,
> because conforming queries cannot notice the difference.)
I'm curious about your rationale for claiming that <null predicate> has
precedence exactly equal to "<" according to the spec. AFAICS the SQL
spec doesn't really tell us much about precedence of different subparts
of the grammar; at best you can infer that some things bind tighter than
others.
> I attempted to catalog the diverse precedence changes in commit c6b3c93:
> 1. Decrease precedence of "<=", ">=" and "<>" to match "<".
Check.
> 2. Increase precedence of, for example, "BETWEEN x AND Y" to match precedence
> with "BETWEEN" keyword instead of "AND" keyword. Make similar precedence
> changes to other multiple-keyword productions involving "AND", "NOT", etc.
Uh, no, I wouldn't have said that. I decreased BETWEEN's precedence,
along with IN's, to be less than OVERLAPS' precedence, matching the
precedence of LIKE/ILIKE/SIMILAR. (But see comment below about OVERLAPS.)
There was not any case where the AND would have determined its precedence
AFAICS.
> 3. Decrease precedence of IS [NOT] {TRUE | FALSE | UNKNOWN} to fall between
> NOT and "<".
Check.
> 4. Decrease precedence of IS [NOT] NULL and IS[NOT]NULL to match IS [NOT]
> {TRUE | FALSE | UNKNOWN}.
Check.
> 5. Forbid chains of "=" (make it nonassoc), and increase its precedence to
> match "<".
Check.
> 6. Decrease precedence of BETWEEN and IN keywords to match "LIKE".
Check.
>> It's
>> definitely weird that the IS tests bind more tightly than multicharacter
>> Ops but less tightly than + - * /.
> (1), (2) and (3) improve SQL conformance, and that last sentence seems to
> explain your rationale for (4).
The real reason for (4) is that it would be very difficult to get bison to
consider IS TRUE to have different precedence (against an operator on its
left) than IS NULL; we'd need even more lookahead mess than we have now.
They were not different precedence before, and they aren't now.
I did change ISNULL and NOTNULL to have exactly the same precedence as the
long forms IS NULL and IS NOT NULL, where before they were (for no good
reason AFAICS) slightly different precedence. I think that's justifiable
on the grounds that they should not act differently from the long forms.
In any case the SQL standard has nothing to teach us on the point, since
it doesn't admit these shorthands.
> I've been unable to explain (5) and (6).
I'm not following your concern about (5). The spec seems to clearly
put all six basic comparison operators on the same precedence level.
I believe that the reason our grammar had '=' as right-associative is
someone's idea that we might someday consider assignment as a regular
operator a la C, but that never has been true and seems unlikely to
become true in the future. There's certainly nothing in the spec
suggesting that '=' should be right-associative.
The reason for (6) was mainly that having IN/BETWEEN bind tighter than
LIKE doesn't seem to me to have any justification in the spec; moreover,
if it does bind tighter, we're delivering a boolean result to LIKE which
seems unlikely to be what anyone wants. So I figured we could simplify
things a bit by having one (or two really) fewer precedence levels.
Also, because said level is %nonassoc, this will now force people to
parenthesize if they do indeed want something like a BETWEEN as argument
of a LIKE, which seems like a good thing all round.
> Why in particular the following three precedence groups instead of
> combining them as in SQL or subdividing further as in PostgreSQL 9.4?
>> +%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
>> +%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
>> %nonassoc OVERLAPS
I think that the spec is fairly clear that the six comparison operators
bind looser than other operators. Now you could argue about whether LIKE
et al are "operators" but Postgres certainly treats them as such.
OVERLAPS is a special case in that it doesn't really need precedence at
all: both its arguments are required to be parenthesized. We could
possibly have removed it from the precedence hierarchy altogether, but
I didn't bother experimenting with that, just left it alone. But
because of that, "moving BETWEEN/IN below it" doesn't really change
anything.
regards, tom lane
I wrote:
> Noah Misch <noah@leadboat.com> writes:
>> Why in particular the following three precedence groups instead of
>> combining them as in SQL or subdividing further as in PostgreSQL 9.4?
> +%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
> +%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
> %nonassoc OVERLAPS
> OVERLAPS is a special case in that it doesn't really need precedence at
> all: both its arguments are required to be parenthesized. We could
> possibly have removed it from the precedence hierarchy altogether, but
> I didn't bother experimenting with that, just left it alone. But
> because of that, "moving BETWEEN/IN below it" doesn't really change
> anything.
I got off my rear and did the experiment, and found that indeed simply
removing "%nonassoc OVERLAPS" seems to work and not change any behavior
(in fact, the generated gram.c is identical). Shall we do that and remove
the listing of OVERLAPS in the documentation's precedence table?
regards, tom lane
On Sun, Aug 09, 2015 at 04:48:22PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > SQL has two groups of IS tests with different precedence. The <boolean test>
> > productions IS [NOT] {TRUE | FALSE | UNKNOWN} have precedence just lower than
> > "<", and the <null predicate> productions IS [NOT] NULL have precedence equal
> > to "<". (An implementation giving them the same precedence can conform,
> > because conforming queries cannot notice the difference.)
>
> I'm curious about your rationale for claiming that <null predicate> has
> precedence exactly equal to "<" according to the spec. AFAICS the SQL
> spec doesn't really tell us much about precedence of different subparts
> of the grammar; at best you can infer that some things bind tighter than
> others.
Both <null predicate> and <comparison predicate> are in the set of productions
that take <row value predicand> arguments and appear only in <predicate>.
Passing a production in that set as an argument to a production in that set
requires parentheses. To restate (non-associative) "precedence equal" more
pedantically, there exists no conforming query whose interpretation hinges on
the relative precedence of "IS NULL" and "<".
> > 2. Increase precedence of, for example, "BETWEEN x AND Y" to match precedence
> > with "BETWEEN" keyword instead of "AND" keyword. Make similar precedence
> > changes to other multiple-keyword productions involving "AND", "NOT", etc.
>
> Uh, no, I wouldn't have said that. I decreased BETWEEN's precedence,
> along with IN's, to be less than OVERLAPS' precedence, matching the
> precedence of LIKE/ILIKE/SIMILAR. (But see comment below about OVERLAPS.)
> There was not any case where the AND would have determined its precedence
> AFAICS.
Ah. I read this patch hunk carelessly:
> > > @@ -11420,7 +11436,7 @@ a_expr: c_expr { $$ = $1; }
> > > {
> > > $$ = (Node *) makeSimpleA_Expr(AEXPR_OF, "<>", $1, (Node *) $6, @2);
> > > }
> > > - | a_expr BETWEEN opt_asymmetric b_expr AND b_expr %prec BETWEEN
> > > + | a_expr BETWEEN opt_asymmetric b_expr AND a_expr %prec BETWEEN
That's allowing additional productions in the final BETWEEN operand, not
changing precedence.
> > 5. Forbid chains of "=" (make it nonassoc), and increase its precedence to
> > match "<".
> > 6. Decrease precedence of BETWEEN and IN keywords to match "LIKE".
> > I've been unable to explain (5) and (6).
>
> I'm not following your concern about (5). The spec seems to clearly
> put all six basic comparison operators on the same precedence level.
> [snipped rest of explanation]
No particular concern beyond wanting to know the rationale; thanks for writing
one. Getting this wrong a second time would be awfully sad, so I'm being more
cautious than usual.
> > Why in particular the following three precedence groups instead of
> > combining them as in SQL or subdividing further as in PostgreSQL 9.4?
>
> >> +%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
> >> +%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
> >> %nonassoc OVERLAPS
>
> I think that the spec is fairly clear that the six comparison operators
> bind looser than other operators. Now you could argue about whether LIKE
> et al are "operators" but Postgres certainly treats them as such.
To my knowledge, SQL is agnostic about whether LIKE "is an operator". The six
comparison operators bind looser than <common value expression> syntax like
"*" and "||", tighter than IS TRUE, and indistinguishable from <predicate>
syntax like IS DISTINCT FROM and LIKE.
> OVERLAPS is a special case in that it doesn't really need precedence at
> all: both its arguments are required to be parenthesized. We could
> possibly have removed it from the precedence hierarchy altogether, but
> I didn't bother experimenting with that, just left it alone. But
> because of that, "moving BETWEEN/IN below it" doesn't really change
> anything.
Ah, quite right. SQL OVERLAPS takes various forms of two-column input, but
PostgreSQL OVERLAPS is more particular. I like your subsequent proposal to
remove OVERLAPS from the order of precedence.
Thanks,
nm
Noah Misch <noah@leadboat.com> writes:
> On Sun, Aug 09, 2015 at 04:48:22PM -0400, Tom Lane wrote:
>> I'm curious about your rationale for claiming that <null predicate> has
>> precedence exactly equal to "<" according to the spec.
> Both <null predicate> and <comparison predicate> are in the set of productions
> that take <row value predicand> arguments and appear only in <predicate>.
> Passing a production in that set as an argument to a production in that set
> requires parentheses. To restate (non-associative) "precedence equal" more
> pedantically, there exists no conforming query whose interpretation hinges on
> the relative precedence of "IS NULL" and "<".
Ah. So really, according to the spec IS and "<" could have any precedence
relative to each other as long as there is no other construct between.
Works for me.
> To my knowledge, SQL is agnostic about whether LIKE "is an operator". The six
> comparison operators bind looser than <common value expression> syntax like
> "*" and "||", tighter than IS TRUE, and indistinguishable from <predicate>
> syntax like IS DISTINCT FROM and LIKE.
"Indistinguishable" in the same sense as above, right? So for our
purposes, it's better to keep BETWEEN and friends as binding slightly
tighter than '<' than to make them the same precedence. Same precedence
risks breaking things that weren't broken before. Also, while I argued
above that making BETWEEN a potential argument for LIKE wasn't sensible,
that's not true for the comparison operators. In particular, boolean '<='
is the only SQL-provided spelling for logical implication.
>> OVERLAPS is a special case in that it doesn't really need precedence at
>> all: both its arguments are required to be parenthesized. We could
>> possibly have removed it from the precedence hierarchy altogether, but
>> I didn't bother experimenting with that, just left it alone. But
>> because of that, "moving BETWEEN/IN below it" doesn't really change
>> anything.
> Ah, quite right. SQL OVERLAPS takes various forms of two-column input, but
> PostgreSQL OVERLAPS is more particular. I like your subsequent proposal to
> remove OVERLAPS from the order of precedence.
Yeah. If we ever extend our OVERLAPS syntax, we might have to assign a
precedence to OVERLAPS, but we can do that then --- and it would be good
if we did not at that time have a false impression that the precedence
was already determined by previous decisions. I'll go make that change.
regards, tom lane
On Sun, Aug 09, 2015 at 06:44:58PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Sun, Aug 09, 2015 at 04:48:22PM -0400, Tom Lane wrote: > >> I'm curious about your rationale for claiming that <null predicate> has > >> precedence exactly equal to "<" according to the spec. > > > Both <null predicate> and <comparison predicate> are in the set of productions > > that take <row value predicand> arguments and appear only in <predicate>. > > Passing a production in that set as an argument to a production in that set > > requires parentheses. To restate (non-associative) "precedence equal" more > > pedantically, there exists no conforming query whose interpretation hinges on > > the relative precedence of "IS NULL" and "<". > > Ah. So really, according to the spec IS and "<" could have any precedence > relative to each other as long as there is no other construct between. > Works for me. > > > To my knowledge, SQL is agnostic about whether LIKE "is an operator". The six > > comparison operators bind looser than <common value expression> syntax like > > "*" and "||", tighter than IS TRUE, and indistinguishable from <predicate> > > syntax like IS DISTINCT FROM and LIKE. > > "Indistinguishable" in the same sense as above, right? Right. > So for our > purposes, it's better to keep BETWEEN and friends as binding slightly > tighter than '<' than to make them the same precedence. Same precedence > risks breaking things that weren't broken before. It does risk that. Same deal with making "=" have the same precedence as "<" instead of keeping it slightly lower.
Noah Misch <noah@leadboat.com> writes:
> On Sun, Aug 09, 2015 at 06:44:58PM -0400, Tom Lane wrote:
>> So for our
>> purposes, it's better to keep BETWEEN and friends as binding slightly
>> tighter than '<' than to make them the same precedence. Same precedence
>> risks breaking things that weren't broken before.
> It does risk that. Same deal with making "=" have the same precedence as "<"
> instead of keeping it slightly lower.
Agreed, but in that case I think our hand is forced by the SQL standard.
regards, tom lane
On Sun, Aug 09, 2015 at 07:16:02PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Sun, Aug 09, 2015 at 06:44:58PM -0400, Tom Lane wrote: > >> So for our > >> purposes, it's better to keep BETWEEN and friends as binding slightly > >> tighter than '<' than to make them the same precedence. Same precedence > >> risks breaking things that weren't broken before. > > > It does risk that. Same deal with making "=" have the same precedence as "<" > > instead of keeping it slightly lower. > > Agreed, but in that case I think our hand is forced by the SQL standard. In SQL:2008 and SQL:2011 at least, "=", "<" and "BETWEEN" are all in the same boat. They have no precedence relationships to each other; SQL sidesteps the question by requiring parentheses. They share a set of precedence relationships to other constructs. SQL does not imply whether to put them in one %nonassoc precedence group or in a few, but we can contemplate whether users prefer an error or prefer the 9.4 behavior for affected queries.
Noah Misch <noah@leadboat.com> writes:
> On Sun, Aug 09, 2015 at 07:16:02PM -0400, Tom Lane wrote:
>> Noah Misch <noah@leadboat.com> writes:
>>> It does risk that. Same deal with making "=" have the same precedence as "<"
>>> instead of keeping it slightly lower.
>> Agreed, but in that case I think our hand is forced by the SQL standard.
> In SQL:2008 and SQL:2011 at least, "=", "<" and "BETWEEN" are all in the same
> boat. They have no precedence relationships to each other; SQL sidesteps the
> question by requiring parentheses. They share a set of precedence
> relationships to other constructs. SQL does not imply whether to put them in
> one %nonassoc precedence group or in a few, but we can contemplate whether
> users prefer an error or prefer the 9.4 behavior for affected queries.
Part of my thinking was that the 9.4 behavior fails the principle of least
astonishment, because I seriously doubt that people expect '=' to be
either right-associative or lower priority than '<'. Here's one example:
regression=# select false = true < false;?column?
----------t
(1 row)
Not only does that seem unintuitive, but I actually had to experiment
a bit before finding a combination of values in which I got a different
result from what you'd expect if you think the precedence is (x = y) < z.
So it's not hard to imagine that somebody might write a query thinking
that that's how it works, and even have it get through desultory testing
before silently giving unexpected answers in production.
So yeah, I do think that getting a syntax error if you don't use
parentheses is the preferable behavior here.
regards, tom lane
On Sun, Aug 09, 2015 at 08:06:11PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > In SQL:2008 and SQL:2011 at least, "=", "<" and "BETWEEN" are all in the same > > boat. They have no precedence relationships to each other; SQL sidesteps the > > question by requiring parentheses. They share a set of precedence > > relationships to other constructs. SQL does not imply whether to put them in > > one %nonassoc precedence group or in a few, but we can contemplate whether > > users prefer an error or prefer the 9.4 behavior for affected queries. > > Part of my thinking was that the 9.4 behavior fails the principle of least > astonishment, because I seriously doubt that people expect '=' to be > either right-associative or lower priority than '<'. Here's one example: > > regression=# select false = true < false; > ?column? > ---------- > t > (1 row) > So yeah, I do think that getting a syntax error if you don't use > parentheses is the preferable behavior here. I agree.
2015-08-10 5:37 GMT+02:00 Noah Misch <noah@leadboat.com>:
On Sun, Aug 09, 2015 at 08:06:11PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > In SQL:2008 and SQL:2011 at least, "=", "<" and "BETWEEN" are all in the same
> > boat. They have no precedence relationships to each other; SQL sidesteps the
> > question by requiring parentheses. They share a set of precedence
> > relationships to other constructs. SQL does not imply whether to put them in
> > one %nonassoc precedence group or in a few, but we can contemplate whether
> > users prefer an error or prefer the 9.4 behavior for affected queries.
>
> Part of my thinking was that the 9.4 behavior fails the principle of least
> astonishment, because I seriously doubt that people expect '=' to be
> either right-associative or lower priority than '<'. Here's one example:
>
> regression=# select false = true < false;
> ?column?
> ----------
> t
> (1 row)
> So yeah, I do think that getting a syntax error if you don't use
> parentheses is the preferable behavior here.
If we raise a syntax error, then there should be very informative message, because pattern
true = 2 > 1 is probably relative often
and it is hard to find syntax error on this trivial expression
Regards
Pavel
I agree.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Pavel Stehule <pavel.stehule@gmail.com> writes:
>> On Sun, Aug 09, 2015 at 08:06:11PM -0400, Tom Lane wrote:
>>> So yeah, I do think that getting a syntax error if you don't use
>>> parentheses is the preferable behavior here.
> If we raise a syntax error, then there should be very informative message,
Yeah, it would sure be nice to throw something better than "syntax error".
But I've looked at bison's facilities for that, and they're pretty bad.
I'm not sure there's much we can do short of moving to some other parser
generator tool, which would be a Large Undertaking ... and I don't know
of any arguably-better tool anyway.
regards, tom lane
Pavel Stehule <pavel.stehule@gmail.com> writes:
>> On Sun, Aug 09, 2015 at 08:06:11PM -0400, Tom Lane wrote:
>>> So yeah, I do think that getting a syntax error if you don't use
>>> parentheses is the preferable behavior here.
> If we raise a syntax error, then there should be very informative message,
Yeah, it would sure be nice to throw something better than "syntax error".
But I've looked at bison's facilities for that, and they're pretty bad.
I'm not sure there's much we can do short of moving to some other parser
generator tool, which would be a Large Undertaking ... and I don't know
of any arguably-better tool anyway.
I would say that anyone who's tricksy enough to be using boolean logic in the way you describe would be expected to have enough nouse about them to either a) know what the precedences are or b) know that their lack of knowledge means they should sprinkle their code with
brackets
.
Returning a syntax error for something that isn't actually an error in syntax is a poor showing.
Are we to start expecting
syntax errors when people use comparison operators on NULLable
columns
without a COALESCE
because the comparison might do something they don't expect if they haven't tested their code with NULL values
?IMO using a = b < c as described is unnecessarily* horrid, whichever way you mean it, and will only produce the sort of unreadability that generates errors in the long run anyway (even if you understand it, chances are the next poor sap reading your code won't) and deserves code that breaks, especially if you're the sort of person who uses it without fully understanding it.
(*Unnecessarily because there are clearer constructs - CASE springs to mind - that do the same thing without the associated unreadability and/or ambiguity)
Geoff