Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Start work on avoiding table locks for upserts #2684

Merged
merged 4 commits into from
Nov 16, 2017
Merged

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Nov 16, 2017

Add a loop to _simple_upsert so that it can be used without a lock in more
circumstances, and a PoC to demonstrate it in add_pusher.

WDYT?

Anyway, it's all very well but doesn't really help us for more complex
operations where we do more than one thing in the transaction (for example,
delete_pusher_by_app_id_pushkey_user_id). There we have a few options:

  • decide that the delete from pushers and the upsert into deleted_pushers
    don't actually need to happen in a single transaction, and use
    _simple_upsert there too

  • cargo-cult the loop from _simple_upsert into
    delete_pusher_by_app_id_pushkey_user_id

  • factor out the loop to a retry_on_integrity_error or something and use that

Bail out early to reduce indentation
wrap the call to _simple_upsert_txn in a loop so that we retry on an
integrityerror: this means we can avoid locking the table provided there is an
unique index.
Now that _simple_upsert will retry on IntegrityError, we don't need to lock the
table.
@ara4n
Copy link
Member

ara4n commented Nov 16, 2017

it's surprising to me that we'd actually need a delete from pusher & upsert into deleted_pushers to be atomic...? surely nothing cares particularly about deleted_pushers.

@richvdh
Copy link
Member Author

richvdh commented Nov 16, 2017

it's surprising to me that we'd actually need a delete from pusher & upsert into deleted_pushers to be atomic...? surely nothing cares particularly about deleted_pushers.

Yes, that was option 1.

deleted_pushers is a way of making sure that the workers know that a pusher has been deleted, aiui. Don't know why it being atomic helps, though.

@erikjohnston
Copy link
Member

@ara4n I believe deleted_pushers is what is used to stop the pusher worker from actually sending push to that device, so I would be fairly keen to not risk the stream from getting out of sync with the data.

@richvdh TBH we could probably make deleted_pushers be a simple insert; given its a stream I don't think it matters at all if things get inserted multiple times. I'd also imagine that generally we'd not have many "duplicate" inserts

@richvdh
Copy link
Member Author

richvdh commented Nov 16, 2017

(I'll fix the tests)

@richvdh
Copy link
Member Author

richvdh commented Nov 16, 2017

Assuming that the remaining test failure is the thing erik is fixing in matrix-org/sytest#421

@richvdh richvdh merged commit ba05f28 into develop Nov 16, 2017
@richvdh richvdh deleted the rav/unlock_upsert branch November 29, 2017 16:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants