SQLAlchemy: Use server-side now() function for "autoincrement" columns

[amotl]

Now uses the server-side now() function to patch column models using the "autoincrement" property, as suggested by @seut at crate/sqlalchemy-cratedb#77. Thanks!

[codecov]

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed	Coverage
mlflow_cratedb/patch/crate_python.py	100.00%

📢 Thoughts on this report? Let us know!.

[amotl]

That's a fluke, possibly because of too less granularity in the time domain when only using millisecond resolution. However, this visited the test suite on CI already on a previous occasion, see crate/crate-python#11, so it is not holding me back on merging this.

-- https://github.com/crate-workbench/mlflow-cratedb/actions/runs/6224569395/job/16893059034?pr=24#step:6:175

[hammerhead]

Does autoincrement in SQLAlchemy guarantee uniqueness? Although unlikely in practice, using NOW() makes it possible that two concurrent INSERT statements assign the same value to such a column?

[amotl]

Does autoincrement in SQLAlchemy guarantee uniqueness?

It will have the same guarantees what the relevant DBMS has to offer. In the case of PostgreSQL, it is implemented by its "sequences" subsystem. I think it will guarantee one the highest levels of uniqueness, because, as a feature typically used for generating primary keys, it is used by all kinds of DBMS all over the world for this purpose.

Although unlikely in practice, using NOW() makes it possible that two concurrent INSERT statements assign the same value to such a column?

You are right, but this is not unlikely at all, and can easily be invoked by corresponding test cases simulating high concurrency, what MLflow actually does, that's why it is occasionally tripping. This is why I also was proposing an alternative client-side approach with microsecond resolution at crate/sqlalchemy-cratedb#77, to lower the chance of collisions. However, in this case, the outcome doesn't show any significant difference, and works in 95% of the test runs.

[amotl]

Another test run on this patch worked well, so it can't be that wrong.