malthe / pq

A PostgreSQL job queueing system
376 stars 41 forks source link

queue.put() inside a transaction sets enqueued_at to the transaction start time, not the current time #65

Open miggec opened 3 years ago

miggec commented 3 years ago

Expected Behavior

I'd expect enqueued_at to represent the time at which the task was put into the queue table (e.g. CLOCK_TIMESTAMP(), since that is less arbitrary than the start time of the transaction (CURRENT_TIMESTAMP).

Actual Behavior

enqueued_at is set to the transaction start time by default.

Steps to Reproduce the Problem

with q as cursor:
    thing = q.get()
    # do some processing
    time.sleep(3)
    # put the task back on the queue
    q.put(thing.data)  # timestamp here is the transaction start time, not the current time

Specifications

malthe commented 3 years ago

I can see why that might be more ideal, but to motivate this, do you have a specific situation where this is a problem? If we change the behavior, how would we motivate/document this change in the change log?

miggec commented 3 years ago

Thanks - ultimately I defer to you as to whether the behaviour should be changed or not by default, as it's fairly trivial to override with a bit of SQL.

My use case that motivated this report: I want to perpetually process some tasks, and then re-queue them to be processed again in the near future. I dequeue and enqueue within the same transaction to be sure that the task is always in my queue. I expect to have hundreds of tasks and a handful of workers, so tasks may sit in the queue for a few minutes or more.

I want to measure and report on the length of time that tasks spend sitting in the queue, so that I can do something about it if that becomes too long. But for that I need an accurate enqueue time, otherwise I am also measuring the length of time taken by my task.

The motivation statement in my view: the enqueued_at time should be the time a task was enqueued. The start of a transaction is an implementation detail that has little relevance to that concept, rendering the timestamp somewhat arbitrary in this case.

malthe commented 3 years ago

@stas I think this makes sense.

stas commented 3 years ago

Oh, yup it does, even though it sounds quite specific.

Generally I don't think this should affect end-users too much, otherwise this issue would have been brought up already :joy: I'm totally ok with changing this!