Skip to content

Commit

Permalink
OGR provider: only enable journal_mode = wal when editing. requires G…
Browse files Browse the repository at this point in the history 
…DAL >= 3.4.2 (fixes #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
  • Loading branch information
@rouault @nyalldawson
rouault authored and nyalldawson committed Feb 1, 2022
1 parent 62eb4cd commit fe410ec
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 40 deletions.
28 changes: 25 additions & 3 deletions src/core/providers/ogr/qgsogrprovider.cpp
View file
Expand Up @@ -3471,15 +3471,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() )
Expand Down Expand Up @@ -3593,10 +3606,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();
Expand All @@ -3614,7 +3633,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
{
Expand Down Expand Up @@ -3653,6 +3672,9 @@ void QgsOgrProvider::open( OpenMode mode )

void QgsOgrProvider::close()
{
if ( mWriteAccess && mForceRecomputeExtent )
extent();

mOgrSqlLayer.reset();
mOgrOrigLayer.reset();
mOgrLayer = nullptr;
Expand Down
97 changes: 72 additions & 25 deletions src/core/providers/ogr/qgsogrproviderutils.cpp
View file
Expand Up @@ -999,17 +999,37 @@ 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
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3,4,2)
QgsDebugMsgLevel( QStringLiteral( "Enabling WAL" ), 3 );
#endif
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
}
bIsLocalGpkg = true;
}
else if ( bIsGpkg )
Expand All @@ -1019,6 +1039,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 )
{
Expand Down Expand Up @@ -1144,35 +1169,26 @@ void QgsOgrProviderUtils::GDALCloseWrapper( GDALDatasetH hDS )
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", nullptr ) )
{
bool openedAsUpdate = false;
bool tryReturnToWall = false;
bool tryReturnToDelete = false;
{
QMutexLocker locker( sGlobalMutex() );
( *sMapCountOpenedDS() )[ datasetName ] --;
if ( ( *sMapCountOpenedDS() )[ datasetName ] == 0 )
{
sMapCountOpenedDS()->remove( datasetName );
openedAsUpdate = ( *sMapDSHandleToUpdateMode() )[hDS];
tryReturnToWall = true;
tryReturnToDelete = true;
}
sMapDSHandleToUpdateMode()->remove( hDS );
}
if ( tryReturnToWall )
if ( tryReturnToDelete )
{
bool bSuccess = false;
if ( openedAsUpdate )
{
// We need to reset all iterators on layers, otherwise we will not
// be able to change journal_mode.
int layerCount = GDALDatasetGetLayerCount( hDS );
for ( int i = 0; i < layerCount; i ++ )
{
OGR_L_ResetReading( GDALDatasetGetLayer( hDS, i ) );
}

CPLPushErrorHandler( CPLQuietErrorHandler );
QgsDebugMsgLevel( QStringLiteral( "GPKG: Trying to return to delete mode" ), 2 );
// Check if the current journal_mode is not already delete
{
OGRLayerH hSqlLyr = GDALDatasetExecuteSQL( hDS,
"PRAGMA journal_mode = delete",
"PRAGMA journal_mode",
nullptr, nullptr );
if ( hSqlLyr )
{
Expand All @@ -1183,13 +1199,44 @@ void QgsOgrProviderUtils::GDALCloseWrapper( GDALDatasetH hDS )
bSuccess = EQUAL( pszRet, "delete" );
QgsDebugMsgLevel( QStringLiteral( "Return: %1" ).arg( pszRet ), 2 );
}
GDALDatasetReleaseResultSet( hDS, hSqlLyr );
}
else if ( CPLGetLastErrorType() != CE_None )
}

if ( openedAsUpdate )
{
// We need to reset all iterators on layers, otherwise we will not
// be able to change journal_mode.
int layerCount = GDALDatasetGetLayerCount( hDS );
for ( int i = 0; i < layerCount; i ++ )
{
OGR_L_ResetReading( GDALDatasetGetLayer( hDS, i ) );
}

if ( !bSuccess )
{
QgsDebugMsg( QStringLiteral( "Return: %1" ).arg( CPLGetLastErrorMsg() ) );
CPLPushErrorHandler( CPLQuietErrorHandler );
QgsDebugMsgLevel( QStringLiteral( "GPKG: Trying to return to delete mode" ), 2 );
OGRLayerH hSqlLyr = GDALDatasetExecuteSQL( hDS,
"PRAGMA journal_mode = delete",
nullptr, nullptr );
if ( hSqlLyr )
{
gdal::ogr_feature_unique_ptr hFeat( OGR_L_GetNextFeature( hSqlLyr ) );
if ( hFeat )
{
const char *pszRet = OGR_F_GetFieldAsString( hFeat.get(), 0 );
bSuccess = EQUAL( pszRet, "delete" );
QgsDebugMsgLevel( QStringLiteral( "Return: %1" ).arg( pszRet ), 2 );
}
}
else if ( CPLGetLastErrorType() != CE_None )
{
QgsDebugMsg( QStringLiteral( "Return: %1" ).arg( CPLGetLastErrorMsg() ) );
}
GDALDatasetReleaseResultSet( hDS, hSqlLyr );
CPLPopErrorHandler();
}
GDALDatasetReleaseResultSet( hDS, hSqlLyr );
CPLPopErrorHandler();
}
GDALClose( hDS );

Expand Down
4 changes: 2 additions & 2 deletions src/core/vector/qgsvectorlayer.cpp
View file
Expand Up @@ -1477,10 +1477,10 @@ bool QgsVectorLayer::startEditing()
return false;
}

emit beforeEditingStarted();

mDataProvider->enterUpdateMode();

emit beforeEditingStarted();

if ( mDataProvider->transaction() )
{
mEditBuffer = new QgsVectorLayerEditPassthrough( this );
Expand Down
3 changes: 3 additions & 0 deletions tests/src/python/providertestbase.py
View file
Expand Up @@ -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
Expand Down
62 changes: 52 additions & 10 deletions tests/src/python/test_provider_ogr_gpkg.py
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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"""

Expand Down Expand Up @@ -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)
Expand All @@ -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())
Expand All @@ -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)'
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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'
Expand Down

0 comments on commit fe410ec

Please sign in to comment.