Alias locking updates

[beikov]

In Hibernate 5 a query like select a.value from Entity a will never apply an alias lock for a to the SQL. The logic in 5 only applies the locking to the SQL if the entity is actually selected e.g. select a.value, a from Entity a or select a from Entity a which is kind of limiting.

In 6 we could fix this and apply the locks. There is one backwards compatibility concern though. Since no database supports applying locks if an aggregate function is involved, a query like select sum(a.value) from Entity a could suddenly fail if it uses an alias lock for a. In 5, the lock mode is just ignored, but for 6 we would render it through and the database will complain.

Steve and I were discussing this a bit and we generally agree that this is a "fix" rather than a possible regression, but wanted to start a discussion so that others can participate as well.

Any thoughts on this topic?

[gavinking]

There is one backwards compatibility concern though. Since no database supports applying locks if an aggregate function is involved, a query like select sum(a.value) from Entity a could suddenly fail if it uses an alias lock for a. In 5, the lock mode is just ignored, but for 6 we would render it through and the database will complain.

I think this is fine, as long as we detect and produce a meaningful error ourselves, instead of just passing it through to the database and getting a nasty error that looks like we have a bug.

[beikov]

The error on DB2 is The FOR UPDATE clause is not allowed because the table specified by the cursor cannot be modified.. which is kind of meaningful. The documentation for the SQLState for DB2 is better though.

Locking on a query that involves aggregates is pretty rare I'd guess. Nobody complained so far that we for update was not applied to the SQL (it would fail on the DB anyway), so I assume this is just an edge case that we need to handle for completeness sake. I agree with your reasoning, but in this particular case I don't think it's worth it. The error we could provide is not necessarily better and would require a check that causes overhead for all queries.

[gavinking]

which is kind of meaningful.

It doesn't identify for the user what they did wrong in terms of their usage of the Hibernate APIs / query language.

Good error reporting means identifying for the user what they did wrong.

I assume this is just an edge case that we need to handle for completeness sake.

Having good error reporting and good usability is all about how you handle the edge cases. That something is an edge case isn't an excuse to just throw up your hands and say: "oh whatever, the user was doing something crazy, they deserve the unhelpful error they're going to get". Usability is about carefully identifying the edge cases and making sure you handle them elegantly.

would require a check that causes overhead for all queries.

It would require a negligible check only for queries executed with setLockMode(), which you've just said are not the common case.

[beikov]

Good error reporting means identifying for the user what they did wrong.

How about we parse the error message/code instead and give the user a more appropriate message?

It would require a negligible check only for queries executed with setLockMode(), which you've just said are not the common case.

Right, there certainly is some way to do it but as always, this comes at some cost. I'd prefer to parse the error message/code to improve the error message if that is ok for you.

[gavinking]

Well, I mean, I guess in a sense I don't really care at all precisely how you implement it. The important thing is the information carried by the error.

How about we parse the error message/code instead

But this just seems to me like a way harder way to solve the problem. You want to implement/test this for every single Dialect we support rather than just doing a really pretty trivial upfront check of something that we know for sure is wrong? I don't see why: it's more code to write, and much more fragile code at that.

[beikov]

You are right. I'll see what I can come up with.

[beikov]

I added the checks as part of the following PR, so I'll lock and close this discussion: #4009