TalentBox / sequel-rails

A gem for using Sequel with Rails 5.x, 6.x, 7.x, 8.x
http://talentbox.github.io/sequel-rails/
MIT License
326 stars 81 forks source link

Threaded app without preload makes sequel drastically slow. #137

Closed gencer closed 7 years ago

gencer commented 7 years ago

I don't know if this is directly a sequel-rails issue but I have one problem.

You can see this code:


on_worker_boot do
  # Ensure we don't keep connections
  if defined?(Sequel)
    ::Sequel::Model.db.disconnect
    ::Sequel::DATABASES.each{|db| db.disconnect }
  end
end

after_worker_boot do
  SequelRails.setup Rails.env if defined?(SequelRails)
end

This code exists and you know why. When preload used for multiple workers, we need to disconnect them as stated at sequel docs. Problem is, I do not have preload feature. My puma config has 24 workers with 100 threads each without preload but enabled with prune_bundler.

From now on I will talk on basics:

I created simple ActiveRecord enabled Rails 5.1 project. Simply added one database and one model. I created one line code that returns a field from database and print it to screen. (text)

I did the same thing without ActiveRecord and with Sequel and SequelRails.

Now, AR version can handle 1500 concurrent requests and get data from database without any error. It handles my workers and threads successfully.

Sequel has not. I get 100reqs/s on first and 15reqs/s in second and 6reqs/s on third round (benchmark test). Something doesn't correct. I tried above code on_boot, before_fork. None of them worked. Most interesting thing is, I manually created a second database feature. (You know Rails has only one). Now sith Sequel app, I assign my models to this second database and tried to fetch data from it. Bingo! 3000 request in a second is handled.

Basically, primary connection either disconnected or connected or unnecessary checked by sequel, sequel-rails or whatever the reason.

I am pretty sure either sequel or this gem because same app with same content with ActiveRecord is just works. Sequel is not. (Except if I add as secondary db). So there is something done on primary.

Is this happens because of this gem or do you have any idea?

gencer commented 7 years ago

Okay, until I find a proper (or maybe this is the proper one?) way, I simply re-establish each connection on worker start. This way i am able to achieve same speed and concurrency.

Update:

Yes, if we not say preload_app! then we are alone and have to establish our connection after app is loaded. This could be via initializers or directly on (or abstracted) model.

JosephHalter commented 7 years ago

Yes, I don't think there's anything to change in sequel-rails here, if you close all connections manually without preload, it's better to open them manually too (for each worker) instead of relying on the queries to force a reconnection.

gencer commented 7 years ago

Thanks @JosephHalter. But it makes me wonder how ActiveRecord works on same conditions. Without preload_app! ActiveRecord is fast as with preload_app! I should open an issue on Sequel.

gencer commented 7 years ago

@JosephHalter, Ok. No issues. This is absolutely an expected behavior on Threaded apps who do not uses preload. (We need to disable preload for phased restarts)

So article here explains all. I just did the right thing. Now, Im at least confident.

Issue closed.

gencer commented 7 years ago

@JosephHalter, may I kindly get your attention here for a short time. Would you please explain me about max_connection/pool? This issue just disappear when I disable/remove max_connections parameter. Thats why my initializer works. Because initializer uses pool parameter which is sequel ignores it. But SequelRails convert it to max_connections.

So here are 2 things:

  1. Multi-process Multi-thread app and SequelRails uses max_connection globally. Not per worker.
  2. MaxConnection is not suitable for threaded apps.

All I can say, If i remove it from database.yml, it just works with threaded apps. But when I add it, single threaded works multi threaded not works.

JonathanTron commented 7 years ago

Hi @gencer,

as far as SequelRails is concerned, we're just passing the max_connections to the Sequel.connect method, which you can read more about on Sequel documentation page.

About the multi-process/multi-thread issue, there are different things:

  1. Multi-Process: each process should have it's own Pool of connection, that's why you have to disconnect/reconnect using your server hooks.
  2. Multi-Thread: inside each of your worker, the Pool is responsible to give threads accessing it a connection, if you have 100 threads and you ask for a pool of 100 connection (by setting the max_connections) then you will eventually open 100 connections to the database.

Now that this is a given, let's say you ask Puma for 10 workers each with 100 threads and you set max_connections = 100, you will eventually open 10 x 100 = 1000 which is really not a good idea.

gencer commented 7 years ago

I see. I was thought max_connection with threaded app should be the same number as thread count of puma.

I currently did (and failed the tests) as; 24 workers 100 threads each 100 max_connections

Now I fixed by this: 24 workers 100 threads each 4 (default) max_connection.

Seems it just runs fine. Am I get it right?

Thanks for the prompt explanation, btw. I just mixed processes with threads wrongly and insist on high number of max_connection. In your explanation, this shouldn't be done this way.

JonathanTron commented 7 years ago

Yes, you're right... it all depends on your workload and your db usage obviously, but it's usually better to have less connections than threads.

The number of connections gives you the maximum concurrency for requests accessing the database.