Add basic read/write lock

[erikjohnston]

Similar to the existing worker locks, add support for read/write style locks.

The idea of the read/write lock tables is that the combination of unique and foreign key constraints enforces the read/write constraint. The triggers are there simply to reduce the number of statements required to be sent by Synapse to reduce latency of acquiring/releasing the lock.

Note that this implementation of read/write lock suffers from writer starvation, i.e. if processes keep taking out read locks without any gap, an attempt to get a write lock will never succeed.

Reviewable commit-by-commit.

Part of #13476 (comment)

[MadLittleMods]

I think this looks good 👍

I haven't fully mind-melded into the code around the constraints to see something wrong (I did look at everything though) but the tests look robust enough to catch any errant behavior. If you want me to fully understand this, feel free to poke again specifically.

[MadLittleMods]

Gave the constraints a closer look. I think they make sense.

Kept trying to think if we could do it with one table but couldn't figure anything out that gets us that same read xor write behavior.

[MadLittleMods]

We discussed this a bit this morning that the foreign key has to be added after the fact because the worker_read_write_locks table isn't created yet when we first define the worker_read_write_locks_mode table.

Could we instead move this ALTER TABLE statement after worker_read_write_locks is created in synapse/storage/schema/main/delta/78/03_read_write_locks.sql. Or maybe not because each delta is done in a single transaction?

[erikjohnston]

Could we instead move this ALTER TABLE statement after worker_read_write_locks is created in synapse/storage/schema/main/delta/78/03_read_write_locks.sql. Or maybe not because each delta is done in a single transaction?

We cannot, as we can't apply this to SQLite (due to them not support ALTER TABLE, and thus not supporting "circular" foreign key constraints). I'll add a comment.

[reivilibre]

I wonder if you can temporarily disable foreign key constraints to get around the problem of not being able to add circular ones.

See section 7 of https://www.sqlite.org/lang_altertable.html: when doing other kinds of table recreations, they suggest you to disable the FKs temporarily, presumably to give you a window in which you can mess around with them and not get told off.

[erikjohnston]

Ok, so turns out it is possible to do foreign key constraints in SQLite, and it works out of the box if you just create the two tables in a transaction. However, that doesn't work for postgres, which means its non-trivial to support both.

Options here are:

1. Just have separate schema files for both SQLite and Postgres, making it non-obvious they're the same schema really.
2. Ignore the second foreign key constraint for SQLite, as we care less about it (and the triggers should ensure that the correct things happen anyway).

[MadLittleMods]

Either way seems fine.

They're already pretty disparate with the trigger code so separating the schemas isn't a stretch.

And the other way is also fine since we were willing to not to have the foreign key in SQLite before we realized circular foreign keys was actually supported. My only thought around this is if we think the triggers should do the right thing, then perhaps we don't need the extra foreign key for Postgres either. But it's also just a easy thing to add to ensure the triggers are doing the right thing into the future 🤷

[erikjohnston]

I've opted to split the schemas.