Skip to content

Commit

Permalink
Merge pull request #3524 from rouault/fix_15351
Browse files Browse the repository at this point in the history
Fix database locking when editing GeoPackage
  • Loading branch information
rouault authored Sep 23, 2016
2 parents 911af05 + b6b8759 commit 7a685f1
Show file tree
Hide file tree
Showing 6 changed files with 259 additions and 4 deletions.
3 changes: 2 additions & 1 deletion src/providers/ogr/qgsogrconnpool.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#define QGSOGRCONNPOOL_H

#include "qgsconnectionpool.h"
#include "qgsogrprovider.h"
#include <ogr_api.h>


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

inline void qgsConnectionPool_ConnectionDestroy( QgsOgrConn* c )
{
OGR_DS_Destroy( c->ds );
QgsOgrProviderUtils::OGRDestroyWrapper( c->ds );
delete c;
}

Expand Down
7 changes: 7 additions & 0 deletions src/providers/ogr/qgsogrfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@ bool QgsOgrFeatureIterator::close()

iteratorClosed();

// Will for example release SQLite3 statements
if ( ogrLayer )
{
OGR_L_ResetReading( ogrLayer );
}

if ( mSubsetStringSet )
{
OGR_DS_ReleaseResultSet( mConn->ds, ogrLayer );
Expand All @@ -263,6 +269,7 @@ bool QgsOgrFeatureIterator::close()
QgsOgrConnPool::instance()->releaseConnection( mConn );

mConn = nullptr;
ogrLayer = nullptr;

mClosed = true;
return true;
Expand Down
148 changes: 146 additions & 2 deletions src/providers/ogr/qgsogrprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ email : sherman at mrcc.com
#include "qgsvectorlayerimport.h"
#include "qgslocalec.h"

#ifdef Q_OS_WIN
#include <windows.h>
#endif
#ifdef Q_OS_LINUX
#include <sys/vfs.h>
#endif

static const QString TEXT_PROVIDER_KEY = "ogr";
static const QString TEXT_PROVIDER_DESCRIPTION =
QString( "OGR data provider" )
Expand Down Expand Up @@ -382,11 +389,14 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )

QgsOgrProvider::~QgsOgrProvider()
{
close();
QgsOgrConnPool::instance()->unref( dataSourceUri() );
// We must also make sure to flush unusef cached connections so that
// the file can be removed (#15137)
QgsOgrConnPool::instance()->invalidateConnections( dataSourceUri() );

// Do that as last step for final cleanup that might be prevented by
// still opened datasets.
close();
}

QgsAbstractFeatureSource* QgsOgrProvider::featureSource() const
Expand Down Expand Up @@ -2903,6 +2913,125 @@ OGRDataSourceH QgsOgrProviderUtils::OGROpenWrapper( const char* pszPath, bool bU
return hDS;
}

static bool IsLocalFile( const QString& path )
{
QString dirName( QFileInfo( path ).absolutePath() );
// Start with the OS specific methods since the QT >= 5.4 method just
// return a string and not an enumerated type.
#if defined(Q_OS_WIN)
if ( dirName.startsWith( "\\\\" ) )
return false;
if ( dirName.length() >= 3 && dirName[1] == ':' &&
( dirName[2] == '\\' || dirName[2] == '/' ) )
{
dirName.resize( 3 );
return GetDriveType( dirName.toAscii().constData() ) != DRIVE_REMOTE;
}
return true;
#elif defined(Q_OS_LINUX)
struct statfs sStatFS;
if ( statfs( dirName.toAscii().constData(), &sStatFS ) == 0 )
{
// Codes from http://man7.org/linux/man-pages/man2/statfs.2.html
if ( sStatFS.f_type == 0x6969 /* NFS */ ||
sStatFS.f_type == 0x517b /* SMB */ ||
sStatFS.f_type == 0xff534d42 /* CIFS */ )
{
return false;
}
}
return true;
#elif QT_VERSION >= QT_VERSION_CHECK(5, 4, 0)
QStorageInfo info( dirName );
QString fileSystem( info.fileSystemType() );
QgsDebugMsg( QString( "Filesystem for %1 is %2" ).arg( path ).arg( fileSystem ) );
return path != "nfs" && path != "smbfs";
#else
return true;
#endif
}

