Remove option to skip locking of tables during emulated upserts

[squahtx]

To perform an emulated upsert into a table safely, we must either:

• lock the table,
• be the only writer upserting into the table
• or rely on another unique index being present.

When the 2nd or 3rd cases were applicable, we previously avoided locking
the table as an optimization. However, as seen in #14406, it is easy to
slip up when adding new schema deltas and corrupt the database.

The only time we lock when performing emulated upserts is while waiting
for background updates on postgres. On sqlite, we do no locking at all.

Let's remove the option to skip locking tables, so that we don't shoot
ourselves in the foot again.

Signed-off-by: Sean Quah seanq@matrix.org

---

Originally suggested by @DMRobertson.

[DMRobertson]

I went through the cases affected to see if any of them might use an emulated upsert. (I.e. would any of these upserts start locking tables after this change, when they wouldn't have before?)

AFAICS I don't think that's true of any of these cases. (I do find all of this quite confusing, so I wouldn't mind a sanity check of my reasoning.)

[DMRobertson]

we cannot make the same atomicity guarantees that a native upsert can

I wonder if my phrasing was poor here. Maybe we can make those guarantees if we're using a sufficiently high isolation level?

[squahtx]

I tried to reword it a little. Suggestions welcome.

[DMRobertson]

Unique index exists here for postgres:

synapse/synapse/storage/schema/main/delta/42/user_dir.py

Line 59 in 25f43fa

CREATE UNIQUE INDEX user_directory_search_user_idx ON user_directory_search(user_id);

... but not for sqlite. It's marked as unsafe to upsert into on sqlite here:

synapse/synapse/storage/database.py

Lines 534 to 538 in c3a4780

	# We add the user_directory_search table to the blacklist on SQLite
	# because the existing search table does not have an index, making it
	# unsafe to use native upserts.
	if isinstance(self.engine, Sqlite3Engine):
	self._unsafe_to_upsert_tables.add("user_directory_search")

Does that mean that with this change we're now going to start locking user_directory_search on sqlite deployments? Ahh, but locking a table is a no-op on sqlite, presumably because there's at most one worker writing?

synapse/synapse/storage/engines/sqlite.py

Lines 103 to 104 in 8c94dd3

	def lock_table(self, txn: Cursor, table: str) -> None:
	return

[squahtx]

Good spot with user_directory_search. I wasn't expecting that. Thankfully the no-op locking saves us.

But if locking on sqlite has never done anything, then the rationale for lock=False cannot have been because sqlite had no native upserts. Which means it must have been added as an optimization for the background update case on postgres?

[DMRobertson]

But if locking on sqlite has never done anything, then the rationale for lock=False cannot have been because sqlite had no native upserts.

Oops, sorry. That's probably me jumping to conclusions.

Which means it must have been added as an optimization for the background update case on postgres?

I suppose so! But to check: I don't think that suddenly makes it unsafe to remove the footgun?

[squahtx]

Yep, it's not unsafe, it just means this PR is rolling back a previous optimization we decided to add.

[squahtx]

Thanks for checking all the upserts. I hadn't realised that we always do an emulated upsert on user_directory_search when using sqlite.

[DMRobertson]

Thanks Sean. I think this LGTM. It'd be good to check with someone else on the team though---given that that this was my suggestion in the first place, I'd like to be reassured that we haven't missed any perf implications here. @erikjohnston perhaps?

[erikjohnston]

Why are we keeping this one conditional?

[squahtx]

It's to support simple_upsert_many_txn_emulated, which does its own locking beforehand.

synapse/synapse/storage/database.py

Lines 1499 to 1509 in 9cae44f

	if lock:
	# Lock the table just once, to prevent it being done once per row.
	# Note that, according to Postgres' documentation, once obtained,
	# the lock is held for the remainder of the current transaction.
	self.engine.lock_table(txn, "user_ips")
	for keyv, valv in zip(key_values, value_values):
	_keys = {x: y for x, y in zip(key_names, keyv)}
	_vals = {x: y for x, y in zip(value_names, valv)}
	self.simple_upsert_txn_emulated(txn, table, _keys, _vals, lock=False)