Skip to content

Commit b6b8759

Browse files
rouaultrouault
committedSep 23, 2016
Fix database locking when editing GeoPackage
Concurrent read and write can lock a GeoPackage database given the default journaling mode of SQLite (delete). Use WAL when possible to avoid that. Fixes #15351
1 parent 0542aac commit b6b8759

File tree

6 files changed

+259
-4
lines changed

6 files changed

+259
-4
lines changed
 

‎src/providers/ogr/qgsogrconnpool.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#define QGSOGRCONNPOOL_H
1818

1919
#include "qgsconnectionpool.h"
20+
#include "qgsogrprovider.h"
2021
#include <ogr_api.h>
2122

2223

@@ -43,7 +44,7 @@ inline void qgsConnectionPool_ConnectionCreate( QString connInfo, QgsOgrConn*& c
4344

4445
inline void qgsConnectionPool_ConnectionDestroy( QgsOgrConn* c )
4546
{
46-
OGR_DS_Destroy( c->ds );
47+
QgsOgrProviderUtils::OGRDestroyWrapper( c->ds );
4748
delete c;
4849
}
4950

‎src/providers/ogr/qgsogrfeatureiterator.cpp

+7
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,12 @@ bool QgsOgrFeatureIterator::close()
254254

255255
iteratorClosed();
256256

257+
// Will for example release SQLite3 statements
258+
if ( ogrLayer )
259+
{
260+
OGR_L_ResetReading( ogrLayer );
261+
}
262+
257263
if ( mSubsetStringSet )
258264
{
259265
OGR_DS_ReleaseResultSet( mConn->ds, ogrLayer );
@@ -263,6 +269,7 @@ bool QgsOgrFeatureIterator::close()
263269
QgsOgrConnPool::instance()->releaseConnection( mConn );
264270

265271
mConn = nullptr;
272+
ogrLayer = nullptr;
266273

267274
mClosed = true;
268275
return true;

‎src/providers/ogr/qgsogrprovider.cpp

+146-2
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,13 @@ email : sherman at mrcc.com
4848
#include "qgsvectorlayerimport.h"
4949
#include "qgslocalec.h"
5050

51+
#ifdef Q_OS_WIN
52+
#include <windows.h>
53+
#endif
54+
#ifdef Q_OS_LINUX
55+
#include <sys/vfs.h>
56+
#endif
57+
5158
static const QString TEXT_PROVIDER_KEY = "ogr";
5259
static const QString TEXT_PROVIDER_DESCRIPTION =
5360
QString( "OGR data provider" )
@@ -382,11 +389,14 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )
382389

383390
QgsOgrProvider::~QgsOgrProvider()
384391
{
385-
close();
386392
QgsOgrConnPool::instance()->unref( dataSourceUri() );
387393
// We must also make sure to flush unusef cached connections so that
388394
// the file can be removed (#15137)
389395
QgsOgrConnPool::instance()->invalidateConnections( dataSourceUri() );
396+
397+
// Do that as last step for final cleanup that might be prevented by
398+
// still opened datasets.
399+
close();
390400
}
391401

392402
QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const
@@ -2903,6 +2913,125 @@ OGRDataSourceH QgsOgrProviderUtils::OGROpenWrapper( const char* pszPath, bool bU
29032913
return hDS;
29042914
}
29052915

2916+
static bool IsLocalFile( const QString& path )
2917+
{
2918+
QString dirName( QFileInfo( path ).absolutePath() );
2919+
// Start with the OS specific methods since the QT >= 5.4 method just
2920+
// return a string and not an enumerated type.
2921+
#if defined(Q_OS_WIN)
2922+
if ( dirName.startsWith( "\\\\" ) )
2923+
return false;
2924+
if ( dirName.length() >= 3 && dirName[1] == ':' &&
2925+
( dirName[2] == '\\' || dirName[2] == '/' ) )
2926+
{
2927+
dirName.resize( 3 );
2928+
return GetDriveType( dirName.toAscii().constData() ) != DRIVE_REMOTE;
2929+
}
2930+
return true;
2931+
#elif defined(Q_OS_LINUX)
2932+
struct statfs sStatFS;
2933+
if ( statfs( dirName.toAscii().constData(), &sStatFS ) == 0 )
2934+
{
2935+
// Codes from http://man7.org/linux/man-pages/man2/statfs.2.html
2936+
if ( sStatFS.f_type == 0x6969 /* NFS */ ||
2937+
sStatFS.f_type == 0x517b /* SMB */ ||
2938+
sStatFS.f_type == 0xff534d42 /* CIFS */ )
2939+
{
2940+
return false;
2941+
}
2942+
}
2943+
return true;
2944+
#elif QT_VERSION >= QT_VERSION_CHECK(5, 4, 0)
2945+
QStorageInfo info( dirName );
2946+
QString fileSystem( info.fileSystemType() );
2947+
QgsDebugMsg( QString( "Filesystem for %1 is %2" ).arg( path ).arg( fileSystem ) );
2948+
return path != "nfs" && path != "smbfs";
2949+
#else
2950+
return true;
2951+
#endif
2952+
}
2953+
2954+
void QgsOgrProviderUtils::OGRDestroyWrapper( OGRDataSourceH ogrDataSource )
2955+
{
2956+
if ( !ogrDataSource )
2957+
return;
2958+
OGRSFDriverH ogrDriver = OGR_DS_GetDriver( ogrDataSource );
2959+
QString ogrDriverName = OGR_Dr_GetName( ogrDriver );
2960+
QString datasetName( FROM8( OGR_DS_GetName( ogrDataSource ) ) );
2961+
if ( ogrDriverName == "GPKG" &&
2962+
IsLocalFile( datasetName ) &&
2963+
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) )
2964+
{
2965+
// We need to reset all iterators on layers, otherwise we will not
2966+
// be able to change journal_mode.
2967+
int layerCount = OGR_DS_GetLayerCount( ogrDataSource );
2968+
for ( int i = 0; i < layerCount; i ++ )
2969+
{
2970+
OGR_L_ResetReading( OGR_DS_GetLayer( ogrDataSource, i ) );
2971+
}
2972+
2973+
CPLPushErrorHandler( CPLQuietErrorHandler );
2974+
QgsDebugMsg( "GPKG: Trying to return to delete mode" );
2975+
bool bSuccess = false;
2976+
OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
2977+
"PRAGMA journal_mode = delete",
2978+
NULL, NULL );
2979+
if ( hSqlLyr != NULL )
2980+
{
2981+
OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
2982+
if ( hFeat != NULL )
2983+
{
2984+
const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
2985+
bSuccess = EQUAL( pszRet, "delete" );
2986+
QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
2987+
OGR_F_Destroy( hFeat );
2988+
}
2989+
}
2990+
else if ( CPLGetLastErrorType() != CE_None )
2991+
{
2992+
QgsDebugMsg( QString( "Return: %1" ).arg( CPLGetLastErrorMsg() ) );
2993+
}
2994+
OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
2995+
CPLPopErrorHandler();
2996+
OGR_DS_Destroy( ogrDataSource );
2997+
2998+
// This may have not worked if the file was opened in read-only mode,
2999+
// so retry in update mode
3000+
if ( !bSuccess )
3001+
{
3002+
QgsDebugMsg( "GPKG: Trying again" );
3003+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "DELETE" );
3004+
ogrDataSource = OGROpen( TO8F( datasetName ), TRUE, NULL );
3005+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
3006+
if ( ogrDataSource )
3007+
{
3008+
#ifdef QGISDEBUG
3009+
CPLPushErrorHandler( CPLQuietErrorHandler );
3010+
OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
3011+
"PRAGMA journal_mode",
3012+
NULL, NULL );
3013+
CPLPopErrorHandler();
3014+
if ( hSqlLyr != NULL )
3015+
{
3016+
OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
3017+
if ( hFeat != NULL )
3018+
{
3019+
const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
3020+
QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
3021+
OGR_F_Destroy( hFeat );
3022+
}
3023+
OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
3024+
}
3025+
#endif
3026+
OGR_DS_Destroy( ogrDataSource );
3027+
}
3028+
}
3029+
}
3030+
else
3031+
{
3032+
OGR_DS_Destroy( ogrDataSource );
3033+
}
3034+
}
29063035

