Code review comment for lp:~stewart/drizzle/remove-ddl-implicit-commit

Revision history for this message
Stewart Smith (stewart) wrote :

So it turns out we don't have a test for executing the SQL done by transaction reader in the test suite (we probably should have one).

The next issue is that we've historically had DDL wrapped in BEGIN and COMMIT to be forwards compatible for when we have DDL transactions... and this worked okay because we currently have had an implicit commit in DDL operations. With this patch, that goes away.

So we could either:
a) change the server to detect that the txn only can have DDL or DML in it
b) move all DDL ops to be within txn calls and the engines have to throw errors themselves (what I've viewed to be the eventual solution)
c) change transaction_reader to optionally (and at least at the moment by default) not wrap DDL in BEGIN/COMMIT (or use SET AUTOCOMMIT=0, which makes it magically just work)

I think i'm preferring (c) with SET AUTOCOMMIT=0 for DDL only txns.

« Back to merge proposal