db: fix SQL TRANSACTION syntax compatible with all DB backends

[tmszi]

SQL TRANSACTION syntax (compatible with all DB backends):

BEGIN;
...;
COMMIT;

This default SQL TRANSACTION syntax is not compatible with MySQL/MariaDB DB backend:

BEGIN TRANSACTION;
...;
END TRANSACTION;

Successfully tested with SQLite, PostgreSQL, MySQL DB backend.

[nilason]

I wonder if this shouldn't be approached in a different way. There is C API (driver specific) for this (db_begin_transaction(), db_commit_transaction()).

Anyways, it should have been START TRANSACTION and COMMIT, not BEGIN TRANSACTION and END TRANSACTION.

[tmszi]

Anyways, it should have been START TRANSACTION and COMMIT, not BEGIN TRANSACTION and END TRANSACTION.

or BEGIN as alias for START TRANSACTION.

[nilason]

Anyways, it should have been START TRANSACTION and COMMIT, not BEGIN TRANSACTION and END TRANSACTION.

or BEGIN as alias for START TRANSACTION.

Yes, for the drivers that support it. You may take a look at how this is implemented for the different drivers in db/drivers/*/execute.c files.

[tmszi]

I wonder if this shouldn't be approached in a different way. There is C API (driver specific) for this (db_begin_transaction(), db_commit_transaction()).

Yes you are right we can refactor it to better implementation on the low C lang level.

[nilason]

Injecting sql code in db.execute breaks current behaviour, so that is not a good option. I was originally thinking of directly use C API in Python, like in: https://github.com/OSGeo/grass/blob/7413740dd81c11c2909ad10e7c3161fc097088f9/gui/wxpython/vdigit/wxdigit.py#L575 or https://github.com/OSGeo/grass/blob/7413740dd81c11c2909ad10e7c3161fc097088f9/gui/wxpython/vdigit/wxdigit.py#L1246

but that will require quite a lot of rewriting.

But you could perhaps use: https://github.com/OSGeo/grass/blob/7413740dd81c11c2909ad10e7c3161fc097088f9/python/grass/script/db.py

https://github.com/OSGeo/grass/blob/7413740dd81c11c2909ad10e7c3161fc097088f9/python/grass/script/db.py#L235

[nilason]

I like where this is going. Something like this is what I had in mind, although it is way beyond the original intent of this PR. But I don't mind. Allows for more flexibility and control using C API directly, than via the module db.execute. Not sure what to do about the change of behaviour of db_commit_transaction(API break). I can also imagine making this a class, reducing the need to pass driver, database etc to every function call, and possibility to push sql commands (instead of building a string)... but before you put more work into this let us ask for the general opinion of this change by e.g. @petrasovaa, @wenzeslaus, @marisn.

[wenzeslaus]

I also would like to replace putting the strings together by something better given our C database API and Python database APIs. While alternatives exists and some are employed in the code already, using the C library directly through C types is a straightforward option.

The API change is a potential issue. How big, I don't know, I'm guessing not big. Still, breaking it seems wrong. You also mentioned the issue of passing multiple parameters. I would certainly try how this would look as a class. This would be one way to avoid the change in the API. Perhaps following PEP 249 – Python Database API Specification or mimicking relevant parts of an existing SQL database Python API (Psycopg, sqlite3, SQLAlchemy, ...) would be good. Ideally, such API would actually hide whatever is happening in the background (constructing string for db.execute, passing to a native wrapper like sqlite3 or passing directly, or through a subprocess, to C API).

While when ctypes works, everything is just perfect, the experience is that something is often missing from the things which need to be in place for C API to work in Python. Hence, we have all the lazy imports in this PR and elsewhere. That's a thing which can be gradually improved, though. The many updates to ctypes lately were certainly helpful. Switching to Filesystem Hierarchy Standard might be another update which would help here ([GRASS-dev] GRASS 8.0 support in GDAL and QGIS).

I'm curious what others think.

[marisn]

I also would like to replace putting the strings together by something better given our C database API and Python database APIs. While alternatives exists and some are employed in the code already, using the C library directly through C types is a straightforward option.

Proper prepared statements are way to go but lets take one step at a time (that would require a new C and Python API).

The API change is a potential issue. How big, I don't know, I'm guessing not big. Still, breaking it seems wrong.

This wouldn't be the first time when we break API on a minor release. Taking into account that this is not the most commonly used functionality, it fixes compatibility with one of our supported back-ends, I would not cry over this.

[echoix]

Is this still planned for the due 8.4.0 release? If so, what is missing?

[nilason]

https://github.com/OSGeo/grass/pull/3636/commits/093c461380326dccd897db142fc603d50aea2490: great initiative!

[wenzeslaus]

While very valuable, we can't fit this into 8.4, moving milestone to 8.5. I hope that's okay.

[tmszi]

All suggestions was incorporated into commit no https://github.com/OSGeo/grass/pull/3636/commits/072d0be7622dc177d6e5c993017577060be80923.

[echoix]

There's now some conflicts to solve here