Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support lock-and-fetch for MSSQL. Explicit LIMIT for all. #371

Merged
merged 36 commits into from
Oct 25, 2023

Conversation

kagkarlsson
Copy link
Owner

@kagkarlsson kagkarlsson commented Apr 17, 2023

  • Detect databases Oracle, MariaDB, Mysql >= 8, Mysql < 8 and add specific JdbcCustomizations. (Groundwork necessary to support SELECT FOR UPDATE .. SKIP LOCKED and explicit LIMIT for different databases.)
  • Refactor polling-logic to support custom polling-queries and limits from JdbcCustomization
  • Adding support for lock-and-fetch aka. SELECT FOR UPDATE .. SKIP LOCKED for MSSQL/SqlServer. However, testing has shown that lock-and-fetch is prone to deadlocks for SqlServer, so it is not recommended until that is understood/resolved (reproduce via disabled test in MssqlClusterTest). A silver-lining is that a query-hint was also added to fetch-and-lock-on-execute for SqlServer, and it appears this resolves issues with deadlocks for that (default) strategy.
  • Updates jdbc-handling dependency micro-jdbc
    • Support running multiple statements in a transaction for JdbcRunner and refactor transaction-handling (see PR #4)
    • Add support for batch-updates (see PR #5)
  • Shading micro-jdbc dependency
  • Explicit limit for SqlServer, MySQL, Oracle
  • Adds ClusterTest for SqlServer (test concurrency)
  • Adds (or re-enables) compatibility-tests for MariaDB, Oracle, Mysql >= 8, Mysql < 8, PostgreSQL generic lock-and-fetch

Fixes

Further work / fix later

  • Mysql v8 skip locked syntax
  • MariaDB skip locked syntax
  • Oracle skip locked syntax

Reminders

  • Added/ran automated tests
  • Update README and/or examples

cc @kagkarlsson

@kagkarlsson
Copy link
Owner Author

Remaining to test against MSSQL. Verified to work with postgres using new generic method that is transaction-based.

@kagkarlsson kagkarlsson changed the title Upgrade to micro-jdbc supporting transactions for JdbcRunner Support lock-and-fetch for MSSQL Apr 21, 2023
@kagkarlsson kagkarlsson marked this pull request as ready for review April 21, 2023 06:55
@kagkarlsson
Copy link
Owner Author

This is a bit tricky to test since Testcontainers does not seem to work on M1 Macs and testing via Github Actions sporadically fails. I have done some local testing using AzureSQLEdge, and the lock-and-fetch strategy seem to be working. If anyone using MSSQL could try this branch out that would be helpful.

With a little luck this might fix the deadlock issues experienced using the default polling strategy. I have gotten the deadlock locally (AzureSQLEdge) using default polling, but not using lock-and-fetch.

Is it possible for you do help out verifying this change for MSSQL @rafaelhofmann ?

This is the query currently in use:

    @Override
    public String createGenericSelectForUpdateQuery(String tableName, int limit, String requiredAndCondition) {
        return "select TOP "+limit+" * from " + tableName +
            " WITH (readpast,rowlock) where picked = ? and execution_time <= ? " + requiredAndCondition +
            " order by execution_time asc ";
    }

@kagkarlsson
Copy link
Owner Author

I added with (readpast) to the regular query as well for mssql (locally), and now I do not get the dead-lock issues @rafaelhofmann

@rafaelhofmann
Copy link

Hi @kagkarlsson

I will test this new branch and check if we still have problems with the indexes.

@kagkarlsson
Copy link
Owner Author

I have enabled both polling-strategies for MSSQL in this branch. Both seem to run successfully on GA via MssqlCompatibilityTest

Additionally, I added hints to skip locked to default-query as well since deadlocks were causing sporadic test-failures (for MSSQL)

@rafaelhofmann
Copy link

Hi @kagkarlsson,

Do you have a published snapshot version that we can use to test or should we build our own version?

Thanks

@kagkarlsson
Copy link
Owner Author

Currently you would need to build your own. Let me know if that is a problem.

@rafaelhofmann
Copy link

I have built the branch and upgraded our ACPT system (including all the indexes that were deleted in the past) to the new release, but we are running into the same problem as before: A deadlock about once every hour, when we have a load of 300 tasks/min.
I have been experimenting with a query that combines the SELECT and UPDATE in one, but I am not quite happy yet with the query hint for MSSQL:

String selectForUpdateQuery =
            " UPDATE " + ctx.tableName +
                " SET " + ctx.tableName + ".picked = ?, " +
                "     " + ctx.tableName + ".picked_by = ?, " +
                "     " + ctx.tableName + ".last_heartbeat = ?, " +
                "     " + ctx.tableName + ".version = " + ctx.tableName + ".version + 1 " +
                " OUTPUT [inserted].* " +
                " FROM ( " +
                "   SELECT TOP(?) ist2.task_name, ist2.task_instance " +
                "   FROM " + ctx.tableName + " ist2 WITH (ROWLOCK, READPAST) " +
                "   WHERE ist2.picked = ? AND ist2.execution_time <= ? " + unresolvedFilter.andCondition() +
                "   ORDER BY ist2.execution_time ASC " +
                " ) AS st2 " +
                " WHERE st2.task_name = " + ctx.tableName + ".task_name " +
                "   AND st2.task_instance = " + ctx.tableName + ".task_instance";

In theory this should do the same as the lock-and-fetch query from PostgreSQL.

@kagkarlsson
Copy link
Owner Author

kagkarlsson commented May 2, 2023

Ok, thanks. Did you try both lock-and-fetch and fetch-and-lock-on-execute? I think they should both work for MSSQL now. (lock-and-fetch for MSSQL is select-for-update and update spanned by a transaction)

@kagkarlsson
Copy link
Owner Author

Your combined query looks interesting

@rafaelhofmann
Copy link

We had deadlocks with the lock-and-fetch strategy, but so far no errors with the default fetch strategy after four hours of running. Will keep this running for a few more days to verify.

@kagkarlsson
Copy link
Owner Author

How has the fetch strategy run?

@kagkarlsson kagkarlsson changed the title feat: Support lock-and-fetch for MSSQL feat: Support lock-and-fetch for MSSQL. Explicit LIMIT for all. Sep 7, 2023
@kagkarlsson kagkarlsson mentioned this pull request Sep 15, 2023
1 task
@kagkarlsson kagkarlsson merged commit 31c4bab into master Oct 25, 2023
16 checks passed
@kgunnerud
Copy link

Great fix! Any ETA on release with this fix?

@kagkarlsson
Copy link
Owner Author

Good question, nothing blocking it from being released, just time I guess :). Hopefully I can release it within 1-2 weeks

@kagkarlsson
Copy link
Owner Author

🎉 This issue has been resolved in v13.0.0 (Release Notes)

@kagkarlsson kagkarlsson added the released Issue has been released label Dec 6, 2023
@ChristianCiach
Copy link

ChristianCiach commented Feb 6, 2024

Hi! I am currently evaluating db-scheduler for our use-cases and doing some code reviews in the process.

I see that the MariaDBJdbcCustomization, now uses FOR UPDATE SKIP LOCKED for SELECT-queries even though MariaDB only supports SKIP LOCKED since MariaDB 10.6, according to the documentation: https://mariadb.com/kb/en/select/#skip-locked

I don't particularly care about older versions of MariaDB, but I just want to let you know that the versions 10.4 and 10.5 are technically still supported, according to https://mariadb.com/kb/en/mariadb-server-release-dates/. I see you already differentiate the versions of MySQL, so maybe you want to do the same for MariaDB.

Anyway, thank you for your work! I like the simplicity of db-scheduler and I hope we can use it to eventually replace Quartz, which seems to be completely dead by now.

@kagkarlsson
Copy link
Owner Author

I had some issue when testing the support in MySQL (or MariaDB) and limited time, so I opted for disabling the support until someone had the time to verify it works as expected. Feel free to fork and try it out.

  @Override
  public boolean supportsGenericLockAndFetch() {
    // FIXLATER: fix syntax and enable it for versions of MariaDB that supports it
    return false;
  }

felixscheinost added a commit to felixscheinost/db-scheduler that referenced this pull request Sep 18, 2024
The support was nearly finished in kagkarlsson#371 but later commented out due to "some issue [..] and limited time".

I tried to find out what those issues were and discovered that the syntax of the select was wrong. In MySQL the `LIMIT` comes before the `FOR UPDATE SKIP LOCKED`.

Using that fix `Mysql8CompatibilityTest` passes, I have done no further testing yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants