Fixes #224. I believe this is a Postgres only issue

[hms]

Per all of my earlier discussion, Postgres transactions become invalid upon any database contraint invalidating an operation and make all follow on database interactions invalid until a rollback is executed.

Semephore#attempt_creation uses a rather common database pattern to rely on database locks to reliably syncrhronize based on uniqueness of values. The catch is that Postgresql requires a little more handholding to use this pattern. By wrapping just the Semaphore.create statement in a SAVEPOINT (a pretend nested transaction), we have what appears to be a transparent operation that works across all of the other supported databases without any known consequences AND allows Postgres to continue to work as expected per the existing code.

This change works across the Solid supported databases (at least based on the tests) and I believe the extra overhead for the non-postgres databases is small enough that special casing the code or resorting to the complexity of dependency injection is just not worth it.

I added two tests to the concurrency_controls_test.rb. One to show the error is real and is easy to reproduce. The other to show the fix fixes it.

[hms]

@rosa

I'm pausing on this for just a little as I did a little more digging (probably should have checked before coding a fix....) and there are some significant concerns around adding sub-transactions / savepoint for high volume deployments, especially those with replication / failover.

While my proposed solution is database agnostic (ignoring the Postgres performance issues referenced above), the tests cannot be. Both SqLite and mySQL work without a testable failure with either my PR or the original SolidQueue implementation.

There look to be some non-nested transaction / savepoint implementations, but they all require different syntax for each of the 3 supported databases. I'm open to investigating those alternatives, but it would help to understand if the SolidQueue team would be open to a PR based on database specific syntax and how to organize the code for any PRs with this approach.

Open issues that I could use some direction on:

• Does the SolidQueue team consider this a SolidQueue problem, a Rails upstream issue, or somewhere else I'm not looking at yet?
  • If the SolidQueue tam would prefer to defer to the upstream work that appears to be under consideration, is the team open to a temporary solution for the problem that does exist now?
  • If yes to a SolidQueue fix now, can I get some direction on the team's requirements for how to structure / organize / call out database specific implementations where and when necessary?

Thanks,

Hal

[hms]

@rosa

I've revamped this PR to avoid nested transaction.

Postgres, unlike mysql and sqlite, invalidates the current transaction on
a database contraint violation such as an attempt to insert
a duplicate key into a unique index.

SolidQueue uses a pretty standard design pattern of application level
conflict detection and resolution via error handling as part of it's
concurrency management in Semaphore::Proxy#attempt create (via create! and rescue).

For Postgres based implementations when:
* enqueing a job inside a transaction
* under load such that the race condition triggerng the simultanious conflicted insert

the transaction would become unrecoverable at the application level.

There are two simple paths to address this issue in Postgres:
* Nested transactions
* Insert using 'on conflict do nothing'

Nested transaction are the easiest to implement and are portable across all 3
of the Rails standard databases. Unfortunately, nested transactions have a
known and very signifant performance problem, especially for databases under
load and/or replicated databases with long running queries.

Each of the big three database inplements the ANSI standard Insert on conflict do nothing
in a very different way.

Because of this, this PR only replaces the current implementation of
application level conflict handling for Postgres.  It takes advantage
of the Rails standard ActiveRecord::Persistence::Insert method supporting on
conflict semantics.

[rosa]

Hey @hms, so sorry for the delay here! I've been totally focused on other stuff (and other projects) but I'm finally trying to wrap up everything for v1.0 and would like to include this fix. This last approach looks great to me. I have some suggestions for the implementation, but it's possible that, being so long ago, you no longer have time (or interest!) to implement these. In that case just let me know, happy to add these myself.

Thank you so much for figuring out this one 🙏

[hms]

@rosa

I'd love any feedback / suggestions to make the PR better and more durable for the project. I'm happy to get this wrapped up to releaseable standards.

Hal

[rosa]

Since we have TARGET_DB, I think I'd use that here, like here.

[hms]

That is better. I'll submit the change.

[rosa]

Hmm... actually, I'm not completely sure if we need this test at all, since it seems it's testing more something internal of Active Record, rather than something we have in Solid Queue 🤔

[hms]

While this really is more of a test of AR, its value is that we have a confirmation of outcomes with the two different ways to add records to the Semaphore table and we show the two mechanisms "side-by-side". I realized that it was missing an assert to meet that goal. See what you think with the extra line.

In the end, if you still feel strongly about cleaning this test up a little, I'll remove it.

[rosa]

Sounds good! I don't feel strongly about this one 😊

[rosa]

In this project I'm following the style of not having a space after private when the class has public and private methods, so this one was on purpose 😅

[rosa]

It seems there's a method that checks directly whether the adapter supports unique_by exposed in Active Record's Abstract adapter, that we could use simply as

connection.supports_insert_conflict_target?

I think this might be more robust than checking the adapter name, because other adapters for PostgreSQL might have a different name, like the PostGIS adapter.

[hms]

So funny and just goes to show a bit of tunnel vision. I spent a fair amount of time digging around for an alternative to the original nested transactions methodology and in that digging I came across this method in the Rails adaptors.

The tunnel vision part was no thinking to use those methods as the much, much better way to test which of the code paths to take.

Thanks for this -- it's a great reminder to slow down and think before coding.

[rosa]

I'm thinking that this all could perhaps live in the Semaphore class in a simpler way. Something like this method:

diff --git a/app/models/solid_queue/semaphore.rb b/app/models/solid_queue/semaphore.rb
index 34bf8dc..1448e67 100644
--- a/app/models/solid_queue/semaphore.rb
+++ b/app/models/solid_queue/semaphore.rb
@@ -17,6 +17,17 @@ module SolidQueue
       def signal_all(jobs)
         Proxy.signal_all(jobs)
       end
+
+
+      def create_unique_by(attributes)
+        if connection.supports_insert_conflict_target?
+          insert(attributes, unique_by: :key).any?
+        else
+          create!(attributes)
+        end
+      rescue ActiveRecord::RecordNotUnique
+        false
+      end
     end

     class Proxy

[rosa]

Then that could be used in Proxy class like this (BTW I might remove that class in the future and leave everything in Semaphore... it was not to have to pass all the job attributes around, but still... anyway, not the goal of this PR):

        def attempt_creation
          if Semaphore.create_unique_by(key: key, value: limit - 1, expires_at: expires_at)
            true
          else
            check_limit_or_decrement
          end
        end

        def check_limit_or_decrement
          if limit == 1 then false
          else
            attempt_decrement
          end
        end

And you wouldn't need to define aliases.

[hms]

@rosa

Thank you for your feedback and suggestions. The PR is significantly better because of them.

I have a PR ready. How do you feel about rebasing and squishing the older commits that, given the last set of changes, don't add much value from a history perspective? The only downside is I think your comments might get lost in that process and given how important they are to the finished PR, I'm not sure how I feel about that.

Hal

[rosa]

Hey @hms! Thanks a lot for that! 🙏 No need to squash the commits, totally as you prefer. Feel free to push things and keep the original history.

[hms]

@rosa I could use some help here. The tests run clean in my local environment over multiple runs.

Interestingly, at least part of the error output refers to the Postgres transactional error that was the root cause for this PR. I'm a RSpec person, so I'm not nearly as familiar with mini-test as I should be. Does it wrap tests in transactions? If so, then any test that hits a database managed constraint (unique, checks, foreign keys, etc) are all going to be at risk for trigging: PG::InFailedSqlTransaction: ERROR: current transaction is aborted, commands ignored until end of transaction block (ActiveRecord::StatementInvalid)

Hal

[rosa]

Ohhh yes! I think some of those failures are legit and not related to your changes. I've got a similar fix to what you did here in the works in 3d40b1a

[rosa]

Thanks again for this, @hms, amazing work 🙏 🙇