void QgsOgrProviderUtils::OGRDestroyWrapper( OGRDataSourceH ogrDataSource )
{
if ( !ogrDataSource )
return;
OGRSFDriverH ogrDriver = OGR_DS_GetDriver( ogrDataSource );
QString ogrDriverName = OGR_Dr_GetName( ogrDriver );
QString datasetName( FROM8( OGR_DS_GetName( ogrDataSource ) ) );
if ( ogrDriverName == "GPKG" &&
IsLocalFile( datasetName ) &&
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) )
{
// We need to reset all iterators on layers, otherwise we will not
// be able to change journal_mode.
int layerCount = OGR_DS_GetLayerCount( ogrDataSource );
for ( int i = 0; i < layerCount; i ++ )
{
OGR_L_ResetReading( OGR_DS_GetLayer( ogrDataSource, i ) );
}

CPLPushErrorHandler( CPLQuietErrorHandler );
QgsDebugMsg( "GPKG: Trying to return to delete mode" );
bool bSuccess = false;
OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
"PRAGMA journal_mode = delete",
NULL, NULL );
if ( hSqlLyr != NULL )
{
OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
if ( hFeat != NULL )
{
const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
bSuccess = EQUAL( pszRet, "delete" );
QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
OGR_F_Destroy( hFeat );
}
}
else if ( CPLGetLastErrorType() != CE_None )
{
QgsDebugMsg( QString( "Return: %1" ).arg( CPLGetLastErrorMsg() ) );
}
OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
CPLPopErrorHandler();
OGR_DS_Destroy( ogrDataSource );

// This may have not worked if the file was opened in read-only mode,
// so retry in update mode
if ( !bSuccess )
{
QgsDebugMsg( "GPKG: Trying again" );
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "DELETE" );
ogrDataSource = OGROpen( TO8F( datasetName ), TRUE, NULL );
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
if ( ogrDataSource )
{
#ifdef QGISDEBUG
CPLPushErrorHandler( CPLQuietErrorHandler );
OGRLayerH hSqlLyr = OGR_DS_ExecuteSQL( ogrDataSource,
"PRAGMA journal_mode",
NULL, NULL );
CPLPopErrorHandler();
if ( hSqlLyr != NULL )
{
OGRFeatureH hFeat = OGR_L_GetNextFeature( hSqlLyr );
if ( hFeat != NULL )
{
const char* pszRet = OGR_F_GetFieldAsString( hFeat, 0 );
QgsDebugMsg( QString( "Return: %1" ).arg( pszRet ) );
OGR_F_Destroy( hFeat );
}
OGR_DS_ReleaseResultSet( ogrDataSource, hSqlLyr );
}
#endif
OGR_DS_Destroy( ogrDataSource );
}
}
}
else
{
OGR_DS_Destroy( ogrDataSource );
}
}

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

// first try to open in update mode (unless specified otherwise)
if ( !openReadOnly )
{
if ( QFileInfo( mFilePath ).suffix().compare( "gpkg", Qt::CaseInsensitive ) == 0 &&
IsLocalFile( mFilePath ) &&
!CPLGetConfigOption( "OGR_SQLITE_JOURNAL", NULL ) &&
QSettings().value( "/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 advertized not to work
// on network shares
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", "WAL" );
}
ogrDataSource = QgsOgrProviderUtils::OGROpenWrapper( TO8F( mFilePath ), true, &ogrDriver );
CPLSetThreadLocalConfigOption( "OGR_SQLITE_JOURNAL", NULL );
}

mValid = false;
if ( ogrDataSource )
Expand Down Expand Up @@ -3268,7 +3412,7 @@ void QgsOgrProvider::close()

if ( ogrDataSource )
{
OGR_DS_Destroy( ogrDataSource );
QgsOgrProviderUtils::OGRDestroyWrapper( ogrDataSource );
}
ogrDataSource = nullptr;
ogrLayer = nullptr;
Expand Down
6 changes: 6 additions & 0 deletions src/providers/ogr/qgsogrprovider.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ email : sherman at mrcc.com
* *
***************************************************************************/

#ifndef QGSOGRPROVIDER_H
#define QGSOGRPROVIDER_H

#include "QTextCodec"

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

static OGRDataSourceH OGROpenWrapper( const char* pszPath, bool bUpdate, OGRSFDriverH *phDriver );
static void OGRDestroyWrapper( OGRDataSourceH ogrDataSource );
};

