Skip to content

Commit f939e9c

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
·
final-2_14_22final-2_14_7
1 parent bcec7ab commit f939e9c

File tree

5 files changed

+355
-4
lines changed

5 files changed

+355
-4
lines changed
 

‎src/providers/ogr/qgsogrconnpool.h

Lines changed: 2 additions & 1 deletion
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+
QgsOgrUtils::OGRDestroyWrapper( c->ds );
4748
delete c;
4849
}
4950

‎src/providers/ogr/qgsogrfeatureiterator.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,12 @@ bool QgsOgrFeatureIterator::close()
301301

302302
iteratorClosed();
303303

304+
// Will for example release SQLite3 statements
305+
if ( ogrLayer )
306+
{
307+
OGR_L_ResetReading( ogrLayer );
308+
}
309+
304310
if ( mSubsetStringSet )
305311
{
306312
OGR_DS_ReleaseResultSet( mConn->ds, ogrLayer );
@@ -310,6 +316,7 @@ bool QgsOgrFeatureIterator::close()
310316
QgsOgrConnPool::instance()->releaseConnection( mConn );
311317

312318
mConn = nullptr;
319+
ogrLayer = nullptr;
313320

314321
mClosed = true;
315322
return true;

‎src/providers/ogr/qgsogrprovider.cpp

Lines changed: 174 additions & 3 deletions
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" )
@@ -383,11 +390,14 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )
383390

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

393403
QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const
@@ -2627,6 +2637,152 @@ void QgsOgrProvider::forceReload()
26272637
QgsOgrConnPool::instance()->invalidateConnections( dataSourceUri() );
26282638
}
26292639

