Potential bug in "jobPollingWithResourceSql" (not using "FOR UPDATE" clause)

[saurabhnanda]

If you compare jobPollingSql vs jobPollingWithResourceSql you'll find that the latter not using "FOR UPDATE"

odd-jobs/src/OddJobs/Job/Query.hs

Lines 22 to 34 in a159d7a

	jobPollingSql :: Query
	jobPollingSql =
	"update ? set status = ?, locked_at = ?, locked_by = ?, attempts=attempts+1 \
	\ WHERE id in (select id from ? where (run_at<=? AND ((status in ?) OR (status = ? and locked_at<?))) \
	\ ORDER BY attempts ASC, run_at ASC LIMIT 1 FOR UPDATE) RETURNING id"
	jobPollingWithResourceSql :: Query
	jobPollingWithResourceSql =
	" UPDATE ? SET status = ?, locked_at = ?, locked_by = ?, attempts = attempts + 1 \
	\ WHERE id in (select id from ? where (run_at<=? AND ((status in ?) OR (status = ? and locked_at<?))) \
	\ AND ?(id) \
	\ ORDER BY attempts ASC, run_at ASC LIMIT 1) \
	\ RETURNING id"

cc: @ivb-supercede

[jezen]

FYI, Isaac no longer works at Supercede, so he's probably only reachable through @ivanbakel.

[ivanbakel]

Thanks @jezen!

I agre, this is a potential bug and could lead to duplicated jobs being queued. However, there's the potential that the FOR UPDATE will lock all the rows accessed by the resource function (the docs are not super clear on this case when using a procedure). Therefore, there is the option to do FOR UPDATE OF ? and only lock the job table row that is being polled. For Supercede (IIRC, it's been a while), I believe this would be a roughly appropriate semantics, since the rows in the resource table do not need to be locked when queuing a new job - they are only read from, and synchronisation would not do anything.

However, it's not clear to me that this would be the case in general, and I don't have the resources or a test case to dig into whether FOR UPDATE or FOR UPDATE OF ? is better or more performant.