#endif // QGSOGRPROVIDER_H
76 changes: 76 additions & 0 deletions tests/src/python/test_provider_ogr_gpkg.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ def GDAL_COMPUTE_VERSION(maj, min, rev):
return ((maj) * 1000000 + (min) * 10000 + (rev) * 100)


class ErrorReceiver():

def __init__(self):
self.msg = None

def receiveError(self, msg):
self.msg = msg


class TestPyQgsOGRProviderGpkg(unittest.TestCase):

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

def internalTestBug15351(self, orderClosing):

tmpfile = os.path.join(self.basetestpath, 'testBug15351.gpkg')
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometry(ogr.CreateGeometryFromWkt('POINT(0 0)'))
lyr.CreateFeature(f)
f = None
ds = None

vl = QgsVectorLayer(u'{}'.format(tmpfile), u'test', u'ogr')
self.assertTrue(vl.startEditing())
self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('Point (3 50)')))

# Iterate over features (will open a new OGR connection), but do not
# close the iterator for now
it = vl.getFeatures()
f = QgsFeature()
it.nextFeature(f)

if orderClosing == 'closeIter_commit_closeProvider':
it = None

# Commit changes
cbk = ErrorReceiver()
vl.dataProvider().raiseError.connect(cbk.receiveError)
self.assertTrue(vl.commitChanges())
self.assertIsNone(cbk.msg)

# Close layer and iterator in different orders
if orderClosing == 'closeIter_commit_closeProvider':
vl = None
elif orderClosing == 'commit_closeProvider_closeIter':
vl = None
it = None
else:
assert orderClosing == 'commit_closeIter_closeProvider'
it = None
vl = None

# Test that we succeeded restoring default journal mode, and we
# are not let in WAL mode.
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')

@unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
# We need GDAL 2.0 to issue PRAGMA journal_mode
# Note: for that case, we don't strictly need turning on WAL
def testBug15351_closeIter_commit_closeProvider(self):
self.internalTestBug15351('closeIter_commit_closeProvider')

@unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
# We need GDAL 2.0 to issue PRAGMA journal_mode
def testBug15351_closeIter_commit_closeProvider(self):
self.internalTestBug15351('closeIter_commit_closeProvider')

@unittest.expectedFailure(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 0, 0))
# We need GDAL 2.0 to issue PRAGMA journal_mode
def testBug15351_commit_closeIter_closeProvider(self):
self.internalTestBug15351('commit_closeIter_closeProvider')

if __name__ == '__main__':
unittest.main()
23 changes: 22 additions & 1 deletion tests/src/python/test_provider_spatialite.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import shutil
import tempfile

from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature
from qgis.core import QgsVectorLayer, QgsPoint, QgsFeature, QgsGeometry

from qgis.testing import start_app, unittest
from utilities import unitTestDataPath
Expand Down Expand Up @@ -347,6 +347,27 @@ def test_arrays(self):
self.assertEqual(read_back['ints'], new_f['ints'])
self.assertEqual(read_back['reals'], new_f['reals'])

# This test would fail. It would require turning on WAL
def XXXXXtestLocking(self):

temp_dbname = self.dbname + '.locking'
shutil.copy(self.dbname, temp_dbname)

vl = QgsVectorLayer("dbname=%s table=test_n (geometry)" % temp_dbname, "test_n", "spatialite")
self.assertTrue(vl.isValid())

self.assertTrue(vl.startEditing())
self.assertTrue(vl.changeGeometry(1, QgsGeometry.fromWkt('POLYGON((0 0,1 0,1 1,0 1,0 0))')))

# The iterator will take one extra connection
myiter = vl.getFeatures()

# Consume one feature but the iterator is still opened
f = next(myiter)
self.assertTrue(f.isValid())

self.assertTrue(vl.commitChanges())


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

0 comments on commit 7a685f1

Please sign in to comment.