rails / solid_queue

Database-backed Active Job backend
MIT License
1.66k stars 90 forks source link

Add indexes for created_at on executions table #207

Closed Intrepidd closed 3 months ago

Intrepidd commented 3 months ago

Necessary in order to implement this : https://github.com/rails/mission_control-jobs/pull/114

I added the index on all executions tables but this may well be overkill, happy to change it if you believe we should only add it on failed executions for now

rosa commented 3 months ago

Thanks @Intrepidd! I continued thinking about that one... and what if we sorted failed_executions on job_id, but descending? This is what we do for recurring_executions in Mission Control. In that case the sort is going to mimic very well the creation date order because recurring executions are created at the same time as the job. For failed executions, sorting on the job_id won't correspond exactly to the same order jobs failed because it depends on how long a job takes to fail, but I think it should be a good approximation in most cases. In this way, we wouldn't need any new indexes.

Intrepidd commented 3 months ago

I thought about it but it would not correctly handle retries would it ? You probably want on top of the pages the most recent failures.

Imagine a job failed a few days ago sitting in page 3, if you retry it and it fails again you want to see it on top of page one don't you ?

rosa commented 3 months ago

Hmm yes, that's true 🤔 Perhaps we could sort by primary key instead 🤔 That is, failed_executions.id.

rosa commented 3 months ago

Depending on the filtering, no index could be used for sorting, though. But the same would be true for a new index on created_at anyway. It'd only be used when no filtering is done. I had suggested job_id because I think, with the joins, we do for filtering, we could also use the index on job_id for sorting, but the retries prevent that, as you said.

Intrepidd commented 3 months ago

Hmm yes, that's true 🤔 Perhaps we could sort by primary key instead 🤔 That is, failed_executions.id.

This seems obvious now :D I'll make the changes in the other PR

rosa commented 3 months ago

Awesome! Thank you 🙏🏻