Reduce SQL calls when incrementing/decrementing run counters #881
Conversation
SQL logs before changes (using `#increment!`):
```sql
Bulkrax::ImporterRun Load (8.9ms) SELECT "bulkrax_importer_runs".* FROM "bulkrax_importer_runs" WHERE "bulkrax_importer_runs"."id" = $1 LIMIT $2 [["id", 1], ["LIMIT", 1]]
Bulkrax::ImporterRun Update All (2.5ms) UPDATE "bulkrax_importer_runs" SET "processed_records" = COALESCE("processed_records", 0) + 1 WHERE "bulkrax_importer_runs"."id" = $1 [["id", 1]]
```
SQL logs after changes (using `#increment_counter`):
```sql
Bulkrax::ImporterRun Update All (6.5ms) UPDATE "bulkrax_importer_runs" SET "processed_records" = COALESCE("processed_records", 0) + 1 WHERE "bulkrax_importer_runs"."id" = $1 [["id", 1]]
```
The `SELECT` statement serves no purpose since the `UPDATE` statement is
atomic
These columns (`processed_children` and `failed_children`) were renamed in the RenameChildrenCountersToRelationships migration; they no longer exist
| # rubocop:disable Rails/SkipsModelValidations | ||
| Bulkrax::ImporterRun.find(importer_run_id).increment!(:processed_relationships, number_of_successes) | ||
| # rubocop:enable Rails/SkipsModelValidations | ||
| ImporterRun.connection.execute(<<-SQL) |
There was a problem hiding this comment.
Should this call ImporterRun.increment_counter(:processed_relationships, importer_run_id, amount: number_of_successes) ?
I don't like the raw SQL so far from the model.
There was a problem hiding this comment.
Oops…the increment_counter is a Rails method. Hmm.
There was a problem hiding this comment.
Wondering about using the update_counters method: https://api.rubyonrails.org/classes/ActiveRecord/CounterCache/ClassMethods.html#method-i-update_counters
There was a problem hiding this comment.
#update_counters seems good to me. Same SQL, just gives you control over the amount, which is nice.
I'm curious, why do you not like raw SQL?
There was a problem hiding this comment.
does this raw sql work in mysql? all versions of postgres? msql? oracle db? sqlite? because the old version does and that's the reason we avoid raw sql whenever we can.
There was a problem hiding this comment.
Replaced the raw SQL with #update_counters and CI is still green so I think we're good!
| @@ -21,14 +21,14 @@ def perform(entry_id, importer_run_id) | |||
| entry.build | |||
| if entry.succeeded? | |||
| # rubocop:disable Rails/SkipsModelValidations | |||
| ImporterRun.find(importer_run_id).increment!(:processed_records) | |||
| ImporterRun.find(importer_run_id).increment!(:processed_file_sets) | |||
| ImporterRun.increment_counter(:processed_records, importer_run_id) | |||
* main: (24 commits) Retry and delete take 2 (#894) 🎁 Add `Bulkrax.persistence_adapter` (#895) 💸 Mint v6.0.1 (#892) 🐛 Fix #work_identifier_search_field logic (#891) 💸 Bump to v6.0.0 (#889) make search string used to look up objects configurable (#884) 💸 v5.5.0 (#888) unpin dry-monads. its not a dependency of bulkrax (#885) fix syntax error in ERB (#883) add support for Rails 6, Hyrax 4, and Blacklight 7 (#782) Reduce SQL calls when incrementing/decrementing run counters (#881) Update readme to remove references to samvera-labs (#880) add Compatibility section to readme (#879) 🐛 Fix tabs for Hydra application (#875) Nav-tabs event scoping (#874) 📚 Update docs in preparation for best practices seminar (#873) use the `GlobalID` library tooling to determine global id (#869) Avoid NoMethodError in Bulkrax::Importers::Controller#create. (#870) preparing to deploy v5.4.1 (#868) 5.4.0-bug-fixes (#865) ...
Story
SQL logs before changes (using
#increment!):SQL logs after changes (using
#increment_counter):The
SELECTstatement serves no purpose since theUPDATEstatement is atomic.I haven't done any benchmarks on these changes, but since less SQL statements seems like a pretty obvious win and since we update counters a ton, I'm hoping we'll see at least some speed improvements