29073036
QByteArray QgsOgrProviderUtils::quotedIdentifier( QByteArray field, const QString& ogrDriverName )
29083037
{
@@ -3138,7 +3267,22 @@ void QgsOgrProvider::open( OpenMode mode )
31383267

31393268
// first try to open in update mode (unless specified otherwise)
31403269
if ( !openReadOnly )
3270+
{
3271+
if ( QFileInfo( mFilePath ).suffix().compare( "gpkg", Qt::CaseInsensitive ) == 0 &&
3272+
IsLocalFile( mFilePath ) &&
3273+
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) &&
3274+
QSettings().value( "/qgis/walForSqlite3", true ).toBool() )
3275+
{
3276+
// For GeoPackage, we force opening of the file in WAL (Write Ahead Log)
3277+
// mode so as to avoid readers blocking writer(s), and vice-versa.
3278+
// https://www.sqlite.org/wal.html
3279+
// But only do that on a local file since WAL is advertized not to work
3280+
// on network shares
3281+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
3282+
}
31413283
ogrDataSource = QgsOgrProviderUtils::OGROpenWrapper( TO8F( mFilePath ), true, &ogrDriver );
3284+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
3285+
}
31423286

31433287
mValid = false;
31443288
if ( ogrDataSource )
@@ -3268,7 +3412,7 @@ void QgsOgrProvider::close()
32683412

