Lock Sliding Sync connections when inserting lazy members, to prevent repeated deadlocks.

[reivilibre]

Got paged today for this. The sliding sync worker in question had loads of deadlocks in the logs.
I restarted it and it got unwedged, but we should have a more robust defence, which this PR proposes.

psycopg2.errors.DeadlockDetected: deadlock detected
DETAIL:  Process 257324 waits for ShareLock on transaction 688227036; blocked by process 254908.
Process 254908 waits for ShareLock on transaction 688222971; blocked by process 256179.
Process 256179 waits for ExclusiveLock on tuple (302352,92) of relation 2962200779 of database 16403; blocked by process 257213.
Process 257213 waits for ShareLock on transaction 688225005; blocked by process 254905.
Process 254905 waits for ShareLock on transaction 688228814; blocked by process 257324.
HINT:  See server log for query details.
CONTEXT:  while inserting index tuple (183070,103) in relation "sliding_sync_connection_lazy_members"

I wonder if an unfortunate side effect is that these repeated attempts leave a lot of dead tuples on the table,
which would then harm the performance of the next attempt to insert the tuples,
I suspect making it more likely that they will deadlock again (?).

---

By acquring a FOR NO KEY UPDATE lock upfront before beginning work, we can ensure that one
of the transactions gets queued behind the other one, meaning the first one can succeed unimpeded.

FOR NO KEY UPDATE blocks other FOR NO KEY UPDATE locks and is the weakest lock level that blocks itself.

[erikjohnston]

We probably want to go further than this and block all concurrent writes on a given connection to also avoid serialisation failures. Generally it's also good to do as much locking as possible at the start of the transaction.

We already fetch the connection_key from sliding_sync_connections as the first query in the transaction, so it might just be as easy as adding the locking there?

[reivilibre]

Yup derp, I did mean to go to the top but didn't occur to me to check whether this was called as part of something else. I will blame the 3am factor.

[MadLittleMods]

For my own reference, can someone explain the real life situations that causes this?

Someone is sending multiple concurrent requests with the same connection_key?

[reivilibre]

It would appear so, I guess? There were multiple users involved.

I suppose it's possible that some of these were retries after a connection dropped/timed out/... or something like that.

[erikjohnston]

I don't suppose we can optionally add this to the query above if its postgres? Or does it not work for more complex select statements?

[reivilibre]

That's on a different table.
We need a lock over the connection_key so it feels like putting it on the sliding_sync_connections table might be best.

I'm not really seeing a sensible way to rejig this otherwise; in the other branch of the if-else there may not be rows on sliding sync connection positions to lock

[erikjohnston]

The select above joins on sliding_sync_connections so I think should work? You can also do FOR NO KEY UPDATE OF sliding_sync_connections by the looks of it

[reivilibre]

Ahhhh right I wasn't aware of OF xxx. That'll do it

[erikjohnston]

Thanks!