Re: 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 <boolean term>

<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>
| <overlaps predicate>
| <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 or equals operator>

<row value expression> ::=
<row value special case>
| <row value constructor>

<contextually typed row value expression> ::=
<row value special case>
| <contextually typed 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(at)sss(dot)pgh(dot)pa(dot)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(at)ymail(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)ymail(dot)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(at)ymail(dot)com> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
>>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmx(dot)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(at)gmx(dot)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

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

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(at)sss(dot)pgh(dot)pa(dot)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(at)2ndQuadrant(dot)com> writes:
> On 20 February 2015 at 20:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)2ndQuadrant(dot)com> writes:
>> On 20 February 2015 at 20:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)dunslane(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)2ndQuadrant(dot)com> writes:
> On 26 February 2015 at 15:56, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmx(dot)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 bigger and had security implications. Also people didn't really switch because of it until the default changed. At the very least 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(at)anarazel(dot)de> writes:
> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e(at)gmx(dot)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(at)anarazel(dot)de> writes:
>> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e(at)gmx(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)anarazel(dot)de> writes:
>> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e(at)gmx(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Andres Freund <andres(at)anarazel(dot)de> writes:
>>> On February 26, 2015 10:29:18 PM CET, Peter Eisentraut <peter_e(at)gmx(dot)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 9:37 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Mar 10, 2015 at 12:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> On Tue, Mar 10, 2015 at 9:37 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Mar 10, 2015 at 12:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston
> <david(dot)g(dot)johnston(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston
>> <david(dot)g(dot)johnston(at)gmail(dot)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(at)gmail(dot)com> writes:
> On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
> On Tue, Mar 10, 2015 at 10:11 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)gmail(dot)com> writes:
>> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston
>> <david(dot)g(dot)johnston(at)gmail(dot)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(at)gmx(dot)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(at)gmail(dot)com> writes:
>> On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Tue, Mar 10, 2015 at 1:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)ymail(dot)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(at)mit(dot)edu> wrote:
> On Wed, Mar 11, 2015 at 8:00 PM, Kevin Grittner <kgrittn(at)ymail(dot)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(at)ymail(dot)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(at)gmail(dot)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(at)ymail(dot)com> writes:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)ymail(dot)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(at)ymail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)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(at)gmail(dot)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 AS
SELECT (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(at)ymail(dot)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(at)gmail(dot)com> writes:
> >> On Tue, Mar 10, 2015 at 12:45 PM, David G. Johnston
> >> <david(dot)g(dot)johnston(at)gmail(dot)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(at)momjian(dot)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>
| <interval value 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>
| <subtype treatment>
| <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>
| <unique predicate>
| <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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)com> writes:
> On Sun, Aug 09, 2015 at 07:16:02PM -0400, Tom Lane wrote:
>> Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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(at)leadboat(dot)com>:

> On Sun, Aug 09, 2015 at 08:06:11PM -0400, Tom Lane wrote:
> > Noah Misch <noah(at)leadboat(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

On 10 August 2015 at 16:31, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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