New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
db: fix SQL TRANSACTION syntax compatible with all DB backends #3636
base: main
Are you sure you want to change the base?
db: fix SQL TRANSACTION syntax compatible with all DB backends #3636
Conversation
I wonder if this shouldn't be approached in a different way. There is C API (driver specific) for this ( Anyways, it should have been |
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 |
Yes you are right we can refactor it to better implementation on the low C lang level. |
SQL TRANSACTION syntax: ``` BEGIN; ...; COMMIT; ```
ad77c20
to
6e66ee9
Compare
Injecting sql code in grass/gui/wxpython/vdigit/wxdigit.py Line 575 in 7413740
or grass/gui/wxpython/vdigit/wxdigit.py Line 1246 in 7413740
but that will require quite a lot of rewriting. But you could perhaps use: grass/python/grass/script/db.py Line 235 in 7413740
|
…fter" This reverts commit 06bbc23.
…ansaction() func To use C API db_begin_transaction(), db_commit_transaction(). Used by modules: - db.dropcolumn - v.db.addcolumn - v.db.dropcolumn - v.db.join.py - v.db.univar - v.dissolve - v.rast.stats
…it_transaction() and release opened driver/db connection
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 |
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. |
@@ -122,9 +122,15 @@ def main(): | |||
sql = f'ALTER TABLE {table} DROP COLUMN "{column}"' | |||
|
|||
try: | |||
pdriver = db_begin_transaction(driver_name=driver, database=database) | |||
grass.write_command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not db_execute
as in other cases?
Proper prepared statements are way to go but lets take one step at a time (that would require a new C and Python API).
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. |
SQL TRANSACTION syntax (compatible with all DB backends):
This default SQL TRANSACTION syntax is not compatible with MySQL/MariaDB DB backend:
Successfully tested with SQLite, PostgreSQL, MySQL DB backend.