Postgres code generation is wrong for -> operator (HSTORE column access) pre-psql-9.5

[lincolnq]

On sqlalchemy 1.0.15, the below test code outputs

(test -> %(test_1)s) IS NOT NULL

On sqlalchemy 1.1.0b3, it outputs

test -> %(test_1)s IS NOT NULL

Note the omitted parentheses.

The latter code is correct in Postgres 9.5 and above where the IS NOT NULL operator has lower precedence than ->, but below 9.5, the IS NOT NULL operator's precedence is higher than the -> operator. Thus, SQL fails to compile the expression because -> can't take a boolean on the rhs.

Here is the test case:

from sqlalchemy import Column
from sqlalchemy.dialects import postgresql

col = Column(postgresql.HSTORE, name='test', primary_key=True)
print(str((col['key'] != None).compile(dialect=postgresql.dialect())))

[lincolnq]

I noticed that commit a80bb4e5 changed the precedence of -> from 5 to 15, which seems like it would explain the problem.

[Mike Bayer]

If I change it to 5 I get this:

        self.assert_compile(
            col[col2 + 8],
            "x -> (y + :y_1)",
            checkparams={'y_1': 8}
        )

fails with:

x -> y + :y_1

which is a disaster.

I don't have a solution at the moment and I have a great fear that the precedence rules here are not backend-agnostic, meaning the whole refactor would have to be done again.

[Mike Bayer]

uh but if I change precedence to '5' your test fails anyway. so...that's not the problem.

[Mike Bayer]

oh, because HSTORE is separate.

[Mike Bayer]

proposed fix at https://gerrit.sqlalchemy.org/197

[lincolnq]

I think your fix is probably the right direction -- Sqlalchemy should output extra parentheses for safety and this seems like the right way to do it (though I don't know the code at all so I can't comment on most of it).

This bug's existence implies a whole class of similar operator-precedence bugs on old Postgres (many having nothing to do with HSTORE/JSON) -- it looks like you only implemented the change for HSTORE and JSON, right? It seems like it would be worth making sure all operators which changed precedence between these two versions have the eager_grouping enabled.

https://www.postgresql.org/docs/9.4/static/sql-syntax-lexical.html#SQL-PRECEDENCE-TABLE https://www.postgresql.org/docs/9.5/static/sql-syntax-lexical.html#SQL-PRECEDENCE-TABLE

[Mike Bayer]

it looks like they moved the precedence of "IS / IS NOT" to be lower. The use case of "a <operator> b <IS/ISNOT> (NULL/true/false)" is not that common outside of the json/hstore index operations, and in any case the issue here is a regression from 1.0.x specific to index operations. The solution I took here was just to hardwire extra parenthesization into these operators, the reason we are conservative about parenthesis otherwise is that you if you aren't you end up with things like "((a + b) / c) + ((((e + f) + g) + h ) + i)" which is difficult to work with and is often rejected by the database itself if the expression is in a less common place (not Postgresql, more like SQL Server, Oracle, SQLite etc. have had issues with unnecessary parenthesis causing syntax errors).

we've not had any issues reported from the change in precedence of IS/ISNOT from 9.4 to 9.5 across existing SQLAlchemy releases, those can be dealt with as they are reported.

[Mike Bayer]

Add "eager_parenthesis" late-compilation rule, use w/ PG JSON/HSTORE

Added compiler-level flags used by Postgresql to place additional parenthesis than would normally be generated by precedence rules around operations involving JSON, HSTORE indexing operators as well as within their operands since it has been observed that Postgresql's precedence rules for at least the HSTORE indexing operator is not consistent between 9.4 and 9.5.

Fixes: #3806 Change-Id: I5899677b330595264543b055abd54f3c76bfabf2

→ <<cset 333414fe9494>>