Use proper fields for finding prunable jobs

[ademenev]

Using "scheduled_at" is not correct. Depending on job state, one of "cancelled_at", "discarded_at" or "completed_at" should be used.

[sorentwo]

We switched to the scheduled_at timestamp to make use of the compound index and speed up pruning. There are likely to be many more completed jobs than cancelled or discarded, and a completed job has to execute to end up in that state. So, let's use scheduled_at for the completed state to make use of that index.

Suggested change

	|> where([j], j.state == "completed" and j.completed_at < ^time)
	|> where([j], j.state == "completed" and j.scheduled_at < ^time)

[ademenev]

Yes, that makes perfect sense performance-wise

If you do not do some tricky stuff, completed, cancelled and discarded rows never change. Probably they could all use a single timestamp field instead of 3, with a compound index on state and that field

[ademenev]

Isn't this redundant? queue column is not nullable

where([j], not is_nil(j.queue))

[sorentwo]

The is_nil check is pointless with the current index structure, but it was required to force index usage with the old compound table where queue was before state. The index change was optional (way back when), and some people never chose that option.

We can probably remove it at this point, but it doesn't change the query plan.

[sorentwo]

Thanks for submitting this improvement!

[ademenev]

Would you consider this?

If you do not do some tricky stuff, completed, cancelled and discarded rows never change. Probably they could all use a single timestamp field instead of 3, with a compound index on state and that field

May be a bit of work and require a heavy migration though

[sorentwo]

Would you consider this?

In theory, yes. It would require a few migrations for zero downtime and coordinated changes between packages. That's a lot and I'm not convinced it's worth it.