Skip to content
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

grass_dbmidriver: Support no locking for SQLite driver over NFS #1667

Merged
merged 3 commits into from
Apr 11, 2022

Conversation

metzm
Copy link
Contributor

@metzm metzm commented Jun 20, 2021

From https://sqlite.org/faq.html: "file locking is broken on many NFS implementations"
This causes db operations to be extremely slow or not working at all if the GRASS SQLite db is located on a NFS drive.

This PR attempts to solve this issue for affected NFS implementations.

The NFS implementations I have access to are all working fine wrt SQLite, therefore additional testing is needed.

This is not a bug in GRASS, but a bug in certain combinations of SQLite + NFS.

@metzm metzm added the enhancement New feature or request label Jun 20, 2021
@metzm metzm added this to the 8.0.0 milestone Jun 20, 2021
@metzm metzm requested a review from neteler June 20, 2021 19:55
@metzm
Copy link
Contributor Author

metzm commented Jun 21, 2021

How to test:

  • have a GRASS db on some NFS drive where db operations are slow (e.g. import of vector points, v.db.update)
  • set the env var GRASS_SQLITE_NOLOCK to some number not 0 (zero)
  • look for the message "Disabling SQLite locking" and check if db operations are now faster

increase buffer for enlarged file name
@neteler neteler modified the milestones: 8.0.0, 8.0.1 Jan 12, 2022
@wenzeslaus wenzeslaus modified the milestones: 8.0.1, 8.0.2, 8.4.0 Feb 23, 2022
@metzm metzm marked this pull request as ready for review April 11, 2022 20:48
@metzm metzm merged commit 0729796 into OSGeo:main Apr 11, 2022
@metzm metzm deleted the sqlite_nfs branch April 11, 2022 20:51
@wenzeslaus wenzeslaus modified the milestones: 8.4.0, 8.2.0 Apr 11, 2022
@neteler
Copy link
Member

neteler commented Apr 14, 2022

Would it make sense to add this test?

The message Disabling SQLite locking appears:

diff --git a/scripts/v.db.update/testsuite/test_v_db_update.py b/scripts/v.db.update/testsuite/test_v_db_update.py
index 243ea71a3a..9357377a0e 100644
--- a/scripts/v.db.update/testsuite/test_v_db_update.py
+++ b/scripts/v.db.update/testsuite/test_v_db_update.py
@@ -4,6 +4,7 @@ Created on Sun Jun 08 22:14:26 2018
 @author: Sanjeet Bhatti
 """
 
+import os
 from grass.gunittest.case import TestCase
 from grass.gunittest.main import test
 from grass.gunittest.gmodules import SimpleModule
@@ -40,6 +41,19 @@ class TestVDbUpdate(TestCase):
         )
         self.assertModule(module)
 
+    def test_update_no_locking(self):
+        """update value test with GRASS_SQLITE_NOLOCK (https://github.com/OSGeo/grass/pull/1667)"""
+        run_command("v.db.addcolumn", map=self.mapName, column="zval2 double precision")
+        os.environ["GRASS_SQLITE_NOLOCK"] = "1"
+        module = SimpleModule(
+            "v.db.update",
+            map=self.mapName,
+            column="zval2",
+            query_column="CAST(z_value AS double precision)",
+            where="z_value <> 'N/A'",
+        )
+        self.assertModule(module)
+
 
 if __name__ == "__main__":
     test()

1 similar comment
@neteler
Copy link
Member

neteler commented Apr 14, 2022

Would it make sense to add this test?

The message Disabling SQLite locking appears:

diff --git a/scripts/v.db.update/testsuite/test_v_db_update.py b/scripts/v.db.update/testsuite/test_v_db_update.py
index 243ea71a3a..9357377a0e 100644
--- a/scripts/v.db.update/testsuite/test_v_db_update.py
+++ b/scripts/v.db.update/testsuite/test_v_db_update.py
@@ -4,6 +4,7 @@ Created on Sun Jun 08 22:14:26 2018
 @author: Sanjeet Bhatti
 """
 
+import os
 from grass.gunittest.case import TestCase
 from grass.gunittest.main import test
 from grass.gunittest.gmodules import SimpleModule
@@ -40,6 +41,19 @@ class TestVDbUpdate(TestCase):
         )
         self.assertModule(module)
 
+    def test_update_no_locking(self):
+        """update value test with GRASS_SQLITE_NOLOCK (https://github.com/OSGeo/grass/pull/1667)"""
+        run_command("v.db.addcolumn", map=self.mapName, column="zval2 double precision")
+        os.environ["GRASS_SQLITE_NOLOCK"] = "1"
+        module = SimpleModule(
+            "v.db.update",
+            map=self.mapName,
+            column="zval2",
+            query_column="CAST(z_value AS double precision)",
+            where="z_value <> 'N/A'",
+        )
+        self.assertModule(module)
+
 
 if __name__ == "__main__":
     test()

@wenzeslaus wenzeslaus changed the title sqlite driver: support no locking for sqlite over nfs grass_dbmidriver: Support no locking for SQLite driver over NFS Apr 27, 2022
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Oct 26, 2022
* sqlite nfs no lock env var added
* increase buffer for enlarged file name
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
* sqlite nfs no lock env var added
* increase buffer for enlarged file name
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
* sqlite nfs no lock env var added
* increase buffer for enlarged file name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants