Tidy up and type-hint the database engine modules

[DMRobertson]

A Friday indulgence.

We tell the engines which module to use as a database driver. I wanted to change this because a) it seems weird, and b) my secret ulterior motive is to add yet more type coverage to Synapse.

Closes #12724.

[DMRobertson]

poor commit hygiene.

[DMRobertson]

I dislike importing this here rather than at the top level. But this was the least-bad/easiest way I could see to not require existing installations to install psycopg2.

[DMRobertson]

If accepted, python/typeshed#7834 would make this cast redundant.

[DMRobertson]

I can see that one might want database config that applies independently of your choice of DB (or DB driver, for that matter). So I don't mind dropping this change if people aren't convinced.

[squahtx]

I can see that one might want database config that applies independently of your choice of DB (or DB driver, for that matter).

Given your reasoning, I'm in favour of leaving the config parameter in, even if we don't use it currently.

[DMRobertson]

Agreed. 4d8b840 reverts this.

[DMRobertson]

my secret ulterior motive is to add yet more type coverage to Synapse.

On that front:

$ mypy -p synapse --disallow-untyped-defs | tail -n 1
Found 60 errors in 8 files (checked 487 source files)

This excludes all the tests, of course...

There are also 8 other functions in synapse whose modules have been exempted in mypy.ini.

[squahtx]

Note that we'll be running the import on every database transaction. I think it's fine?
Reimporting a previously imported module didn't seem to take too long in a very unscientific test using the repl:

>>> t0 = time.time(); import psycopg2.extensions; time.time() - t0
1.0013580322265625e-05 (~10 microseconds, worst number)
>>> t0 = time.time(); time.time() - t0
2.384185791015625e-06 (~2.5 microseconds, worst number)

[DMRobertson]

I'd call it "ugly, but fine".

The obstacle blocking us from importing psycopg2 freely in this module is that we except to be able to import PostgresEngine everywhere, even if psycopg2 is not available. For example:

synapse/synapse/storage/databases/main/events_worker.py

Line 181 in 01e6255

if isinstance(database.engine, PostgresEngine):

synapse/synapse/storage/databases/state/bg_updates.py

Line 102 in 3e0536c

if isinstance(self.database_engine, PostgresEngine):

synapse/synapse/storage/databases/main/relations.py

Line 468 in 75dff3d

if isinstance(self.database_engine, PostgresEngine):

I mulled over doing something like

• rename postgres.py to _postgres.py
• create a new file postgres.py with contents like this:

  try:
      from ._postgres import PostgresEngine
  except ImportError:
      class PostgresEngine: pass
  
  __all__ = ["PostgresEngine"] 

But I'm not sure how well Pycharm and mypy will handle that. Other ideas:

• have DatabaseEngine have some kind of "engine_kind" attribute which points to an enum. Felt like duplicating information we already know, and just generally gave me a smelly impression.
• make psycopg2 a mandatory requirement for Synapse, That'd be my preferred option, but I suspect it'd be controversial.

[clokep]

@DMRobertson another idea would be to add more flags to database engine that talk about the features as opposed to the specific class. E.g. it looks like those need (respectively):

• Sequence support
• RECURSIVE support
• DISTINCT ON support

E.g. expand the features listed near

synapse/synapse/storage/engines/postgres.py

Lines 151 to 166 in 86a515c

	@property
	def can_native_upsert(self):
	"""
	Can we use native UPSERTs?
	"""
	return True
	@property
	def supports_using_any_list(self):
	"""Do we support using `a = ANY(?)` and passing a list"""
	return True
	@property
	def supports_returning(self) -> bool:
	"""Do we support the `RETURNING` clause in insert/update/delete?"""
	return True

[DMRobertson]

That's a good idea, though I think it'd be tricky to retrofit those in across the source tree without being familiar with the differences between the two databases.

(Obligatory grumble: I don't want us to be rewriting and maintaining our own version of SQLAlchemy core)

[clokep]

Completely fair, not trying to scope creep! 👍 I've thought about doing this in the past also!

[squahtx]

I can see that one might want database config that applies independently of your choice of DB (or DB driver, for that matter).

Given your reasoning, I'm in favour of leaving the config parameter in, even if we don't use it currently.

[squahtx]

Everything else makes sense!

[squahtx]

LGTM! Thanks for writing documentation for the DBABI interface.

I've some really nitty stylistic suggestions that are roughly:

• unabbreviate OS and DB (I have trauma from C codebases)
• backtick class names
• s/you/we/
• make every example a sentence ending in a full stop for consistency