Precedence of standard comparison operators

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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:

/messages/by-id/200712171958.lBHJwOBb037317@wwwmaster.postgresql.org

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers