koolba 6 days ago

The article says:

> Never Handroll Your Own Two-Phase Commit

And then buried at the end:

> A few notable features of this snippet:

> Limiting number of retries makes the code more complicated, but definitely worth it for user-facing side-effects like emails.

This isn't two-phase commit. This is lock the DB indefinitely while remote system is processing and pray we don't crash saving the transaction after it completes. That locked also eats up a database connection so your concurrency is limited by the size of your DB pool.

More importantly, if the email sends but the transaction to update the task status fails, it will try again. And again. Forever. If you're going to track retries it would have to be before you start the attempt. Otherwise the "update the attempts count" logic itself could fail and lead to more retries.

The real answer to all this is to use a provider that supports idempotency keys. Then when you can retry the action repeatedly without it actually happening again. My favorite article on this subject: https://brandur.org/idempotency-keys

  • greener_grass 13 hours ago

    > The real answer to all this is to use a provider that supports idempotency keys. Then when you can retry the action repeatedly without it actually happening again. My favorite article on this subject: https://brandur.org/idempotency-keys

    Turtles all the way down?

    Let's say you are the provider that must support idempotency keys? How should it be done?

    • nothrabannosir 12 hours ago

      Offer 99.something% guaranteed exactly-once-delivery. Compete on number of nines. Charge appropriately.

  • maxmcd 6 days ago

    Just that row should be locked since it's: "for update skip locked".

    I agree the concurrency limitation is kind of rough, but it's kind of elegant because you don't have to implement some kind of timeout/retry thing. You're certainly still exposed to the possibility of double-sending, so yes, probably much nicer to update the row to "processing" and re-process those rows on a timeout.

  • surprisetalk 6 days ago

    Author here! Posting from phone while traveling so sorry for bad formatting.

    It was outside of the scope of this essay, but a lot of these problems can be resolved with a mid-transaction COMMIT and reasonable timeouts

    You can implement a lean idempotency system within the task pattern like this, but it really depends on what you need and what failures you want to prevent

    Thanks for providing more context and safety tips! :)

  • tracker1 6 days ago

    For similar systems I've worked on, I'll use a process id, try/tries and time started as part of the process plucking an item of a db queue table... this way I can have something that resets anything started over N minutes prior that didn't finish, for whatever reason (handling abandoned, broken tasks that are in an unknown state.

    One reason to do this for emails, IE a database queue is to keep a log/history of all sent emails, as well as a render for "view in browser" links in the email itself. Not to mention those rare instances where an email platform goes down and everything starts blowing up.

  • lelanthran 11 hours ago

    > This isn't two-phase commit.

    Agreed

    > This is lock the DB indefinitely while remote system is processing and pray we don't crash saving the transaction after it completes.

    I don't really see the problem here (maybe due to my node.js skillz being less than excellent), because I don't see how it's locking the table; that one row would get locked, though.

    • Tostino 9 hours ago

      You are right. It is just a row level lock... But that doesn't change the fact you are explicitly choosing to use long running transactions, which adds to table bloat and eats active connections to your DB as mentioned. It also hurts things like reindexing.

      I prefer an optimistic locking solution for this type of thing.

      • lelanthran 9 hours ago

        > But that doesn't change the fact you are explicitly choosing to use long running transactions, which adds to table bloat and eats active connections to your DB as mentioned.

        TBH, I'm not too worried about that either - my understanding from the content is that you'd have a few tasks running in the background that service the queue; even one is enough.

        I'd just consider that to be always-active, and turn the knobs accordingly.

        > It also hurts things like reindexing.

        I dunno about this one - does re-indexing on the queue occur often enough to matter? After all, this is a queue of items to process. I'd be surprised if it needed to be re-indexed at all.

        • Tostino an hour ago

          To start off, I said optimistic locking before my and I actually meant pessimistic locking.

          But I think it totally depends on what your queue is used for. In my case, I need durable queues that report status/errors and support retries/back off.

          I deal with it using updates rather than deleting from the queue, because I need a log of what happened for audit purposes. If I need to optimize later, I can easily partition the table. At the start, I just use a partial index for the items to be processed.

          Reindexing, and other maintenance functions that need to rewrite the table will happen more than you like in a production system, so I'd rather make them easy to do.

      • Tostino an hour ago

        I said optimistic in my post above... I really meant pessimistic locking. Just wanted to clarify and couldn't edit original comment.

  • morshu9001 6 days ago

    Idempotency is key. Stripe is good about that.

fastest963 2 hours ago

It can be more complicated depending on your environment but I'd prefer instead to use a Pub/Sub pattern instead. You can have a generic topic if you want to model it like the generic table but it handles retries, metrics, scaling, long-running, etc all for you. If you are running in the cloud then Pub/Sub is something you don't need to maintain and will scale better than this solution. You also won't need to VACUUM the Pub/Sub solution ;)

rictic 6 days ago

Missing from the article: how to communicate progress and failure to the user?

This is much more complicated with task queues. Doable still! But often skipped, because it's tempting to imagine that the backend will just handle the failure by retrying. But there are lots of kinds of failure that can happen.

The recipient's server doesn't accept the email. The recipient's domain name expired. Actually, we don't have an email address for that recipient at all.

