Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace UPDATE with UPSERT on device_max_stream_id table #6363

Merged
merged 5 commits into from Nov 15, 2019

Conversation

@anoadragon453
Copy link
Member

anoadragon453 commented Nov 14, 2019

Helps address #6311 (for the device_max_stream_id table anyways).

We had a problem where when our multiple schema delta files were combined into a single schema, it left out an INSERT statement that insert the first row into the device_max_stream_id. Synapse assumes this row exists and runs an UPDATE against it. However, this would do nothing if no row existed, which was the case here.

This change changes the UPDATE to an UPSERT, by adding a new column to the table with a unique constraint, and always trying to insert with the same value. If a conflict arises (which is should do any time there's a row in that table), then the stream_id value is updated. If there's no row in there (which will be the case with homeservers initially) then a simple INSERT occurs.

There may be a more elegant way to do this, but this works.

@anoadragon453 anoadragon453 self-assigned this Nov 14, 2019
@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Nov 14, 2019
@anoadragon453 anoadragon453 changed the title upsert on max to_device stream id Replace UPDATE with UPSERT on device_max_stream_id table Nov 14, 2019
@anoadragon453 anoadragon453 moved this from In progress to Review in Homeserver Task Board Nov 14, 2019
@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Nov 14, 2019
@dbkr
dbkr approved these changes Nov 15, 2019
Copy link
Member

dbkr left a comment

Hmm, so with this I think it will still reset after it's restarted with this new code, but then update correctly after that, so we swill still get one more round of UISIs after the update, after which it will be fine. We could add some code to the StreamIdGenerator where it loads the current ID to work around this... not sure if it's worthwhile though - probably better to just get this deployed.

sql = "UPDATE device_max_stream_id SET stream_id = ?"
else:
# No rows, perform an insert
sql = "INSERT INTO device_max_stream_id (stream_id) VALUES (?)"

This comment has been minimized.

Copy link
@dbkr

dbkr Nov 15, 2019

Member

You could also do the update as before and then check the affected row count which would then just be a single query in the normal case, but if this is how we're doing upserts elsewhere then I guess just stick with that.

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Nov 15, 2019

Author Member

It's a little more complicated since we have to check that the stream_id we're inserting is greater than what's currently in the DB. If it's not, then the UPDATE will return 0 rows affected.

We don't want to then insert another row at that point.

@anoadragon453

This comment has been minimized.

Copy link
Member Author

anoadragon453 commented Nov 15, 2019

I'll merge this as-is and we can improve in the future if necessary.

@anoadragon453 anoadragon453 merged commit 657d614 into develop Nov 15, 2019
20 checks passed
20 checks passed
buildkite/synapse Build #5532 passed (26 minutes, 33 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 24 seconds)
Details
buildkite/synapse/check-style Passed (2 minutes, 18 seconds)
Details
buildkite/synapse/isort Passed (19 seconds)
Details
buildkite/synapse/mypy Passed (23 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (16 seconds)
Details
buildkite/synapse/packaging Passed (23 seconds)
Details
buildkite/synapse/pipeline Passed (10 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (17 minutes, 41 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (6 minutes, 44 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (23 minutes, 53 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (6 minutes, 23 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (17 minutes, 58 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (17 minutes, 42 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 23 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 57 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (2 minutes, 5 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (15 minutes, 47 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (14 minutes, 27 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (14 minutes, 15 seconds)
Details
Homeserver Task Board automation moved this from Review to Done Nov 15, 2019
@anoadragon453 anoadragon453 deleted the anoa/device_sql branch Nov 15, 2019
@anoadragon453 anoadragon453 restored the anoa/device_sql branch Nov 15, 2019
@anoadragon453 anoadragon453 deleted the anoa/device_sql branch Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.