feat: postgres read and write locks #1891

taddes · 2025-11-13T22:52:38Z

Description

Introduces the logic to lock the db for reads and writes for the Postgres implementation.

Will merge in and rebase based on changes from #1896

Issue(s)

pjenvey · 2025-11-17T20:51:12Z

syncstorage-postgres/syncstorage_postgres_db.md

 | `fxa_kid`       | `TEXT`      | Key identifier; part of the sync crypto context. PK (part 2) |
 | `collection_id` | `INTEGER`    | Maps to a named collection. PK (part 3)                       |
-| `modified`      | `TIMESTAMP` | Last modification time (server-assigned, updated on writes)           |
+| `modified`      | `BIGINT` | Last modification time (server-assigned, updated on writes)           |


Is this too bothersome to support as a TIMESTAMP?

Not necessarily, may opt for a raw query over the ORM methods, as we've discussed, given those can be a pain with the chain methods. Do we prefer this to be a timestamp? I know in Spanner it's a timestamp and in MySQL a BIGINT

For now, @pjenvey I say let's keep it consistent as an integer, given the integrated testing for MySQL and Postgres, much like in Tokenserver. We have a ticket on the books to standardize all datetime ops to chrono, which also happens to include making sure any ops that should move to datetime ones, opposed to using large numbers will update. This way we can update MySQL and Postgres at the same time. Thoughts?

given the integrated testing for MySQL and Postgres

Sorry I don't have enough context to understand this. Can you link to a test as an example? Thanks!

A good example is like the work you did for the Postgres tests that ran in CI for Tokenserver. The reason they were general purpose and could work for both SQL and Postgres was that the interfaces were quite similar. My thought here is that we try and keep true-ish to MySQL and as we near the end of integrating the tests, look at making adjustments if need be. Changing a data type from an i64 to a timestamp for instance results in pretty different logic that would check it. This way we're not duplicating things before we need to.

I think I understand your explanation, @taddes, thanks. But I don't agree or maybe simply don't understand why the existing tests should stop us from using the correct data type(s).

My thought here is that we try and keep true-ish to MySQL and as we near the end of integrating the tests, look at making adjustments if need be.

How do we decide if adjustments are needed?

The tests are against the Db/DbPool interfaces, whereas the column type here is an internal impl. detail. By the time our Box<dyn Db> returns e.g. a results::GetBso, it would have converted whatever internal representation of modified (whether BIGINT or TIMESTAMP) to a SyncTimestamp.

chenba · 2025-11-17T21:26:59Z