32693413
if ( ogrDataSource )
32703414
{
3271-
OGR_DS_Destroy( ogrDataSource );
3415+
QgsOgrProviderUtils::OGRDestroyWrapper( ogrDataSource );
32723416
}
32733417
ogrDataSource = nullptr;
32743418
ogrLayer = nullptr;

‎src/providers/ogr/qgsogrprovider.h

+6
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ email : sherman at mrcc.com
1515
* *
1616
***************************************************************************/
1717

18+
#ifndef QGSOGRPROVIDER_H
19+
#define QGSOGRPROVIDER_H
20+
1821
#include "QTextCodec"
1922

2023
#include "qgsrectangle.h"
@@ -387,4 +390,7 @@ class QgsOgrProviderUtils
387390
static QString quotedValue( const QVariant& value );
388391

389392
static OGRDataSourceH OGROpenWrapper( const char* pszPath, bool bUpdate, OGRSFDriverH *phDriver );
393+
static void OGRDestroyWrapper( OGRDataSourceH ogrDataSource );
390394
};
395+
396+
#endif // QGSOGRPROVIDER_H

‎tests/src/python/test_provider_ogr_gpkg.py

+76
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,15 @@ def GDAL_COMPUTE_VERSION(maj, min, rev):
3131
return ((maj) * 1000000 + (min) * 10000 + (rev) * 100)
3232

3333

34+
class ErrorReceiver():
35+
36+
def __init__(self):
37+
self.msg = None
38+
39+
def receiveError(self, msg):
40+
self.msg = msg
41+
42+
3443
class TestPyQgsOGRProviderGpkg(unittest.TestCase):
3544

3645
@classmethod
@@ -80,5 +89,72 @@ def testCurveGeometryType(self):
8089
# The geometries must be binarily identical
8190
self.assertEqual(got_geom.asWkb(), reference.asWkb(), 'Expected {}, got {}'.format(reference.exportToWkt(), got_geom.exportToWkt()))
8291

