Replace register_noop_background_update with DELETE deltas

[clokep]

From a thread: #12899 (comment)

It seems that register_noop_background_update(...) could be replaced with DELETE FROM background_updates WHERE name = '....';

This would be nicer since we don't end up with dead code, register_noop_background_update was added in 6b02fc8 and from the conversation in #synapse-dev it seems like the DELETE approach was not considered.

[erikjohnston]

AHA, I think I remember why we didn't do this as a delta: there have been times where we've told people to manually insert data into background_updates to re-run those updates for whatever reason. If they do that after the delta to delete the stale background update has run then we'll get an entry stuck in that table forever.

Not sure how much we care about that though.

[richvdh]

AHA, I think I remember why we didn't do this as a delta: there have been times where we've told people to manually insert data into background_updates to re-run those updates for whatever reason. If they do that after the delta to delete the stale background update has run then we'll get an entry stuck in that table forever.

Have we ever done this for anything other than populate_stats_process_rooms and populate_stats_process_users (which we're not getting rid of)?

[erikjohnston]

AHA, I think I remember why we didn't do this as a delta: there have been times where we've told people to manually insert data into background_updates to re-run those updates for whatever reason. If they do that after the delta to delete the stale background update has run then we'll get an entry stuck in that table forever.

Have we ever done this for anything other than populate_stats_process_rooms and populate_stats_process_users (which we're not getting rid of)?

Not sure, probably not? I think we should probably still do this change, but we should remember that after this change its no longer safe to just blindly tell people to insert stuff back into background_updates.

[clokep]

The only use I found that might refer to that while putting together #12954 was:

synapse/synapse/storage/databases/main/stats.py

Lines 123 to 127 in 3c8f129

	# we no longer need to perform clean-up, but we will give ourselves
	# the potential to reintroduce it in the future – so documentation
	# will still encourage the use of this no-op handler.
	self.db_pool.updates.register_noop_background_update("populate_stats_cleanup")
	self.db_pool.updates.register_noop_background_update("populate_stats_prepare")

Those don't seem to be in the docs anymore though, so...

[clokep]

We also now have an admin API for starting background jobs that we expect folks to have to run manually sometimes: https://matrix-org.github.io/synapse/develop/usage/administration/admin_api/background_updates.html#run

The job_name there doesn't actually get inserted, it gets mapped to the true job names:

synapse/synapse/rest/admin/background_updates.py

Lines 125 to 147 in e24ff8e

	if job_name == "populate_stats_process_rooms":
	jobs = [("populate_stats_process_rooms", "{}", "")]
	elif job_name == "regenerate_directory":
	jobs = [
	("populate_user_directory_createtables", "{}", ""),
	(
	"populate_user_directory_process_rooms",
	"{}",
	"populate_user_directory_createtables",
	),
	(
	"populate_user_directory_process_users",
	"{}",
	"populate_user_directory_process_rooms",
	),
	(
	"populate_user_directory_cleanup",
	"{}",
	"populate_user_directory_process_users",
	),
	]
	else:
	raise SynapseError(HTTPStatus.BAD_REQUEST, "Invalid job_name")

I think this gives us a little bit of protection too?

[clokep]

@DMRobertson thought that we might ask people to insert some entries into this for the user directory, I believe we used to but now have the above admin commands.

I did find some references in our upgrade notes though:

https://github.com/matrix-org/synapse/blob/master/docs/upgrade.md#incorrect-database-migration-in-old-synapse-versions

These ask to insert the current_state_events_membership and populate_stats_process_rooms in the background update table. Neither of these are no-op right now.

I think this is safe to do, so I'm going to merge it. 👍

[DMRobertson]

@DMRobertson thought that we might ask people to insert some entries into this for the user directory, I believe we used to but now have the above admin commands.

Sorry, I should have mentioned this somewhere for completeness. I was thinking of here. It used to look like this and refer to a SQL migration file.