let update_synapse_database run on a multi-database configurations

[thefinn93]

The update_synapse_database script checks for the presence of the database key in the config. When multiple databases are used, the rest of synapse expects a different key, databases, which is mutually exclusive with database. this fixes the check to allow either key.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[clokep]

I think we do similar (and more thorough) error checking when we parse the config anyway... I wonder if we can just get rid of these lines of code?

synapse/synapse/config/database.py

Lines 97 to 123 in 493c2fc

	multi_database_config = config.get("databases")
	database_config = config.get("database")
	database_path = config.get("database_path")
	if multi_database_config and database_config:
	raise ConfigError("Can't specify both 'database' and 'databases' in config")
	if multi_database_config:
	if database_path:
	raise ConfigError("Can't specify 'database_path' with 'databases'")
	self.databases = [
	DatabaseConnectionConfig(name, db_conf)
	for name, db_conf in multi_database_config.items()
	]
	if database_config:
	self.databases = [DatabaseConnectionConfig("master", database_config)]
	if database_path:
	if self.databases and self.databases[0].name != "sqlite3":
	logger.warning(NON_SQLITE_DATABASE_PATH_WARNING)
	return
	database_config = {"name": "sqlite3", "args": {}}
	self.databases = [DatabaseConnectionConfig("master", database_config)]
	self.set_databasepath(database_path)

[thefinn93]

I wasn't clear what this check was for. it does seem like there is another check lower down. Should I remove it?

[thefinn93]

I went ahead and removed it

[clokep]

Yeah, I think it is fine to remove it. The error message seems pretty much the same too.

[clokep]

Putting this back in the queue since I've lost all state on it and likely won't get to it soon.

[squahtx]

Does this mean we no longer error when both database and databases are missing?

Also does anything rely on the exit code being 4?

[clokep]

I think we do this checking at:

synapse/synapse/config/database.py

Lines 97 to 123 in 493c2fc

	multi_database_config = config.get("databases")
	database_config = config.get("database")
	database_path = config.get("database_path")
	if multi_database_config and database_config:
	raise ConfigError("Can't specify both 'database' and 'databases' in config")
	if multi_database_config:
	if database_path:
	raise ConfigError("Can't specify 'database_path' with 'databases'")
	self.databases = [
	DatabaseConnectionConfig(name, db_conf)
	for name, db_conf in multi_database_config.items()
	]
	if database_config:
	self.databases = [DatabaseConnectionConfig("master", database_config)]
	if database_path:
	if self.databases and self.databases[0].name != "sqlite3":
	logger.warning(NON_SQLITE_DATABASE_PATH_WARNING)
	return
	database_config = {"name": "sqlite3", "args": {}}
	self.databases = [DatabaseConnectionConfig("master", database_config)]
	self.set_databasepath(database_path)

Maybe I missed something though!

[DMRobertson]

I think

• this seems sane and worthwhile
• I think it's unlikely anything is relying on error code 4
• Patrick's comments about the config parser enforcing that we have databases xor database fields seem sane to me too.

I'd like to merge it.

@thefinn93 were you able to test this change? I'd just like some reassurance that this works. (I can't see why it wouldn't---we call hs.setup() just like we would at startup).

[thefinn93]

yes, this change was tested and didn't seem to cause any issues