92+
def internalTestBug15351(self, orderClosing):
93+
94+
tmpfile = os.path.join(self.basetestpath, 'testBug15351.gpkg')
95+
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
96+
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
97+
f = ogr.Feature(lyr.GetLayerDefn())
98+
f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(0 0)'))
99+
lyr.CreateFeature(f)
100+
f = None
101+
ds = None
102+
103+
vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
104+
self.assertTrue(vl.startEditing())
105+
self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('Point (3 50)')))
106+
107+
# Iterate over features (will open a new OGR connection), but do not
108+
# close the iterator for now
109+
it = vl.getFeatures()
110+
f = QgsFeature()
111+
it.nextFeature(f)
112+
113+
if orderClosing == 'closeIter_commit_closeProvider':
114+
it = None
115+
116+
# Commit changes
117+
cbk = ErrorReceiver()
118+
vl.dataProvider().raiseError.connect(cbk.receiveError)
119+
self.assertTrue(vl.commitChanges())
120+
self.assertIsNone(cbk.msg)
121+
122+
# Close layer and iterator in different orders
123+
if orderClosing == 'closeIter_commit_closeProvider':
124+
vl = None
125+
elif orderClosing == 'commit_closeProvider_closeIter':
126+
vl = None
127+
it = None
128+
else:
129+
assert orderClosing == 'commit_closeIter_closeProvider'
130+
it = None
131+
vl = None
132+
133+
# Test that we succeeded restoring default journal mode, and we
134+
# are not let in WAL mode.
135+
ds = ogr.Open(tmpfile)
136+
lyr = ds.ExecuteSQL('PRAGMA journal_mode')
137+
f = lyr.GetNextFeature()
138+
res = f.GetField(0)
139+
ds.ReleaseResultSet(lyr)
140+
ds = None
141+
self.assertEqual(res, 'delete')
142+
143+
@unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
144+
# We need GDAL 2.0 to issue PRAGMA journal_mode
145+
# Note: for that case, we don't strictly need turning on WAL
146+
def testBug15351_closeIter_commit_closeProvider(self):
147+
self.internalTestBug15351('closeIter_commit_closeProvider')
148+
149+
@unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
150+
# We need GDAL 2.0 to issue PRAGMA journal_mode
151+
def testBug15351_closeIter_commit_closeProvider(self):
152+
self.internalTestBug15351('closeIter_commit_closeProvider')
153+
154+
@unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
155+
# We need GDAL 2.0 to issue PRAGMA journal_mode
156+
def testBug15351_commit_closeIter_closeProvider(self):
157+
self.internalTestBug15351('commit_closeIter_closeProvider')
158+
83159
if __name__ == '__main__':
84160
unittest.main()

‎tests/src/python/test_provider_spatialite.py

+22-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
import shutil
2020
import tempfile
2121

22-
from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature
22+
from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature, QgsGeometry
2323

2424
from qgis.testing import start_app, unittest
2525
from utilities import unitTestDataPath
@@ -347,6 +347,27 @@ def test_arrays(self):
347347
self.assertEqual(read_back['ints'], new_f['ints'])
348348
self.assertEqual(read_back['reals'], new_f['reals'])
349349

350+
# This test would fail. It would require turning on WAL
351+
def XXXXXtestLocking(self):
352+
353+
temp_dbname = self.dbname + '.locking'
354+
shutil.copy(self.dbname, temp_dbname)
355+
356+
vl = QgsVectorLayer("dbname=%s table=test_n (geometry)" % temp_dbname, "test_n", "spatialite")
357+
self.assertTrue(vl.isValid())
358+
359+
self.assertTrue(vl.startEditing())
360+
self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('POLYGON((0 0,1 0,1 1,0 1,0 0))')))
361+
362+
# The iterator will take one extra connection
363+
myiter = vl.getFeatures()
364+
365+
# Consume one feature but the iterator is still opened
366+
f = next(myiter)
367+
self.assertTrue(f.isValid())
368+
369+
self.assertTrue(vl.commitChanges())
370+
350371

351372
if __name__ == '__main__':
352373
unittest.main()

0 commit comments

Comments
 (0)
Please sign in to comment.