OGR provider: only enable journal_mode = wal when editing. requires G…

…DAL >= 3.4.2 (fixes qgis#23991)

This requires the NOLOCK open option of the GPKG driver added per OSGeo/gdal#5207
For earlier GDAL version, previous behaviour is kept.

With GDAL >= 3.4.2, when creating a QgsOgrProvider object, we first open
it in update mode without forcing WAL, to get the appropriate
capabilities, close it, and re-open it in read-only mode with the
NOLOCK=YES open option. This option will only be honoured if the file
wasn't already in WAL mode.
When editing a layer, the file is re-opened in update mode and with
enabling WAL to avoid blocking between readers and writers.
When closing a file, journal mode is attempted to be reset to DELETE as
before.

I've verified that test_provider_ogr_gpkg.py and test_provider_ogr.py
pass locally with GDAL master + OSGeo/gdal#5207

src/core/providers/ogr/qgsogrprovider.cpp

@@ -3470,15 +3470,28 @@ void QgsOgrProvider::open( OpenMode mode )
   else if ( mode == OpenModeSameAsCurrent && !mWriteAccess )
     openReadOnly = true;
 
+  const bool bIsGpkg = QFileInfo( mFilePath ).suffix().compare( QLatin1String( "gpkg" ), Qt::CaseInsensitive ) == 0;
+
   // first try to open in update mode (unless specified otherwise)
   QString errCause;
   if ( !openReadOnly )
   {
     QStringList options( mOpenOptions );
-    if ( mode == OpenModeForceUpdateRepackOff || ( mDeferRepack && OpenModeSameAsCurrent ) )
+    if ( !bIsGpkg && ( mode == OpenModeForceUpdateRepackOff || ( mDeferRepack && OpenModeSameAsCurrent ) ) )
     {
       options << "AUTO_REPACK=OFF";
     }
+
+#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3,4,2)
+    if ( bIsGpkg && mode == OpenModeInitial )
+    {
+      // A hint to QgsOgrProviderUtils::GDALOpenWrapper() to not force WAL
+      // as in OpenModeInitial we are not going to do anything besides getting capabilities
+      // and re-opening in readonly mode.
+      options << "DO_NOT_ENABLE_WAL=ON";
+    }
+#endif
+
     // We get the layer which was requested by the uri. The layername
     // has precedence over the layerid if both are given.
     if ( !mLayerName.isNull() )
@@ -3592,10 +3605,16 @@ void QgsOgrProvider::open( OpenMode mode )
 
   // For shapefiles or MapInfo .tab, so as to allow concurrent opening between
   // QGIS and MapInfo, we go back to read-only mode for now.
+  // For GPKG too, so to open in read-only nolock mode for GDAL >= 3.4.2
   // We limit to those drivers as re-opening is relatively cheap (other drivers
   // like GeoJSON might do full content ingestion for example)
   if ( mValid && mode == OpenModeInitial && mWriteAccess &&
-       ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) || mGDALDriverName == QLatin1String( "MapInfo File" ) ) )
+       ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) ||
+         mGDALDriverName == QLatin1String( "MapInfo File" )
+#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3,4,2)
+         || mGDALDriverName == QLatin1String( "GPKG" )
+#endif
+       ) )
   {
     mOgrSqlLayer.reset();
     mOgrOrigLayer.reset();
@@ -3613,7 +3632,7 @@ void QgsOgrProvider::open( OpenMode mode )
     // try to open read-only
     if ( !mLayerName.isNull() )
     {
-      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, mOpenOptions, mLayerName, errCause, true );
+      mOgrOrigLayer = QgsOgrProviderUtils::getLayer( mFilePath, false, mOpenOptions,  mLayerName, errCause, true );
     }
     else
     {
@@ -3652,6 +3671,9 @@ void QgsOgrProvider::open( OpenMode mode )
 
 void QgsOgrProvider::close()
 {
+  if ( mWriteAccess && mForceRecomputeExtent )
+    extent();
+
   mOgrSqlLayer.reset();
   mOgrOrigLayer.reset();
   mOgrLayer = nullptr;

src/core/providers/ogr/qgsogrproviderutils.cpp

@@ -999,17 +999,34 @@ GDALDatasetH QgsOgrProviderUtils::GDALOpenWrapper( const char *pszPath, bool bUp
 
   bool bIsGpkg = QFileInfo( filePath ).suffix().compare( QLatin1String( "gpkg" ), Qt::CaseInsensitive ) == 0;
   bool bIsLocalGpkg = false;
+
+#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3,4,2)
+  if ( bIsGpkg && !bUpdate )
+  {
+    papszOpenOptions = CSLSetNameValue( papszOpenOptions, "NOLOCK", "ON" );
+  }
+#endif
+
   if ( bIsGpkg &&
        IsLocalFile( filePath ) &&
        !CPLGetConfigOption( "OGR_SQLITE_JOURNAL", nullptr ) &&
        QgsSettings().value( QStringLiteral( "qgis/walForSqlite3" ), true ).toBool() )
   {
-    // For GeoPackage, we force opening of the file in WAL (Write Ahead Log)
-    // mode so as to avoid readers blocking writer(s), and vice-versa.
-    // https://www.sqlite.org/wal.html
-    // But only do that on a local file since WAL is advertised not to work
-    // on network shares
-    CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
+    // Starting with GDAL 3.4.2, QgsOgrProvider::open(OpenModeInitial) will set
+    // a fake DO_NOT_ENABLE_WAL=ON when doing the initial open in update mode
+    // to indicate that we should not enable WAL.
+    // And NOLOCK=ON will be set in read-only attempts.
+    // Only enable it when enterUpdateMode() has been executed.
+    if ( CSLFetchNameValue( papszOpenOptions, "DO_NOT_ENABLE_WAL" ) == nullptr &&
+         CSLFetchNameValue( papszOpenOptions, "NOLOCK" ) == nullptr )
+    {
+      // For GeoPackage, we force opening of the file in WAL (Write Ahead Log)
+      // mode so as to avoid readers blocking writer(s), and vice-versa.
+      // https://www.sqlite.org/wal.html
+      // But only do that on a local file since WAL is advertised not to work
+      // on network shares
+      CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
+    }
     bIsLocalGpkg = true;
   }
   else if ( bIsGpkg )
@@ -1019,6 +1036,11 @@ GDALDatasetH QgsOgrProviderUtils::GDALOpenWrapper( const char *pszPath, bool bUp
     CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "DELETE" );
   }
 
+  if ( CSLFetchNameValue( papszOpenOptions, "DO_NOT_ENABLE_WAL" ) != nullptr )
+  {
+    papszOpenOptions = CSLSetNameValue( papszOpenOptions, "DO_NOT_ENABLE_WAL", nullptr );
+  }
+
   bool modify_OGR_GPKG_FOREIGN_KEY_CHECK = !CPLGetConfigOption( "OGR_GPKG_FOREIGN_KEY_CHECK", nullptr );
   if ( modify_OGR_GPKG_FOREIGN_KEY_CHECK )
   {

src/core/vector/qgsvectorlayer.cpp

@@ -1477,10 +1477,10 @@ bool QgsVectorLayer::startEditing()
     return false;
   }
 
-  emit beforeEditingStarted();
-
   mDataProvider->enterUpdateMode();
 
+  emit beforeEditingStarted();
+
   if ( mDataProvider->transaction() )
   {
     mEditBuffer = new QgsVectorLayerEditPassthrough( this );

tests/src/python/providertestbase.py

@@ -922,6 +922,9 @@ def testChangeAttributesConstraintViolation(self):
         self.assertFalse(
             result, 'Provider reported success when changing an attribute value that violates a DB level CHECK constraint')
 
+        if getattr(self, 'stopEditableLayerWithCheckConstraint', None):
+            self.stopEditableLayerWithCheckConstraint()
+
     def testUniqueNotNullConstraints(self):
         """Test provider-level NOT NULL and UNIQUE constraints, to enable
         this test, implement getEditableLayerWithUniqueNotNullConstraints

tests/src/python/test_provider_ogr_gpkg.py

@@ -44,7 +44,7 @@
                        QgsVectorDataProvider,
                        QgsLayerMetadata,
                        NULL)
-from qgis.PyQt.QtCore import QCoreApplication, QVariant, QDate, QTime, QDateTime, Qt, QTemporaryDir
+from qgis.PyQt.QtCore import QCoreApplication, QVariant, QDate, QTime, QDateTime, Qt, QTemporaryDir, QFileInfo
 from qgis.PyQt.QtXml import QDomDocument
 from qgis.testing import start_app, unittest
 from qgis.utils import spatialite_connect
@@ -92,6 +92,7 @@ def setUpClass(cls):
         # Create the other layer for constraints check
         cls.check_constraint = QgsVectorLayer(
             cls.basetestfile + '|layername=check_constraint', 'check_constraint', 'ogr')
+        cls.check_constraint_editing_started = False
 
         # Create the other layer for unique and not null constraints check
         cls.unique_not_null_constraints = QgsVectorLayer(
@@ -121,9 +122,17 @@ def getSource(self):
 
     def getEditableLayerWithCheckConstraint(self):
         """Returns the layer for attribute change CHECK constraint violation"""
-
+        if not self.check_constraint_editing_started:
+            self.assertFalse(self.check_constraint_editing_started)
+            self.check_constraint_editing_started = True
+            self.check_constraint.startEditing()
         return self.check_constraint
 
+    def stopEditableLayerWithCheckConstraint(self):
+        self.assertTrue(self.check_constraint_editing_started)
+        self.check_constraint_editing_started = False
+        self.check_constraint.commitChanges()
+
     def getEditableLayerWithUniqueNotNullConstraints(self):
         """Returns the layer for UNIQUE and NOT NULL constraints detection"""
 
@@ -724,6 +733,16 @@ def testStyle(self):
         self.assertEqual(2, vl.dataProvider().subLayerCount())
         self.assertEqual(len(sublayers), 2, sublayers)
 
+    @staticmethod
+    def _getJournalMode(filename):
+        ds = ogr.Open(filename)
+        lyr = ds.ExecuteSQL('PRAGMA journal_mode')
+        f = lyr.GetNextFeature()
+        res = f.GetField(0)
+        ds.ReleaseResultSet(lyr)
+        ds = None
+        return res
+
     def testDisablewalForSqlite3(self):
         ''' Test disabling walForSqlite3 setting '''
         QgsSettings().setValue("/qgis/walForSqlite3", False)
@@ -742,13 +761,7 @@ def testDisablewalForSqlite3(self):
         vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
 
         # Test that we are using default delete mode and not WAL
-        ds = ogr.Open(tmpfile)
-        lyr = ds.ExecuteSQL('PRAGMA journal_mode')
-        f = lyr.GetNextFeature()
-        res = f.GetField(0)
-        ds.ReleaseResultSet(lyr)
-        ds = None
-        self.assertEqual(res, 'delete')
+        self.assertEqual(TestPyQgsOGRProviderGpkg._getJournalMode(tmpfile), 'delete')
 
         self.assertTrue(vl.startEditing())
         feature = next(vl.getFeatures())
@@ -763,6 +776,32 @@ def testDisablewalForSqlite3(self):
 
         QgsSettings().setValue("/qgis/walForSqlite3", None)
 
+    @unittest.skipIf(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(3, 4, 2), "GDAL 3.4.2 required")
+    def testNolock(self):
+        """ Test that with GDAL >= 3.4.2 opening a GPKG file doesn't turn on WAL journal_mode """
+
+        srcpath = os.path.join(TEST_DATA_DIR, 'provider')
+        srcfile = os.path.join(srcpath, 'geopackage.gpkg')
+
+        last_modified = QFileInfo(srcfile).lastModified()
+        vl = QgsVectorLayer(u'{}'.format(srcfile) + "|layername=geopackage", u'test', u'ogr')
+        self.assertEqual(TestPyQgsOGRProviderGpkg._getJournalMode(srcfile), 'delete')
+        del vl
+        self.assertEqual(last_modified, QFileInfo(srcfile).lastModified())
+
+        shutil.copy(os.path.join(srcpath, 'geopackage.gpkg'), self.basetestpath)
+        tmpfile = os.path.join(self.basetestpath, 'geopackage.gpkg')
+
+        vl = QgsVectorLayer(u'{}'.format(tmpfile) + "|layername=geopackage", u'test', u'ogr')
+
+        self.assertEqual(TestPyQgsOGRProviderGpkg._getJournalMode(tmpfile), 'delete')
+
+        vl.startEditing()
+        self.assertEqual(TestPyQgsOGRProviderGpkg._getJournalMode(tmpfile), 'wal')
+        vl.commitChanges()
+
+        self.assertEqual(TestPyQgsOGRProviderGpkg._getJournalMode(tmpfile), 'delete')
+
     def testSimulatedDBManagerImport(self):
         uri = 'point?field=f1:int'
         uri += '&field=f2:double(6,4)'
@@ -858,6 +897,7 @@ def testExportLayerToExistingDatabase(self):
                                           QgsCoordinateReferenceSystem('EPSG:3111'), False, options)
         self.assertFalse(exporter.errorCode(),
                          'unexpected export error {}: {}'.format(exporter.errorCode(), exporter.errorMessage()))
+        del exporter
         options['layerName'] = 'table2'
         exporter = QgsVectorLayerExporter(tmpfile, "ogr", fields, QgsWkbTypes.Point, QgsCoordinateReferenceSystem('EPSG:3113'),
                                           False, options)
@@ -2189,7 +2229,9 @@ def testTransactionGroupCrash(self):
     def _testVectorLayerExporterDeferredSpatialIndex(self, layerOptions, expectSpatialIndex):
         """ Internal method """
 
-        tmpfile = '/vsimem/_testVectorLayerExporterDeferredSpatialIndex.gpkg'
+        tmpfile = os.path.join(
+            self.basetestpath, 'testVectorLayerExporterDeferredSpatialIndex.gpkg')
+        gdal.Unlink(tmpfile)
         options = {}
         options['driverName'] = 'GPKG'
         options['layerName'] = 'table1'