Add indexes, drop redundant column

[bretthoerner]

No description provided.

[tomasfarias]

nit(naming): Should we keep completed_at and drop finished_at? Any thoughts into which name fits better?

[tomasfarias]

I'm kind of indifferent, but wondering if there's any reason to prefer one over the other.

[bretthoerner]

As it's currently used, finished_at feels slightly more correct. Because we set it even when we put a row back in for a retry:

rusty-hook/hook-common/src/pgqueue.rs

Lines 285 to 316 in 126908f

	impl<'c, J> PgTransactionJob<'c, J> {
	pub async fn retry<E: serde::Serialize + std::marker::Sync>(
	mut self,
	error: E,
	preferred_retry_interval: Option<time::Duration>,
	) -> Result<RetryableJob<E>, PgJobError<PgTransactionJob<'c, J>>> {
	if self.job.is_gte_max_attempts() {
	return Err(PgJobError::RetryInvalidError {
	job: self,
	error: "Maximum attempts reached".to_owned(),
	});
	}
	let retryable_job = self.job.retry(error);
	let retry_interval = self
	.retry_policy
	.time_until_next_retry(&retryable_job, preferred_retry_interval);
	let base_query = format!(
	r#"
	UPDATE
	"{0}"
	SET
	finished_at = NOW(),
	status = 'available'::job_status,
	scheduled_at = NOW() + $3,
	errors = array_append("{0}".errors, $4)
	WHERE
	"{0}".id = $2
	AND queue = $1
	RETURNING
	"{0}".*

To me, completed really means we're done (completed or failed) where it seems like we're using finished_at as more like a last_attempt_finished_at (maybe that over verbose name would be better?)

edit: I don't know why I feel like completed is more "final" than finished. Although IMO it would be odd to use completed_at AND have the completed status, and have rows with completed_at not NULL and status = available.

[tomasfarias]

Yeah, I agree completed_at feels more final and implies it was with completed status. We could have one timestamp per status, i.e. completed_at, failed_at, retried_at, but that gets redundant quickly and hard to maintain if we ever add another status.

[bretthoerner]

It could also be an array, only the final timestamp could be completed or failed anyway, anything before the final basically has to be "attempt that failed". I guess all that gets us is knowledge of when failed attempts happened, I don't know how useful that is. You're right that we could add more statuses anyway, though.

[tomasfarias]

nit: I would squash migrations given that this is not yet released anywhere (I personally drop and recreate my local Postgres all the time). Not a blocker though as we can squash before moving this to production.

[bretthoerner]

Ew, I use the vscode SQL Formatter extension and disagree with its choices, one sec.