Make sqlite database migrations transactional again, part two

[squahtx]

#14910 fixed the regression introduced by #13873 where sqlite database
migrations would no longer run inside a transaction. However, it
committed the transaction before Synapse updated its bookkeeping of
which migrations have been run, which means that migrations may be run
again after they have completed successfully.

Leave the transaction open at the end of executescript, to restore the
old, correct behaviour. Also make the PostgreSQL behaviour consistent
with SQLite.

Fixes #14909.

Signed-off-by: Sean Quah [email protected]

[DMRobertson]

LGTM, modulo a few questions:

[DMRobertson]

Some database engines may automatically COMMIT the ongoing transaction

I wonder if we should make this a hard guarantee of this function, so that we have the same semantics across postgres and sqlite.

For example, suppose we need to apply two migrations A and B to upgrade the database schema. IIUC, under postgres, synapse will run

BEGIN TRANSACTION;
...
<A>;
<B>;
COMMIT;

whereas under sqlite we will now run

BEGIN TRANSACTION;
...
COMMIT; BEGIN TRANSACTION;
<A>;
COMMIT; BEGIN TRANSACTION;
<B>;
COMMIT;

I'm not sure if that's a difference we should worry about. WDYT?

[squahtx]

I'm in favour of the idea. It's much easier to reason about things when both database engines behave the same way.

I gave that a go in bb340f7. Let's hope it passes CI.

[DMRobertson]

SGTM!