RFC on PR: Destination Locking
16 views
Skip to first unread message
David E. Wheeler
Sep 12, 2021, 7:12:22 PM9/12/21
to Sqitch Users
Hello fellow Sqitchies,
I’ve been working on adding support for advisory locking to the engine, with implementations for MySQL and Postgres. See #589 for the details; I’d very much appreciate your thoughts here and/or review of the PR.
https://github.com/sqitchers/sqitch/pull/589
The way it works is by requiring engines to implement two methods (or else not to support locking):
* `try_lock`, which will try to create an exclusive lock and immediately return on success or failure
* `wait_lock`, which will try to create an exclusive lock and wait indefinitely to acquire it
Both methods should create exactly the same lock, statically defined by the engines, so that no other instance of Sqitch can be making changes to the database at the same time. The new `lock_destination` method is called at the start of both `deploy` and `revert`. It calls `try_lock`, and if it succeeds, returns, but if it doesn't, it emits a message about its inability to get the lock, then calls `wait_lock` to wait for it. The new `lock_timeout` attribute controls the wait timeout (60s by default), and `lock_destination` handles the timeout by raising an error, which will cause Sqitch to exit.
New tests in `t/engine.t` cover the behavior of `lock_destination`, while new testing and configuration tests engine-specific implementations in `t/lib/DBIEngineTest`.
I'd appreciate a close reading of this PR, as there are a few things we should consider. In particular:
* Do we really want this enabled by default? Or should we add configuration/options to turn it on? Or perhaps configuration/options to turn it off?
* Is using Sys::SigAction sufficient to handle the timeout, including on non-Unix hosts (Windows)? Or is it likely to run into issues/conflicts with the database? Might some database locking carry on even when the timeout kicks in? (I suspect it does, since I had to add code to repeatedly clear locks to the tests, though in practice it should not be an issue, since Sqitch will exit, not hang around holding onto locks unnecessarily. But MySQL in particular is weird).
Thoughts?
Thanks,
David
I’ve been working on adding support for advisory locking to the engine, with implementations for MySQL and Postgres. See #589 for the details; I’d very much appreciate your thoughts here and/or review of the PR.
https://github.com/sqitchers/sqitch/pull/589
The way it works is by requiring engines to implement two methods (or else not to support locking):
* `try_lock`, which will try to create an exclusive lock and immediately return on success or failure
* `wait_lock`, which will try to create an exclusive lock and wait indefinitely to acquire it
Both methods should create exactly the same lock, statically defined by the engines, so that no other instance of Sqitch can be making changes to the database at the same time. The new `lock_destination` method is called at the start of both `deploy` and `revert`. It calls `try_lock`, and if it succeeds, returns, but if it doesn't, it emits a message about its inability to get the lock, then calls `wait_lock` to wait for it. The new `lock_timeout` attribute controls the wait timeout (60s by default), and `lock_destination` handles the timeout by raising an error, which will cause Sqitch to exit.
New tests in `t/engine.t` cover the behavior of `lock_destination`, while new testing and configuration tests engine-specific implementations in `t/lib/DBIEngineTest`.
I'd appreciate a close reading of this PR, as there are a few things we should consider. In particular:
* Do we really want this enabled by default? Or should we add configuration/options to turn it on? Or perhaps configuration/options to turn it off?
* Is using Sys::SigAction sufficient to handle the timeout, including on non-Unix hosts (Windows)? Or is it likely to run into issues/conflicts with the database? Might some database locking carry on even when the timeout kicks in? (I suspect it does, since I had to add code to repeatedly clear locks to the tests, though in practice it should not be an issue, since Sqitch will exit, not hang around holding onto locks unnecessarily. But MySQL in particular is weird).
Thoughts?
Thanks,
David
David E. Wheeler
Sep 12, 2021, 7:21:06 PM9/12/21
to Sqitch Users
On Sep 12, 2021, at 19:12, David E. Wheeler <da...@justatheory.com> wrote:
> I’ve been working on adding support for advisory locking to the engine, with implementations for MySQL and Postgres. See #589 for the details; I’d very much appreciate your thoughts here and/or review of the PR.
>
> https://github.com/sqitchers/sqitch/pull/589
Oh, and if anyone wants to add locking support to the other engines, LMK what the syntax should be. I saw that Oracle has a `DBMS_LOCK.REQUEST` package that looks useful, but I have no idea how to use it in plain SQL. I think SQLite has only transactional locking, though, now that I think fit, since it’s not a server, maybe we could use flock. I have no idea about the other engines, however. If you know how to create advisory locks for any of these, let me know:
> I’ve been working on adding support for advisory locking to the engine, with implementations for MySQL and Postgres. See #589 for the details; I’d very much appreciate your thoughts here and/or review of the PR.
>
> https://github.com/sqitchers/sqitch/pull/589
* Exasol
* Firebird
* Oracle
* Snowflake
* Vertica
Best,
David
David E. Wheeler
Sep 19, 2021, 5:46:53 PM9/19/21
to Sqitch Users
Hello Sqitchies,
Thanks,
David
> I'd appreciate a close reading of this PR, as there are a few things we should consider. In particular:
>
> * Do we really want this enabled by default? Or should we add configuration/options to turn it on? Or perhaps configuration/options to turn it off?
>
> * Is using Sys::SigAction sufficient to handle the timeout, including on non-Unix hosts (Windows)? Or is it likely to run into issues/conflicts with the database? Might some database locking carry on even when the timeout kicks in? (I suspect it does, since I had to add code to repeatedly clear locks to the tests, though in practice it should not be an issue, since Sqitch will exit, not hang around holding onto locks unnecessarily. But MySQL in particular is weird).
Anyone got some neurons to apply to these questions this week? I’m thinking I’d like to get this finalized next weekend and start working on releasing v1.2.0.
>
> * Do we really want this enabled by default? Or should we add configuration/options to turn it on? Or perhaps configuration/options to turn it off?
>
> * Is using Sys::SigAction sufficient to handle the timeout, including on non-Unix hosts (Windows)? Or is it likely to run into issues/conflicts with the database? Might some database locking carry on even when the timeout kicks in? (I suspect it does, since I had to add code to repeatedly clear locks to the tests, though in practice it should not be an issue, since Sqitch will exit, not hang around holding onto locks unnecessarily. But MySQL in particular is weird).
Thanks,
David
David E. Wheeler
Sep 26, 2021, 6:54:54 PM9/26/21
to Sqitch Users
On Sep 19, 2021, at 17:46, David E. Wheeler <da...@justatheory.com> wrote:
> Anyone got some neurons to apply to these questions this week? I’m thinking I’d like to get this finalized next weekend and start working on releasing v1.2.0.
No reviews, but I’m pretty comfortable with it. If I made a prerelease would anyone here be able to stress test it?
> Anyone got some neurons to apply to these questions this week? I’m thinking I’d like to get this finalized next weekend and start working on releasing v1.2.0.
In the meantime, I’ve also opened PR-591 to add --lock-timeout options to the deploy, revert, rebase, and checkout commands, so folx can override the default 60s timeout. Not too tricky, but a quick review would nonetheless be appreciated.
https://github.com/sqitchers/sqitch/pull/591
Thanks,
David
Reply all
Reply to author
Forward
0 new messages