2640+
OGRDataSourceH QgsOgrUtils::OGROpenWrapper( const char* pszPath, bool bUpdate, OGRSFDriverH *phDriver )
2641+
{
2642+
CPLErrorReset();
2643+
OGRSFDriverH hDriver = nullptr;
2644+
OGRDataSourceH hDS = OGROpen( pszPath, bUpdate, &hDriver );
2645+
if ( phDriver )
2646+
*phDriver = hDriver;
2647+
if ( !hDS )
2648+
return nullptr;
2649+
// GDAL < 1.11.5 has a crashing bug with GeoPackage databases with curve geometry
2650+
// types (https://trac.osgeo.org/gdal/ticket/6558)
2651+
#if GDAL_VERSION_MAJOR == 1 && GDAL_VERSION_MINOR == 11 && GDAL_VERSION_MACRO < 5
2652+
const char* pszLastErrorMsg = CPLGetLastErrorMsg();
2653+
if ( hDriver == OGRGetDriverByName( "GPKG" ) &&
2654+
strstr( pszLastErrorMsg, "geometry column" ) &&
2655+
strstr( pszLastErrorMsg, "of type" ) &&
2656+
strstr( pszLastErrorMsg, "ignored" ) )
2657+
{
2658+
QgsDebugMsg( QString( "Ignoring %1 that is a GeoPackage DB with curve geometries" ).arg( pszPath ) );
2659+
OGR_DS_Destroy( hDS );
2660+
hDS = nullptr;
2661+
}
2662+
#endif
2663+
return hDS;
2664+
}
2665+
2666+
static bool IsLocalFile( const QString& path )
2667+
{
2668+
QString dirName( QFileInfo( path ).absolutePath() );
2669+
// Start with the OS specific methods since the QT >= 5.4 method just
2670+
// return a string and not an enumerated type.
2671+
#if defined(Q_OS_WIN)
2672+
if ( dirName.startsWith( "\\\\" ) )
2673+
return false;
2674+
if ( dirName.length() >= 3 && dirName[1] == ':' &&
2675+
( dirName[2] == '\\' || dirName[2] == '/' ) )
2676+
{
2677+
dirName.resize( 3 );
2678+
return GetDriveType( dirName.toAscii().constData() ) != DRIVE_REMOTE;
2679+
}
2680+
return true;
2681+
#elif defined(Q_OS_LINUX)
2682+
struct statfs sStatFS;
2683+
if ( statfs( dirName.toAscii().constData(), &sStatFS ) == 0 )
2684+
{
2685+
// Codes from http://man7.org/linux/man-pages/man2/statfs.2.html
2686+
if ( sStatFS.f_type == 0x6969 /* NFS */ ||
2687+
sStatFS.f_type == 0x517b /* SMB */ ||
2688+
sStatFS.f_type == 0xff534d42 /* CIFS */ )
2689+
{
2690+
return false;
2691+
}
2692+
}
2693+
return true;
2694+
#elif QT_VERSION >= QT_VERSION_CHECK(5, 4, 0)
2695+
QStorageInfo info( dirName );
2696+
QString fileSystem( info.fileSystemType() );
2697+
QgsDebugMsg( QString( "Filesystem for %1 is %2" ).arg( path ).arg( fileSystem ) );
2698+
return path != "nfs" && path != "smbfs";
2699+
#else
2700+
return true;
2701+
#endif
2702+
}
2703+
2704+
void QgsOgrUtils::OGRDestroyWrapper( OGRDataSourceH ogrDataSource )
2705+
{
2706+
if ( !ogrDataSource )
2707+
return;
2708+
OGRSFDriverH ogrDriver = OGR_DS_GetDriver( ogrDataSource );
2709+
QString ogrDriverName = OGR_Dr_GetName( ogrDriver );
2710+
QString datasetName( FROM8( OGR_DS_GetName( ogrDataSource ) ) );
2711+
if ( ogrDriverName == "GPKG" &&
2712+
IsLocalFile( datasetName ) &&
2713+
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) )
2714+
{
2715+
// We need to reset all iterators on layers, otherwise we will not
2716+
// be able to change journal_mode.
2717+
int layerCount = OGR_DS_GetLayerCount( ogrDataSource );
2718+
for ( int i = 0; i < layerCount; i ++ )
2719+
{
2720+
OGR_L_ResetReading( OGR_DS_GetLayer( ogrDataSource, i ) );
2721+
}
2722+
2723+
CPLPushErrorHandler( CPLQuietErrorHandler );
2724+
QgsDebugMsg( "GPKG: Trying to return to delete mode" );
2725+
bool bSuccess = false;
2726+
OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
2727+
"PRAGMA journal_mode = delete",
2728+
NULL, NULL );
2729+
if ( hSqlLyr != NULL )
2730+
{
2731+
OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
2732+
if ( hFeat != NULL )
2733+
{
2734+
const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
2735+
bSuccess = EQUAL( pszRet, "delete" );
2736+
QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
2737+
OGR_F_Destroy( hFeat );
2738+
}
2739+
}
2740+
else if ( CPLGetLastErrorType() != CE_None )
2741+
{
2742+
QgsDebugMsg( QString( "Return: %1" ).arg( CPLGetLastErrorMsg() ) );
2743+
}
2744+
OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
2745+
CPLPopErrorHandler();
2746+
OGR_DS_Destroy( ogrDataSource );
2747+
2748+
// This may have not worked if the file was opened in read-only mode,
2749+
// so retry in update mode
2750+
if ( !bSuccess )
2751+
{
2752+
QgsDebugMsg( "GPKG: Trying again" );
2753+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "DELETE" );
2754+
ogrDataSource = OGROpen( TO8F( datasetName ), TRUE, NULL );
2755+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
2756+
if ( ogrDataSource )
2757+
{
2758+
#ifdef QGISDEBUG
2759+
CPLPushErrorHandler( CPLQuietErrorHandler );
2760+
OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
2761+
"PRAGMA journal_mode",
2762+
NULL, NULL );
2763+
CPLPopErrorHandler();
2764+
if ( hSqlLyr != NULL )
2765+
{
2766+
OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
2767+
if ( hFeat != NULL )
2768+
{
2769+
const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
2770+
QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
2771+
OGR_F_Destroy( hFeat );
2772+
}
2773+
OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
2774+
}
2775+
#endif
2776+
OGR_DS_Destroy( ogrDataSource );
2777+
}
2778+
}
2779+
}
2780+
else
2781+
{
2782+
OGR_DS_Destroy( ogrDataSource );
2783+
}
2784+
}
2785+
26302786
QByteArray QgsOgrUtils::quotedIdentifier( QByteArray field, const QString& ogrDriverName )
26312787
{
26322788
if ( ogrDriverName == "MySQL" )
@@ -2855,7 +3011,22 @@ void QgsOgrProvider::open( OpenMode mode )
28553011

28563012
// first try to open in update mode (unless specified otherwise)
28573013
if ( !openReadOnly )
2858-
ogrDataSource = OGROpen( TO8F( mFilePath ), true, &ogrDriver );
3014+
{
3015+
if ( QFileInfo( mFilePath ).suffix().compare( "gpkg", Qt::CaseInsensitive ) == 0 &&
3016+
IsLocalFile( mFilePath ) &&
3017+
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) &&
3018+
QSettings().value( "/qgis/walForSqlite3", true ).toBool() )
3019+
{
3020+
// For GeoPackage, we force opening of the file in WAL (Write Ahead Log)
3021+
// mode so as to avoid readers blocking writer(s), and vice-versa.
3022+
// https://www.sqlite.org/wal.html
3023+
// But only do that on a local file since WAL is advertized not to work
3024+
// on network shares
3025+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
3026+
}
3027+
ogrDataSource = QgsOgrUtils::OGROpenWrapper( TO8F( mFilePath ), true, &ogrDriver );
3028+
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
3029+
}
28593030

28603031
mValid = false;
28613032
if ( ogrDataSource )
@@ -2985,7 +3156,7 @@ void QgsOgrProvider::close()
29853156

29863157
if ( ogrDataSource )
29873158
{
2988-
OGR_DS_Destroy( ogrDataSource );
3159+
QgsOgrUtils::OGRDestroyWrapper( ogrDataSource );
29893160
}
29903161
ogrDataSource = nullptr;
29913162
ogrLayer = nullptr;

‎src/providers/ogr/qgsogrprovider.h

Lines changed: 8 additions & 0 deletions
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"
@@ -400,4 +403,9 @@ class QgsOgrUtils
400403
/** Quote a value for placement in a SQL string.
401404
*/
402405
static QString quotedValue( const QVariant& value );
406+
407+
static OGRDataSourceH OGROpenWrapper( const char* pszPath, bool bUpdate, OGRSFDriverH *phDriver );
408+
static void OGRDestroyWrapper( OGRDataSourceH ogrDataSource );
403409
};
410+
411+
#endif // QGSOGRPROVIDER_H

‎tests/src/python/test_provider_ogr_gpkg.py

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
# -*- coding: utf-8 -*-
2+
"""QGIS Unit tests for the OGR/GPKG provider.
3+
4+
.. note:: This program is free software; you can redistribute it and/or modify
5+
it under the terms of the GNU General Public License as published by
6+
the Free Software Foundation; either version 2 of the License, or
7+
(at your option) any later version.
8+
"""
9+
__author__ = 'Even Rouault'
10+
__date__ = '2016-04-21'
11+
__copyright__ = 'Copyright 2016, Even Rouault'
12+
# This will get replaced with a git SHA1 when you do a git archive
13+
__revision__ = '$Format:%H$'
14+
15+
import qgis # NOQA
16+
17+
import os
18+
import tempfile
19+
import shutil
20+
import glob
21+
from osgeo import gdal, ogr
22+
23+
from qgis.core import QgsVectorLayer, QgsFeature, QgsGeometry, QgsFeatureRequest
24+
from qgis.testing import start_app, unittest
25+
from utilities import unitTestDataPath
26+
27+
start_app()
28+
29+
30+
def GDAL_COMPUTE_VERSION(maj, min, rev):
31+
return ((maj) * 1000000 + (min) * 10000 + (rev) * 100)
32+
33+
34+
class ErrorReceiver():
35+
36+
def __init__(self):
37+
self.msg = None
38+
39+
def receiveError(self, msg):
40+
self.msg = msg
41+
42+
43+
class TestPyQgsOGRProviderGpkg(unittest.TestCase):
44+
45+
@classmethod
46+
def setUpClass(cls):
47+
"""Run before all tests"""
48+
# Create test layer
49+
cls.basetestpath = tempfile.mkdtemp()
50+
51+
@classmethod
52+
def tearDownClass(cls):
53+
"""Run after all tests"""
54+
shutil.rmtree(cls.basetestpath, True)
55+
56+
def XXXXtestSingleToMultiPolygonPromotion(self):
57+
58+
tmpfile = os.path.join(self.basetestpath, 'testSingleToMultiPolygonPromotion.gpkg')
59+
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
60+
lyr = ds.CreateLayer('test', geom_type=ogr.wkbMultiPolygon)
61+
ds = None
62+
63+
vl = QgsVectorLayer(u'{}|layerid=0'.format(tmpfile), u'test', u'ogr')
64+
f = QgsFeature()
65+
f.setGeometry(QgsGeometry.fromWkt('POLYGON ((0 0,0 1,1 1,0 0))'))
66+
vl.dataProvider().addFeatures([f])
67+
got = [f for f in vl.getFeatures()][0]
68+
got_geom = got.geometry()
69+
reference = QgsGeometry.fromWkt('MultiPolygon (((0 0, 0 1, 1 1, 0 0)))')
70+
# The geometries must be binarily identical
71+
self.assertEqual(got_geom.asWkb(), reference.asWkb(), 'Expected {}, got {}'.format(reference.exportToWkt(), got_geom.exportToWkt()))
72+
73+
def XXXXtestCurveGeometryType(self):
74+
if int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0):
75+
return
76+
77+
tmpfile = os.path.join(self.basetestpath, 'testCurveGeometryType.gpkg')
78+
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
79+
lyr = ds.CreateLayer('test', geom_type=ogr.wkbCurvePolygon)
80+
ds = None
81+
82+
vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
83+
self.assertEqual(vl.dataProvider().subLayers(), [u'0:test:0:CurvePolygon'])
84+
f = QgsFeature()
85+
f.setGeometry(QgsGeometry.fromWkt('POLYGON ((0 0,0 1,1 1,0 0))'))
86+
vl.dataProvider().addFeatures([f])
87+
got = [f for f in vl.getFeatures()][0]
88+
got_geom = got.geometry()
89+
reference = QgsGeometry.fromWkt('CurvePolygon (((0 0, 0 1, 1 1, 0 0)))')
90+
# The geometries must be binarily identical
91+
self.assertEqual(got_geom.asWkb(), reference.asWkb(), 'Expected {}, got {}'.format(reference.exportToWkt(), got_geom.exportToWkt()))
92+
93+
def internalTestBug15351(self, orderClosing):
94+
95+
tmpfile = os.path.join(self.basetestpath, 'testBug15351.gpkg')
96+
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
97+
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
98+
f = ogr.Feature(lyr.GetLayerDefn())
99+
f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(0 0)'))
100+
lyr.CreateFeature(f)
101+
f = None
102+
ds = None
103+
104+
vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
105+
self.assertTrue(vl.startEditing())
106+
self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('Point (3 50)')))
107+
108+
# Iterate over features (will open a new OGR connection), but do not
109+
# close the iterator for now
110+
it = vl.getFeatures()
111+
f = QgsFeature()
112+
it.nextFeature(f)
113+
114+
if orderClosing == 'closeIter_commit_closeProvider':
115+
it = None
116+
117+
# Commit changes
118+
cbk = ErrorReceiver()
119+
vl.dataProvider().raiseError.connect(cbk.receiveError)
120+
self.assertTrue(vl.commitChanges())
121+
self.assertIsNone(cbk.msg)
122+
123+
# Close layer and iterator in different orders
124+
if orderClosing == 'closeIter_commit_closeProvider':
125+
vl = None
126+
elif orderClosing == 'commit_closeProvider_closeIter':
127+
vl = None
128+
it = None
129+
else:
130+
assert orderClosing == 'commit_closeIter_closeProvider'
131+
it = None
132+
vl = None
133+
134+
# Test that we succeeded restoring default journal mode, and we
135+
# are not let in WAL mode.
136+
ds = ogr.Open(tmpfile)
137+
lyr = ds.ExecuteSQL('PRAGMA journal_mode')
138+
f = lyr.GetNextFeature()
139+
res = f.GetField(0)
140+
ds.ReleaseResultSet(lyr)
141+
ds = None
142+
self.assertEqual(res, 'delete')
143+
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+
if int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0):
148+
return
149+
self.internalTestBug15351('closeIter_commit_closeProvider')
150+
151+
# We need GDAL 2.0 to issue PRAGMA journal_mode
152+
def testBug15351_closeIter_commit_closeProvider(self):
153+
if int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0):
154+
return
155+
self.internalTestBug15351('closeIter_commit_closeProvider')
156+
157+
# We need GDAL 2.0 to issue PRAGMA journal_mode
158+
def testBug15351_commit_closeIter_closeProvider(self):
159+
if int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0):
160+
return
161+
self.internalTestBug15351('commit_closeIter_closeProvider')
162+
163+
if __name__ == '__main__':
164+
unittest.main()

1 commit comments

Comments
 (1)

nirvn commented on Jan 15, 2018

@nirvn
nirvn
on Jan 15, 2018
Contributor

@rouault , this has created a side issue: when adding a geopackage layer (or opening a pre-existing project) simply to display its content, the modified date for the geopackage file is modified. This is somewhat problematic for users who actually use this file system attribute to keep track of when a dataset was actually last modified.

Is there a way to fix that side issue?

Please sign in to comment.