Generalize the current database drivers further to minimise engine-specific behaviour

[ShadowJonathan]

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.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <[email protected]>

[ShadowJonathan]

(I invite people to look at the added todo items and give suggestions wherever possible)

[clokep]

My quick thought on this is that I'm not sure that this really generalizes things, the PR could use a more in-depth description of what this is accomplishing and why.

It seems to mostly be swapping out an isinstance check for a property saying whether something is postgres or sqlite. I understand this is related to #9294 -- would it be possible to use the same PostgresEngine wrapper for that? If not, then why?

[ShadowJonathan]

It wont be possible to use the same wrapper, the PostgresEngine isinstance check comes with the assumption that it'll either be the psycopg2 driver, or the sqlite3 driver, we need to generalise this to the point of "oh, what SQL dialect do i need to use?" (postgresql or sqlite), some of these isinstance checks were left in because the branch started using psycopg2-specific logic.

This PR is aimed to make the codebase more intentionally oblivious which exact driver is being used at all times, and removing assumptions it'll then either be a binary of psycopg2 or sqlite3 passing through, so it needs to first be generalized to the two SQL dialects coming up (postgresql and sqlite), with driver-specific logic having fallbacks that don't assume the binary of drivers.

TL;DR: Instead of

if isinstance(engine, PostgresEngine):
  # assuming postgres with psycopg2
else:
  # assuming sqlite(!) with sqlite3(!)

I want

if engine.sql_type.is_postgres():
  # assuming driver-neutral postgres
elif engine.sql_type.is_sqlite():
  # assuming driver-neutral sqlite
else:
  # different sql type
  raise RuntimeError("SQL type not supported")

which is more explicit and less bug-prone, it fails fast, and is easy to search for when adding support for new SQL databases (if that'd happen)

Edit:

Explicit is better than implicit.

• Python Zen

[ShadowJonathan]

Question; does MultiWriterIdGenerator use psycopg2-specific assumptions? e.g. connections being thread-safe and/or psycopg2-specific methods/behaviour.

[ShadowJonathan]

Closing this PR as the changes need to be less vague and targeted, i'll come back to this later (hopefully).