The user has seen "got it, will do, don't worry about it" but if that email is time sensitive, they might want to know that it hasn't been sent yet, and maybe they should place a phone call instead.

  • nrhrjrjrjtntbt 10 hours ago

    You can still do that. You can poll status while the page is open. Toast errors or state changes. Even toast them on return. Anything is possible.

    After all Amazon does this for a physical order. It may be days before a status update!

zanellato19 9 hours ago

I feel like this is basically what the Rails world does. Sidekiq handles a lot of this for you and it's honestly an amazing tool.

It does rely on redis, but it's basically the same idea.

  • owenmakes 8 hours ago

    In Rails 8 you have SolidQueue by default, which doesn't rely on redis

  • arkh 6 hours ago

    Same thing with Symfony and its Messenger component when setup to use a database.

adamzwasserman 7 hours ago

Fair warning: don't bounce off the first paragraph like I did. "Dumb queries" made me think the author was arguing against SQL sophistication — I was halfway through composing a rebuttal about stored procedures and keeping logic in the database before I actually read the rest.

Turns out the article advocates exactly that. The example uses CTEs with multi-table inserts. "Dumb" here means "no synchronous external service calls," not "avoid complex SQL."

w23j 7 hours ago

We use a similar approach.

Fun fact: A query like this will, once in a blue moon, return more than limit (here 1) row, since the inner query is executed multiple times and returns different ids, which is surprising for a lot of people. If your code does not expect that, it may cause problems. (The article seems to, since it uses a list and iteration to handle the result.)

  delete from task
  where task_id in
  ( select task_id
    from task
    order by random() -- use tablesample for better performance
    for update
    skip locked
    limit 1
  )
  returning task_id, task_type, params::jsonb as params 
You can avoid that by using a materialized Common Table Expression. https://stackoverflow.com/questions/73966670/select-for-upda...

Also, if your tasks take a long time, it will create long-running transactions, which may cause dead tuple problems. If you need to avoid that, you can mark the task as "running" in a short-lived transaction and delete it in another. It becomes more complicated then, since you need to handle the case that your application dies while it has taken a task.

arjie 13 hours ago

The sophisticated solution to this problem is Temporal, but yes, I also use an async task queue frequently because it's very easy to roll one's own.

efxhoy 6 days ago

I like it! We have a service with a similar postgres task queue but we use an insert trigger on the tasks table that does NOTIFY and the worker runs LISTEN, it feels a bit tidier than polling IMO.

nullzzz 6 days ago

I can recommend this architecture. So much easier to maintain and understand than using an extra service. The implementation here I didn’t go into much detail, but you can surely roll your own if this doesn’t cut it for you, or use a library like pgboss.

stack_framer 6 days ago

> I like slim and stupid servers, where each endpoint wraps a very dumb DB query.

I thought I was alone in this, but I build all my personal projects this way! I wish I could use this approach at work, but too many colleagues crave "engineering."

  • stronglikedan 6 days ago

    Doesn't that make for exponentially more requests to get the same data, or possibly more data than you really need (bloated responses)?

    • never_inline 7 hours ago

      Some people really overdo HTTP verbs /GET, /POST, /PUT, /DELETE and leave much work to frontend. Irks me a lot.

      But then again, there's GraphQL because frontend developers thought backend developers are anti social.

      • dragonwriter 4 hours ago

        > Some people really overdo HTTP verbs /GET, /POST, /PUT, /DELETE and leave much work to frontend. Irks me a lot.

        If I understand you correctly, I don't think of it as overdoing HTTP verbs so much as using an excessively naive mapping between HTTP resources and base table entities.

rgbrgb 6 days ago

If you're in TS/JS land, I like to use an open source version of this called graphile-worker [0].

[0]: https://worker.graphile.org

  • damidekronik 6 days ago

    I am using pgboss myself, very decent, very simple. Had some issues with graphile back in the days, cant remember what exaclty, it probably did already overcome whatever I was struggling with!

morshu9001 6 days ago

Never do RPCs during an xact like this! Fastest way to lock up your DB. I don't even mean at large scale. I've been forced many times to set up two-phase commit. That way you also get more flexibility and visibility into what it's doing.

claytongulick 7 hours ago

At a brief scan of the code, is there a bug with the way task rows are selected and rolled back?

It looks like multiple task rows are being retrieved via a delete...returning statement, and for each row an email being sent. If there's an error, the delete statement is rolled back.

Let's hypothesize that a batch of ten tasks are retrieved, and the 9th has a bad email address, so the batch gets rolled back on error. Next retry the welcome email would be sent again for the ones that succeeded, right?

Even marking the task as "processed" with the tx in the task code wouldn't work, because that update statement would also get rolled back.

Am I missing something? (entirely possible, the code is written in "clever" style that makes it harder for me to understand, especially the bit where $sql(usr_id) is passed into the sql template before it's been returned, is there CTE magic there that I don't understand?)

I thought that this was the reason that most systems like this only pull one row at a time from the queue with skip locked...

Thanks to anyone who can clarify this for me if it is indeed correct!

  • w23j 7 hours ago

    I mean the inner select uses "limit 1", right? So it will usually (but not always as I said in another comment) only delete and return a single task.