Use transactions for some DB actions.

[dessalines]

Some DB actions like transfer community, should probably be transactions, so that if one DB action fails, it doesn't end up in a half-complete state.

---

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[Nutomic]

@dessalines Did you fix this in the DB rewrite?

[dessalines]

No not yet.

[AndrolGenhald]

I think a big part of the work here is to make all of the db_schema traits take a connection instead of a pool, otherwise they won't be usable inside a transaction; I've only done a cursory look though as I was trying to use a transaction and found it to not really be possible at the moment. Are there any major blockers for this issue?

[phiresky]

I want to caution against "blindly" using transactions everywhere. In my opinion transactions should only be used where absolutely necessary. The priority with a DB for perf is to have as little and short-time locks as possible. Only then can stuff run nicely in parallel.

For context, I'm someone running a (unrelated) postgresql instance that's > 4TB in size, uses 1TB of RAM, has 200k weekly active users and handles on average 1000 to 5000 INSERTs per second.

1. Unless you mark your transactions "ISOLATION LEVEL SERIALIZABLE" transactions actually fix much fewer problems than people expect. The default isolation level is READ COMMITTED (see the docs).
   • In default transactions, just doing a SELECT() and then an UPDATE/INSERT/DELETE() is exactly the same as if you had done it outside the transaction. You only get "consistency" if you do SELECT FOR UPDATE which will lock the rows. But that's usually also not what you want since then you're locking the database longer than you have to.
   • Really, think of transactions as just making sure multiple UPDATE/INSERT/DELETE queries fail together. That's it. It doesn't make them fail less, just more consistently.
2. Transactions with SERIALIZABLE make the code simpler but are terrible for db performance. You will also have to retry transactions all the time and you will be hit by tons of "could not serialize" errors. You will only notice these issues in production because on dev everything will look great.
3. The longer you keep a transaction open, the more likely you will get latency spikes.
   • Do as little application work as possible within the transaction. E.g. if you need to do HTTP requests or collect some Vec<>, do it before starting the transaction.
   • Ideally, a transaction should have zero roundtrips between the rust side and the postgresql side. Every roundtrip means rows being locked and the PG connection holding locks while being in the IDLE state. A bad example of this is the community_language code that does > 100 roundtrips in one transaction, which makes it very likely for a race condition to happen.
   • The real issue in that case is actually that the Community::create() method silently succeeds if the row already exists but the outer from_json function expects it to have inserted a new community, then proceeds to do other stuff that should only happen if the community is actually new (which only the create() function knows).
   • In this case it would be "fixed" by having the transaction be around the whole from_json function, since then the first call will lock the row with the primary key, and the second simultaneous call would wait during the insert_into(community::table) call, then do all the work a second time . But it would be better to remove the on_conflict().do_update() function so the create() function actually fails if it can't create a new entry, then just return the existing value if that's the case.

---

Here's some other suggestions:

• The rust code has a ton of dereference() calls. Logic should be added so that every dereference call first checks if a dereference of the same thing is already in-flight and if so take that one. Right now it does some HTTP requests multiple times, overwriting each other. Then there should probably also be something like an LRU cache of these. Something like a Cache<Url, Shared<Output>> should solve both issues (docs for Shared future). Cache could e.g. be moka. I wouldn't use anything like redis, just use a rust in-memory cache.
  • Basically that means instead of waiting on a database lock, then doing all work a second time, use the existing in-flight fetched value and not add new work.
• Clarify in the code what expectations method have and what results they provide. There should be a difference between create and update. If the function does both it should probably be called create_or_update.

[phiresky]

I'll try implementing a cache based on moka for dereference() in activitypub-federation-rust over the weekend.

[AndrolGenhald]

For context, I'm someone running a (unrelated) postgresql instance that's > 4TB in size, uses 1TB of RAM, has 200k weekly active users and handles on average 1000 to 5000 INSERTs per second.

Great to have input from someone running a database that gets hammered like that!

1. Unless you mark your transactions "ISOLATION LEVEL SERIALIZABLE" transactions actually fix much fewer problems than people expect. The default isolation level is READ COMMITTED (see the docs).

   • In default transactions, just doing a SELECT() and then an UPDATE/INSERT/DELETE() is exactly the same as if you had done it outside the transaction. You only get "consistency" if you do SELECT FOR UPDATE which will lock the rows. But that's usually also not what you want since then you're locking the database longer than you have to.
   • Really, think of transactions as just making sure multiple UPDATE/INSERT/DELETE queries fail together. That's it. It doesn't make them fail less, just more consistently.

Totally agree, but I would still think most methods that involve inserting records into multiple related tables should be done in transactions in order for them to succeed or fail together yes?

3. The longer you keep a transaction open, the more likely you will get latency spikes.

   • Do as little application work as possible within the transaction. E.g. if you need to do HTTP requests or collect some Vec<>, do it before starting the transaction.
   • Ideally, a transaction should have zero roundtrips between the rust side and the postgresql side. Every roundtrip means rows being locked and the PG connection holding locks while being in the IDLE state. A bad example of this is the community_language code that does > 100 roundtrips in one transaction, which makes it very likely for a race condition to happen.

Absolutely, holding a connection while doing an HTTP request is definitely not something you want to do!

[phiresky]

Totally agree, but I would still think most methods that involve inserting records into multiple related tables should be done in transactions in order for them to succeed or fail together yes?

Yes, that is true. My argument is mainly against just putting transactions around everything as a "precaution" which I've seen happen before, especially because of misconceptions of what transactions help with. Sometimes they help, sometimes they make things worse - sadly knowing exactly when needs pretty deep knowledge of the database.

Also, in many cases it's possible to get the same effect or something acceptable without using a transaction.

[dessalines]

Thanks for this work. I could definitely use some help optimizing the DB. The community_language stuff def needs to be reworked.

[Nutomic]

We are using transactions in various places now.