Skip to content

Change the prepared statement limit#256

Merged
pjaspers merged 3 commits into
devfrom
change-prepared-statement-limit
Oct 9, 2021
Merged

Change the prepared statement limit#256
pjaspers merged 3 commits into
devfrom
change-prepared-statement-limit

Conversation

@pjaspers
Copy link
Copy Markdown
Contributor

@pjaspers pjaspers commented Oct 4, 2021

New approach! What could happen is that when multiple projects are
generating invoices at the same time, we generate a lot of prepared
statements, which causes the OOM.

I checked the work_mem of our pg:

  select * from pg_settings where name='work_mem';
  32768kB

We have sidekiq running with a concurrency of 20, the default prepared
statement limit of Rails is 1000, so that could quickly become 20000
prepared statements that we keep in cache, and thus we might overload
our pg and get the dreaded OOM message.

This would also explain that when running them at another time, we
don't run into it.

Questions remaining:

  • Is this really the way to configure a DB on Heroku? (Looks like it)
  • Do we also want to run it like this on the web-dyno's? I think we
    can do a Sidekiq.server? like method to isolate the behavior.
    We can change after the fact, doesn't look to do any immediate harm.
  • I matched the default Rails configuration but want to lower it to
    something like 200. Which might make it slower but should make it
    more dependable. It's hard to tell without more logs from Heroku.
    It's just an env variable, so easily changed an adapted.

I ran the config.rake on production (by copy pasting into a repl):

In production, we currently have:

  `SIDEKIQ_CONCURRENCY` => 20
  `RAILS_MAX_THREADS`   => 5
  `WEB_CONCURRENCY`     => 1
  `DB_POOL`             => 1

This means that we need a pool size of:

  [SIDEKIQ_CONCURRENCY, MAX_THREADS*WEB_CONCURRENCY].max
  => [20, 5*1].max
  => 20

The current pool size is `5`.

Which makes sense, since we only change the pool size in the Sidekiq initializer. After this PR has been deployed I would expect the ActiveRecord::Base.connection_pool.size to be 20 and the ActiveRecord::Base.configurations[Rails.env] should contain our newly added settings.

When we hit the PG OOM errors, we should run select * from pg_prepared_statements, and this should normally show about sidekiq_workers*1000 items (thus eating our memory)

@pjaspers pjaspers requested a review from mestachs October 4, 2021 16:42
New approach! What could happen is that when multiple projects are
generating invoices at the same time, we generate a lot of prepared
statements, which causes the OOM.

I checked the work_mem of our pg:

      select * from pg_settings where name='work_mem';
      32768kB

We have sidekiq running with a concurrency of 20, the default prepared
statement limit of Rails is 1000, so that could quickly become 20000
prepared statements that we keep in cache, and thus we might overload
our pg and get the dreaded OOM message.

This would also explain that when running them at another time, we
don't run into it.

Questions remaining:

- Is this really the way to configure a DB on Heroku? (Looks like it)
- Do we also want to run it like this on the web-dyno's? I think we
  can do a `Sidekiq.server?` like method to isolate the behavior.
- I matched the default Rails configuration but want to lower it to
  something like 200. Which might make it slower but should make it
  more dependable. It's hard to tell without more logs from Heroku.

Let's not break CI

Make test env use passed in keys or defaults

And let's pass in the host

And just to be sure the port as well

Move all DB pool size to database.yml

I've also added a small rake task to output the right number, normally
the SIDEKIQ_CONCURRENCY will always be higher than the amount of web
workers, so no need to set the DB_POOL, but just in case.

noop. Offer pearls to the linter gods

noop. Remove the copy/pasted dot
@pjaspers pjaspers force-pushed the change-prepared-statement-limit branch from 1e1b19c to d1a7dff Compare October 8, 2021 14:23
@mestachs mestachs temporarily deployed to orbf2-change-prepared-s-lwhs6o October 8, 2021 14:26 Inactive
@pjaspers
Copy link
Copy Markdown
Contributor Author

pjaspers commented Oct 8, 2021

❯ heroku run rails console -a orbf2-change-prepared-s-lwhs6o 
Running rails console on ⬢ orbf2-change-prepared-s-lwhs6o... up, run.3351 (Free)
W, [2021-10-08T14:43:49.009760 #4]  WARN -- sentry: ** [Raven] You are running on Heroku but haven't enabled Dyno Metadata. For Sentry's release detection to work correctly, please run `heroku labs:enable runtime-dyno-metadata`
Loading production environment (Rails 5.2.5)
irb(main):001:0> ActiveRecord::Base.configurations[Rails.env]
=> {"pool"=>2, "statement_limit"=>1000, "adapter"=>"postgresql", "username"=>"nhptkyjazswhel", "password"=>"007113c26863a1ee7263b1b9ce449d17070d0f9518f94b10702551830a3e3360", "port"=>5432, "database"=>"d24a6e2tfdvsnm", "host"=>"ec2-34-251-245-108.eu-west-1.compute.amazonaws.com"}

So our statement_limit is coming through. But the pool size is 2 (because the sidekiq concurrency is set to 2 and that overrides the rails max threads) and that is wrong.

@mestachs mestachs temporarily deployed to orbf2-change-prepared-s-lwhs6o October 8, 2021 14:51 Inactive
@pjaspers
Copy link
Copy Markdown
Contributor Author

pjaspers commented Oct 8, 2021

And now it worked:

❯ heroku run rails config:check_db_pool -a orbf2-change-prepared-s-lwhs6o 
Running rails config:check_db_pool on ⬢ orbf2-change-prepared-s-lwhs6o... up, run.2670 (Free)
W, [2021-10-08T14:55:29.346915 #4]  WARN -- sentry: ** [Raven] You are running on Heroku but haven't enabled Dyno Metadata. For Sentry's release detection to work correctly, please run `heroku labs:enable runtime-dyno-metadata`

In production, we currently have:

  `SIDEKIQ_CONCURRENCY` => 2
  `RAILS_MAX_THREADS`   => 5
  `WEB_CONCURRENCY`     => 1
  `DB_POOL`             => 1

This means that we need a pool size of:


      [SIDEKIQ_CONCURRENCY, MAX_THREADS*WEB_CONCURRENCY].max
      => [2, 5*1].max
      => 5

The current pool size is `5`.

@mestachs mestachs temporarily deployed to orbf2-change-prepared-s-lwhs6o October 8, 2021 14:59 Inactive
@pjaspers pjaspers merged commit 2554da4 into dev Oct 9, 2021
@pjaspers pjaspers deleted the change-prepared-statement-limit branch October 9, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants