Ignore or override algorithm=inplace when not supported

[neilgarb]

At the moment you can do MySQL online DDL operations by setting StatementModifiers.Algorithm to "inplace". That's OK for most alters, but some alters aren't supported, e.g. dropping a primary key (https://dev.mysql.com/doc/refman/5.7/en/innodb-online-ddl-operations.html).

Unfortunately, TableDiff.alterClauses is unexported, so it's not easy to determine if algorithm=inplace is supported.

It would be useful either:

1. To export alterClauses so that the user can decide if the operation can be done online, or
2. To override algorithm=inplace if the library determines it's not supported.

Thanks!

[evanelias]

To override algorithm=inplace if the library determines it's not supported.

Thank you for submitting an issue! If I understand your use-case correctly, I believe you can already get this behavior; it's just not explained well in the docs / code comments. The trick is to just leave StatementModifiers.AlgorithmClause and StatementModifiers.LockClause as empty strings. This will omit the ALGORITHM and LOCK clauses from the generated ALTER, causing MySQL to use its default behavior -- which actually is to use online DDL automatically if all of the clauses are online-DDL-compatible.

It's somewhat unintuitive, but the purpose of ALGORITHM=inplace, LOCK=none in MySQL is to require online DDL, with an immediate error in cases where online DDL is unsupported. So the intended use-case for StatementModifiers.AlgorithmClause and StatementModifiers.LockClause in this package is for situations where you want the DDL to either run online or not run at all. If you just prefer online DDL, but it's not a strict requirement, simply omit these fields.

That said, I do agree it could be useful for this package to eventually have a mapping of which ALTER clauses are online-compatible in each database version/flavor. That would provide a way to check in advance if online DDL is supported, rather than relying on the db to return an error when the DDL is attempted to be executed. This is somewhat low-pri on my end, but I’d happily accept a PR for it.

To export alterClauses so that the user can decide

Speaking more generally (since a few folks have asked about this design decision), I'm hoping to keep these unexported intentionally. Conceptually I'd like to have the StatementModifiers fields act as the singular API for configuration knobs into how the DDL is generated. I'm open to reconsidering that though, if there's a compelling use-case.

[neilgarb]

@evanelias Thanks for the considered response. Our in-house DB migration tool uses the presence or absence of algorithm=inplace to determine the "riskiness" of the query. If algorithm=inplace is present, the disruption from running the query will be lower, and so we can either run the migration in production automatically, or give a wider group of people permission to run the migration manually. If algorithm=inplace is absent, we need to be a bit more careful, because the query may cause a disruption.

Your points are taken, though. We'd still be able to add algorithm=inplace at the engineer's discretion after the diff has been generated.

[evanelias]

That makes sense, thanks. I'll keep this open as a feature request for adding a mapping of which clauses are inplace-compatible.

For now, a programmatic work-around would be to run the target's CREATE TABLE in a temporary location, and then attempt to run the relevant ALTER on it with algorithm=inplace, and see whether or not that errors. Skeema's subpackages have a lot of existing methods which would make this easy to do, but that might be difficult to integrate if you're using Tengo stand-alone with a totally custom migration tool.

[evanelias]

Sorry I wasn't able to implement this! Closing ahead of archiving this repo as per https://github.com/skeema/tengo/blob/main/README.md