syncstorage-postgres/migrations/2025-10-20-155711_create_schema/up.sql

 -- user_collections table
 CREATE TABLE user_collections (
-    fxa_uid UUID NOT NULL,
+    fxa_uid BIGINT NOT NULL,


I don't understand this change. Should the column also be renamed? iirc the last time the team chatted about this type we want it to be TEXT.

Agreed, but I'm killing fxa_uid/kid shortly, so it doesn't matter

It'll be in another PR when we change the whole field. Also the PR is still WIP

chenba · 2025-11-17T21:38:50Z

syncstorage-postgres/src/db/db_impl.rs

+    ///
+    /// In theory it would be possible to use serializable transactions rather
+    /// than explicit locking, but our ops team have expressed concerns about
+    /// the efficiency of that approach at scale.


Cool. Are there written notes on that somewhere?

I will look around and see regarding the ops team stuff, though the first portion is what I took from Diesel's internal docs on these lock methods.

chenba · 2025-11-19T19:54:18Z

syncstorage-postgres/src/db/db_impl.rs

+        self.session
+            .coll_locks
+            .insert((user_id as u32, collection_id), CollectionLock::Read);
+        Ok(())


This function is identical to the one in syncstorage-mysql/src/db/db_impl.rs. Do we need two copies of this function?

(The MySQL copy also has two questions in the comments I'm very interested in.)

Yes! I am curious about those comments as well. It's likely we can move said function into a shared crate at some point if both of them use it.

There's some small opportunities to share more common code between the different backends but unfortunately we can't share general diesel ORM usage (utilizing diesel-async) between different backends easily (if at all), at least at the moment.

Basically because diesel's very strict: its base API (and its more internal associated types/trait bounds that come along with it) is typed to specific db backends. This has the benefit of enforcing specific backend dialect API usage at compile time, but with the downside of making it difficult to write code against its generic traits like diesel::Connection -- it just wasn't designed for such usage.

However plain diesel added a kind of layer on top of its core called MultiConnection that enables this kind of usage, but unfortunately it's not yet supported by diesel-async.

The original plan was to prefer raw sql queries for most cases anyway, moving away from the mysql ORM styled code like tokenserver does. Part of building postgres "from scratch" is we're able to make a sort of audit of non spanner impls as the mysql version was never fully finalized (granted self hosters seemed to have had success with it).

I'm not opposed to the ORM usage though, if it's easy enough to use -- in fact I intended for raw sql in most cases but not necessarily all -- I definitely prefer the ORM for at least one case: the get_bsos/_ids call where we dynamically build a query is much easier/cleaner/less bug ridden with an ORM building it for us vs manually constructing it.

The second question I've covered here: #1891 (comment)

The first question sounds like more of a TODO/bug, we shouldn't be casting a u64 to a u32 here (or in mysql)! Let's change the type in the coll_ maps to match legacy_id's. We could even put the entire UserIdentifier in there instead, as long as we're sure it eq/hashes correctly for such usage.

chenba · 2025-11-19T19:54:23Z

syncstorage-postgres/src/db/db_impl.rs

+        self.session
+            .coll_locks
+            .insert((user_id as u32, collection_id), CollectionLock::Write);
+        Ok(())


Same with this function. It's functionally identical to the MySQL one. (There's a to_string here and a to_owned there for the error message so it's not syntactically 100% identical.)

Certainly can swap it, since those to_string and to_owned do much of the same thing.

(I'm pretty sure I responded but I guess the packets leaked into the ocean or something.)

The to_string vs to_owned was just an aside. My point was the Postgres and MySQL versions of the fn are essentially identical.

chenba · 2025-11-19T20:27:44Z

Related to this PR but more of a general question. When are entries in those process-local hashmaps of locks updated? I see inserts but not removals. I feel like I'm missing something very fundamental on db level locks vs what's in those hashmaps.

chenba · 2025-11-19T20:44:48Z

When are entries in those process-local hashmaps of locks

Oh the hashmaps are per request look like. I think I understand now.

pjenvey · 2025-11-20T06:06:03Z

Related to this PR but more of a general question. When are entries in those process-local hashmaps of locks updated? I see inserts but not removals. I feel like I'm missing something very fundamental on db level locks vs what's in those hashmaps.

These were lazily carried over from the Python version: they're never removed, and only really used as a sanity check in case the lock methods are called more than once (which never happens).

This could probably be improved/simplified -- we could remove them upon commit/rollback, but if we're doing that we might consider making commit/rollback consume self (if our usage of those methods allows that).

taddes self-assigned this Nov 13, 2025

taddes marked this pull request as ready for review November 14, 2025 01:37

taddes changed the title ~~feat: postgres read and write locks~~ [WIP] feat: postgres read and write locks Nov 14, 2025

pjenvey reviewed Nov 17, 2025

View reviewed changes

chenba reviewed Nov 17, 2025

View reviewed changes

taddes changed the title ~~[WIP] feat: postgres read and write locks~~ feat: postgres read and write locks Nov 18, 2025

taddes requested a review from pjenvey November 18, 2025 17:42

taddes force-pushed the feat/postgres-read-write-locks-STOR-335 branch from d3b7ac4 to e90a84c Compare November 18, 2025 22:45

chenba reviewed Nov 19, 2025

View reviewed changes

taddes force-pushed the feat/postgres-read-write-locks-STOR-335 branch from e90a84c to 211641f Compare November 20, 2025 22:10

taddes added 9 commits November 20, 2025 18:18

update types

7fadf20

docs for UserIdentifier

8fcc33e

PgDbSession coll lock type

861f823

lock logic

a346f6d

diesel extensions

4095206

update timestamp type to bigint

59ce061

capture matching modified value for user_id and collection_id

27f62c5

write impl

2692392

rebase and use user_id

acdb9c8

taddes force-pushed the feat/postgres-read-write-locks-STOR-335 branch from 211641f to 0e21ca5 Compare November 20, 2025 23:19

rebase map

0e21ca5

feat: postgres read and write locks #1891

Are you sure you want to change the base?

feat: postgres read and write locks #1891

Uh oh!

Conversation

taddes commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taddes Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjenvey Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenba commented Nov 19, 2025

Uh oh!

chenba commented Nov 19, 2025

Uh oh!

pjenvey commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taddes commented Nov 13, 2025 •

edited

Loading

taddes Nov 18, 2025 •

edited

Loading

pjenvey Nov 20, 2025 •

edited

Loading

pjenvey commented Nov 20, 2025 •

edited

Loading