sqlalchemy: Mysql5.7 gets invalid syntax of `.with_for_update(skip_locked=True)`

this is not a v2 thing, this is all a right now thing.

1. all backends should be raising an error when a user behavioral intent that cannot be fulfilled is received by a dialect, so first off, the for_update_clause() in compiler.py should be looking at all of these for_update flags and raising, that is, this is wrong:

   def for_update_clause(self, select, **kw):
        return " FOR UPDATE"

if should be:

   def for_update_clause(self, select, **kw):
        if select._for_update_arg.read:
           raise exc.CompileError("read only for update is not supported by this backend")
       elif select._for_update_arg.of is not None:
           raise exc.CompileError("....")
      elif ... # etc.

     return " FOR UPDATE"
   

1. we should implement all of the above as warnings for 1.3, so that we have some degree of an option to turn them into hard exceptions in 1.4, but if they turn into execptions in 2.0, that’s fine. but we should be warning now, if we have the development resources to take this on.

2. the Compiler raises explicit compile errors for major SQL features that have no “default” implementation. E.g. “RETURNING”, the base “def returning_clause()” raises an error. There’s no “version detection” at this level, and the reason we don’t let the database try to throw an error is because there is not even a SQL standard thing we can do here.

3. For major SQL features that are 100% the SQL standard, like CTEs, the base Compiler implements them and assumes they exist. if the backend database doesnt support it, like MySQL, the database raises an error.

4. Now we’re in the realm of “a specific database”, like different versions of Postgresql, different variants of MySQL.

5a. in this realm, the per-dialect compilers should implement the supported SQL features for the latest and greatest version of that database.

5b. For those SQL features that won’t work on an older version, but an explicit fallback exists, the dialect / compiler should do the fallback. The example of MySQL dialect using SELECT @@transaction_isolation for mysql > 5.7.20 but SELECT @@tx_isolation for other versions is sort of an example, this is a place where we need to know the server version and we need to test it against a specific number.

5c. for those SQL features that wont work on an older version, but the average intent would be to omit the behavior on an older version and things should still be “fine”, we do version detection. This is an area that we should probably do much less of in favor of allowing for “variants”. Looking around I can see examples like we silently drop the “CONCURRENTLY” keyword when we create a PostgreSQL index that asked for “postgresql_concurrently”, the index still gets built but it will not allow concurrent activity on the table on an older version.

5d. for those SQL features that wont work on older version and they clearly indicate a behavior that is very different from what it would be without that feature, the dialect should as often as possible just render it out to the DB and let it fail. If rendering it out to the DB causes a very confusing / unclear error condition then sometimes we will use version detection, but we really should not in most cases.

5e. if the feature is more of a DBAPI level / Python side feature, then we usually have to do more with DBAPI version detection since when you use a Python API incorrectly it’s not really distinguishable from a bug.

The philosophy is: we really don’t want to be chasing down server_version_info and hardcoding server version infos any more than we have to. we do it a bunch but only as a last resort.

This can be documented right now if people want to work on it, it can be on the dialects page or on the “Engines” page. the problem is we are not very consistent in many areas right now including this FOR UPDATE area.

OK it’s too bad this was implemented in 1.3 as silent ignore for mariadb. assuming we agree “SKIP LOCKED” is not safe to degrade from.

that philosophy is not really laid down in any specific document, and if it were, nobody would ever see it because people are usually just looking at the reference docs for the function they want to use, I think the best approach is to add notes to the docstring for this specific method that some of these features are not supported, and which ones will not have a graceful fallback.

right so given this feature does not appear to be a safe silent degrade and there is no equivalent, this issue is IMO not a bug. as far as the “SQLAlchemy should raise an exception” critique, there’s already an exception raised by the database and we generally want the database to do as much of the work as possible.

it depends on if “SKIP LOCKED” represents an improved behavior from which can be safely degraded

That’s a much more succinct way of describing the point I was trying to make. It’s one thing to implement some high_level_SQLA_feature in a version-independent way (e.g., an “upsert” operation that “just works” with or without ON CONFLICT) vs. an explicit invocation of a specific backend feature. In the latter case I would prefer to take the approach that “you can ask, but the backend itself may say ‘no’ depending on what version it is”.