Skip to content

Commit

Permalink
Add an index to better support smaller_number_is_higher_priority (#…
Browse files Browse the repository at this point in the history
…1213)

The preexisting index (introduced in #726) gave a direct answer to the
materialized subquery used to find candidate jobs, subject to the ordering rules
that were in place at that time.  In #883, `GoodJob` deprecated that ordering in
favour of a lower-is-more-important scheme, aligning with Active Job.

When there are a substantial number of completed jobs, the `priority desc` index
does not allow for a straight index scan, and may not actually be used by the
planner.  Aligning the sort orders here allows for the subquery to be satisfied
directly.

Although `ASC NULLS LAST` is the default (as observable in the generated schema)
it seems appropriate to be explicit in the actual index declaration, although
mostly for consistency with the previous version.
  • Loading branch information
mkrfowler committed Jan 19, 2024
1 parent b491e8f commit 6a7332a
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 2 deletions.
7 changes: 7 additions & 0 deletions app/models/good_job/base_execution.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ def active_job_id_index_removal_migrated?
migration_pending_warning!
false
end

def candidate_lookup_index_migrated?
return true if connection.index_name_exists?(:good_jobs, :index_good_job_jobs_for_candidate_lookup)

migration_pending_warning!
false
end
end

# The ActiveJob job class, as a string
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

class CreateIndexGoodJobJobsForCandidateLookup < ActiveRecord::Migration[7.1]
disable_ddl_transaction!

def change
reversible do |dir|
dir.up do
# Ensure this incremental update migration is idempotent
# with monolithic install migration.
return if connection.index_name_exists?(:good_jobs, :index_good_job_jobs_for_candidate_lookup)
end
end

add_index :good_jobs, [:priority, :created_at], order: { priority: "ASC NULLS LAST", created_at: :asc },
where: "finished_at IS NULL", name: :index_good_job_jobs_for_candidate_lookup,
algorithm: :concurrently
end
end
3 changes: 2 additions & 1 deletion demo/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2023_12_18_003738) do
ActiveRecord::Schema.define(version: 2024_01_14_221613) do
# These are extensions that must be enabled in order to support this database
enable_extension "pgcrypto"
enable_extension "plpgsql"
Expand Down Expand Up @@ -88,6 +88,7 @@
t.index ["cron_key", "cron_at"], name: "index_good_jobs_on_cron_key_and_cron_at_cond", unique: true, where: "(cron_key IS NOT NULL)"
t.index ["finished_at"], name: "index_good_jobs_jobs_on_finished_at", where: "((retried_good_job_id IS NULL) AND (finished_at IS NOT NULL))"
t.index ["labels"], name: "index_good_jobs_on_labels", where: "(labels IS NOT NULL)", using: :gin
t.index ["priority", "created_at"], name: "index_good_job_jobs_for_candidate_lookup", where: "(finished_at IS NULL)"
t.index ["priority", "created_at"], name: "index_good_jobs_jobs_on_priority_created_at_when_unfinished", order: { priority: "DESC NULLS LAST" }, where: "(finished_at IS NULL)"
t.index ["queue_name", "scheduled_at"], name: "index_good_jobs_on_queue_name_and_scheduled_at", where: "(finished_at IS NULL)"
t.index ["scheduled_at"], name: "index_good_jobs_on_scheduled_at", where: "(finished_at IS NULL)"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ class CreateGoodJobs < ActiveRecord::Migration<%= migration_version %>
add_index :good_jobs, [:finished_at], where: "retried_good_job_id IS NULL AND finished_at IS NOT NULL", name: :index_good_jobs_jobs_on_finished_at
add_index :good_jobs, [:priority, :created_at], order: { priority: "DESC NULLS LAST", created_at: :asc },
where: "finished_at IS NULL", name: :index_good_jobs_jobs_on_priority_created_at_when_unfinished
add_index :good_jobs, [:priority, :created_at], order: { priority: "ASC NULLS LAST", created_at: :asc },
where: "finished_at IS NULL", name: :index_good_job_jobs_for_candidate_lookup
add_index :good_jobs, [:batch_id], where: "batch_id IS NOT NULL"
add_index :good_jobs, [:batch_callback_id], where: "batch_callback_id IS NOT NULL"
add_index :good_jobs, :labels, using: :gin, where: "(labels IS NOT NULL)", name: :index_good_jobs_on_labels
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

class CreateIndexGoodJobJobsForCandidateLookup < ActiveRecord::Migration<%= migration_version %>
disable_ddl_transaction!

def change
reversible do |dir|
dir.up do
# Ensure this incremental update migration is idempotent
# with monolithic install migration.
return if connection.index_name_exists?(:good_jobs, :index_good_job_jobs_for_candidate_lookup)
end
end

add_index :good_jobs, [:priority, :created_at], order: { priority: "ASC NULLS LAST", created_at: :asc },
where: "finished_at IS NULL", name: :index_good_job_jobs_for_candidate_lookup,
algorithm: :concurrently
end
end
2 changes: 1 addition & 1 deletion lib/good_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def self.deprecator
def self.migrated?
# Always update with the most recent migration check
GoodJob::Execution.reset_column_information
GoodJob::Execution.active_job_id_index_removal_migrated?
GoodJob::Execution.candidate_lookup_index_migrated?
end

ActiveSupport.run_load_hooks(:good_job, self)
Expand Down

0 comments on commit 6a7332a

Please sign in to comment.