Add with_for_update mysql new functionalities

[RobotScribe]

#4860 # Description
Add nowait, skip_lock, of arguments to for_update_clause for mysql

Checklist

This pull request is:

• A documentation / typographical error fix
  • Good to go, no issue or tests are needed
• A short code fix
  • please include the issue number, and create an issue if none exists, which
    must include a complete example of the issue. one line code fixes without an
    issue and demonstration will not be accepted.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests. one line code fixes without tests will not be accepted.
• A new feature implementation
  • please include the issue number, and create an issue if none exists, which must
    include a complete example of how the feature would look.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests.

Have a nice day!

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision cc067d9 of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

New Gerrit review created for change cc067d9: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 2ba1617 of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

Patchset 2ba1617 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Federico Caselli (CaselIT) wrote:

Code-Review+1 Workflow+1

(1 comment)

Looks good.
Can you also add a changelog entry, like described here? https://github.com/sqlalchemy/sqlalchemy/blob/master/doc/build/changelog/README.txt

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Federico Caselli (CaselIT) wrote:

Not sure if we should check here if the db support this?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[RobotScribe]

I don't understand what you mean.
Mysql should always support this ?

[CaselIT]

I meant version before the 8, from the issue it seems to be a new feature of version 8.
Also does mariadb support this? Currently they share this dialect

[RobotScribe]

Ah ok I understand now.
I think the comportment should be the same for all sgbd. For postgresql the version of the db is not checked. Do you agree ?
For mariadb I didn't know they share the same dialect. I will check maria db support it

[CaselIT]

I guess that there is no check in pg since it support from before version 7.1 (the oldest one that has docs on the website).
But I agree that there is not a lot of consistency on what is checked and what it is not checked in general.

I'll leave the decision to mike.

Regarding mariadb, I guess it could be made more prominent in the documentation that the mysql dialect is shared by both

[RobotScribe]

You're right SKIP LOCKED and OF are not implemented for mariadb https://falseisnotnull.wordpress.com/2018/04/23/mysql-vs-mariadb-wait-nowait-skip-locked/
I change that

[CaselIT]

let me know once you have updated it so that I can rerun the tests on gerrit

[RobotScribe]

I added a commit. I will check how to add a changelog

[jvanasco]

I just checked some db driver requirements for postgres.

psycopg2 - the minimum version supported is 7.4 (https://www.psycopg.org/docs/install.html#prerequisites)
pg8000 - minimum support is 9.4; this seems to be the active source - https://github.com/tlocke/pg8000
psycopg2cffi - doesn't seem to show any targeted versions
pypostgresql - can't find the minimum supported; this seems to be the active repo now https://github.com/python-postgres/fe via https://wiki.postgresql.org/wiki/Python
PyGresql - minimum PostgreSQL 9.0 (http://www.pygresql.org/)
jdbc - mimimum is 8.2 [see changelog v 42.0.0 https://jdbc.postgresql.org/documentation/changelog.html#version_42.0.0 )

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 490e822 of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

Patchset 490e822 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[gordthompson]

I don't see an Issue associated with this PR, so there is nothing to link to in the changelog.

[RobotScribe]

I don't see an Issue associated with this PR, so there is nothing to link to in the changelog.

The associated issue is the #4860. But may be I didn't link the issue properly. Is there something I forget ?

[gordthompson]

I don't see an Issue associated with this PR, so there is nothing to link to in the changelog.

The associated issue is the #4860. But may be I didn't link the issue properly. Is there something I forget ?

The convention is to include the issue number on the third line of the commit message:

Implement enhanced frobulation

Fixes: #4860

This update implements recent advances in frobulation algorithms ...

[sqla-tester]

Federico Caselli (CaselIT) wrote:

Code-Review+1 Workflow+1

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Code-Review+1 Workflow+1

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Code-Review+1 Workflow+1

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review-1

(1 comment)

needs to be altered to test for MySQL server version especially that it is checking for mysql vs. mariadb.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

mike bayer (zzzeek) wrote:

this code checks for "mysql" so that if someone specifies "of" or "skip locked", the code will still invoke successfully on MariaDB which we assume here does not support these features.

however, if the same code is run on a MySQL version prior to 8, now it fails and will be a regression if current applications are using these flags.

why allow silent "success" on mariadb but explicit raise on MySQL < 8 ? this seems inconsistent to me. this likely should be testing MySQL server version as well and especially if we want this in 1.3.x.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Will work on testing MySQL server version

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

(2 comments)

Multiple dialect objects interfere with checking the server version.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

This test fails because the server_version_info is None. Debugging shows that on first(?) connect __get_server_version_info() is called and the return value is saved to server_version_info for a particular dialect object (e.g., <sqlalchemy.dialects.mysql.mysqldb.MySQLDialect_mysqldb object at 0x0000007BD0A7FD48>). However, when this code runs, self.dialect is a different dialect object (e.g., <sqlalchemy.dialects.mysql.mysqldb.MySQLDialect_mysqldb object at 0x0000007BD2127308>) which has clobbered the server_version_info value.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Federico Caselli (CaselIT) wrote:

I think you need to set the sets as backend=True and connect fist at least one, so that the dialect initialization happens.
I don't know it there is a way of mocking the server version in the test.
It think it would be useful

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[CaselIT]

I meant tests, not sets

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Sorry for the confusion caused by my incorrect wording. Some tests are failing, but they are failing because of this code in the mysql dialect itself. And like I said, the PyCharm debugger shows that the correct server version values are being retrieved (SELECT VERSION()) and inserted into some dialect object, but not the same dialect object that is being referenced here.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

mike bayer (zzzeek) wrote:

lets give the MySQL dialect a flag:

class MySQLDialect(...)

...

supports_for_update_of = False

compiler only checks self.dialect.supports_for_update_of.

MySQL dialect when it does on_connect (sorry forgot event name), has server version, can update this flag.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

(1 comment)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Federico Caselli (CaselIT) wrote:

(1 comment)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

(1 comment)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Code-Review-1 Workflow-1

On hold pending resolution of Gerrit_1913

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Patch Set 2: Code-Review-1 Workflow-1

On hold pending resolution of Gerrit_1913

Sorry, make that Gerrit_1928.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

mike bayer (zzzeek) wrote:

(1 comment)

we should keep the server version info part of this in the dialect and have the compiler consult a simpler flag like supports_for_update_of which will resolve the issue of dialects that have no database connectivity (which is normal and common).

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Code-Review+1 Workflow+1

(1 comment)

If this looks okay then I can do the backport for 1.3 (1936) again.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+1

i want to do 1928 and this at the same time, so im bumping it? no other issue right

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

mike bayer (zzzeek) wrote:

-Code-Review -Workflow

oh ok we want to backport more things, got it. ill review _ merge the master branch

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review-1

(4 comments)

very nice. needs some fixup on the mysql compile tests.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

• test/dialect/mysql/test_for_update.py (line 169): it would be best if this were broken into one test per statement. test_for_update_one(), test_for_update_two(), test_for_update_of_one(), etc.
• test/dialect/mysql/test_for_update.py (line 172): so this suite if it's doing assertscompiledsql doesnt actually need to do anything with the "config.db". so remove backend , only_on, and requires....
• test/dialect/mysql/test_for_update.py (line 230): then here, instead of connection.dialect, make a mysql.dialect() that has server version = (8, 0). we would only be using testing.requires if these tests were actually running the SQL for real to the database which does not seem to be the case here.
• test/requirements.py (line 1621): oh we want to cherry pick this back to the 1.3, OK

[sqla-tester]

Gord Thompson (gordthompson) wrote:

(4 comments)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

• test/dialect/mysql/test_for_update.py (line 169): Done
• test/dialect/mysql/test_for_update.py (line 172): Done
• test/dialect/mysql/test_for_update.py (line 230): That didn't work, but forcing the .supports_for_update_of property did.
• test/requirements.py (line 1621): Ack

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review-1

(2 comments)

this is great, let's set dialect = mysql.dialect() on that suite as it otherwise fails when config.db is not mysql.

it's probably difficult to follow how asserts_compile is choosing the dialect, it's all right here: https://github.com/sqlalchemy/sqlalchemy/blob/master/lib/sqlalchemy/testing/assertions.py#L360 so config.db is taken into account if nothing else has specified what the "dialect" should be.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

• test/dialect/mysql/test_for_update.py (line 176): looks great

i think the test failures, we need to also set the default dialect:

dialect = mysql.dialect()

• test/dialect/mysql/test_for_update.py (line 278): yeah this is great

[sqla-tester]

Gord Thompson (gordthompson) wrote:

(2 comments)

Thanks!

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

• test/dialect/mysql/test_for_update.py (line 176): Done
• test/dialect/mysql/test_for_update.py (line 278): Ack

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Code-Review+1 Workflow+2

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+2

perfect. we are ready to update the backport.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1928 has been merged. Congratulations! :)

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Code-Review+1 Workflow+2

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+2

I'm assuming this is a clean cherry-pick from master at this point. I like to do both at the same time since I worry about those changelog files under unreleased_13 only getting merged in master and not 1.3.

thanks for the hard work as always!

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936 has been merged. Congratulations! :)

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Patch Set 5: Code-Review+2 Workflow+2

I'm assuming this is a clean cherry-pick from master at this point.

Yup. I ran pick_to.sh on 103260d (Gerrit 1928) and then fixed the merge conflicts.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/1936

[ashb]

I'm not sure what the desired approach here is, but this produces invalid SQL for mysql 5.7 when you do .with_for_update(skip_locked=True).

The fix is relatively easy if the "correct" behaviour (which is what I was hoping for) is to ignore the arg:

-        if select._for_update_arg.skip_locked and self.dialect._is_mysql:
+        if select._for_update_arg.skip_locked and self.dialect.supports_for_update_of:
            tmp += " SKIP LOCKED"

[CaselIT]

@ashb could you please open an issue so this can be better tracked?

Thanks!