[OGR provider] [FEATURE] Add support for transactions on GPKG databases

For complete support, it requires two GDAL fixes: - One to avoid feature count to be invalid when using ROLLBACK TO SAVEPOINT OSGeo/gdal@f73ec8c - Another one to avoid nasty issues, at least on Linux, with the POSIX advisory locks used by libsqlite that could be invalidated due to how GDAL could open files behind the back of libsqlite. The consequence of this could be the deletion of -wal and -shm files, which caused issues in QGIS (non working iterators when the edit is finished, and later edits in the same session not working). Those issues could appear for example if doing ogrinfo on the .gpkg opened by QGIS, or if opening two QGIS session on the .gpkg Both fixes are queued for GDAL 2.3.1
qgis · Jun 15, 2018 · 6a987f9 · 6a987f9
1 parent 370bac9
commit 6a987f9
Show file tree

Hide file tree

Showing 12 changed files with 562 additions and 57 deletions.
diff --git a/python/core/auto_generated/qgstransaction.sip.in b/python/core/auto_generated/qgstransaction.sip.in
@@ -146,6 +146,7 @@ returns the last created savepoint
 .. versionadded:: 3.0
 %End
 
+
   signals:
 
     void afterRollback();

diff --git a/src/core/qgstransaction.cpp b/src/core/qgstransaction.cpp
@@ -51,7 +51,7 @@ QgsTransaction *QgsTransaction::create( const QSet<QgsVectorLayer *> &layers )
 
   QgsVectorLayer *firstLayer = *layers.constBegin();
 
-  QString connStr = QgsDataSourceUri( firstLayer->source() ).connectionInfo( false );
+  QString connStr = connectionString( firstLayer->source() );
   QString providerKey = firstLayer->dataProvider()->name();
   std::unique_ptr<QgsTransaction> transaction( QgsTransaction::create( connStr, providerKey ) );
   if ( transaction )
@@ -81,6 +81,46 @@ QgsTransaction::~QgsTransaction()
   setLayerTransactionIds( nullptr );
 }
 
+// For the needs of the OGR provider with GeoPackage datasources, remove
+// any reference to layers in the connection string
+QString QgsTransaction::removeLayerIdOrName( const QString &str )
+{
+  QString res( str );
+
+  for ( int i = 0; i < 2; i++ )
+  {
+    int pos = res.indexOf( i == 0 ? QLatin1String( "|layername=" ) :  QLatin1String( "|layerid=" ) );
+    if ( pos >= 0 )
+    {
+      int end = res.indexOf( '|', pos + 1 );
+      if ( end >= 0 )
+      {
+        res = res.mid( 0, pos ) + res.mid( end );
+      }
+      else
+      {
+        res = res.mid( 0, pos );
+      }
+    }
+  }
+  return res;
+}
+
+///@cond PRIVATE
+QString QgsTransaction::connectionString( const QString &layerName )
+{
+  QString connString = QgsDataSourceUri( layerName ).connectionInfo( false );
+  // In the case of a OGR datasource, connectionInfo() will return an empty
+  // string. In that case, use the layer->source() itself, and strip any
+  // reference to layers from it.
+  if ( connString.isEmpty() )
+  {
+    connString = removeLayerIdOrName( layerName );
+  }
+  return connString;
+}
+///@endcond
+
 bool QgsTransaction::addLayer( QgsVectorLayer *layer )
 {
   if ( !layer )
@@ -90,17 +130,18 @@ bool QgsTransaction::addLayer( QgsVectorLayer *layer )
     return false;
 
   //test if provider supports transactions
-  if ( !layer->dataProvider() || ( layer->dataProvider()->capabilities() & QgsVectorDataProvider::TransactionSupport ) == 0 )
+  if ( !supportsTransaction( layer ) )
     return false;
 
   if ( layer->dataProvider()->transaction() )
     return false;
 
   //connection string not compatible
-  if ( QgsDataSourceUri( layer->source() ).connectionInfo( false ) != mConnString )
+
+  if ( connectionString( layer->source() ) != mConnString )
   {
     QgsDebugMsg( QString( "Couldn't start transaction because connection string for layer %1 : '%2' does not match '%3'" ).arg(
-                   layer->id(), QgsDataSourceUri( layer->source() ).connectionInfo( false ), mConnString ) );
+                   layer->id(), connectionString( layer->source() ), mConnString ) );
     return false;
   }
 
@@ -162,11 +203,11 @@ bool QgsTransaction::rollback( QString &errorMsg )
 
 bool QgsTransaction::supportsTransaction( const QgsVectorLayer *layer )
 {
-  std::unique_ptr< QLibrary > lib( QgsProviderRegistry::instance()->createProviderLibrary( layer->providerType() ) );
-  if ( !lib )
+  //test if provider supports transactions
+  if ( !layer->dataProvider() || ( layer->dataProvider()->capabilities() & QgsVectorDataProvider::TransactionSupport ) == 0 )
     return false;
 
-  return lib->resolve( "createTransaction" );
+  return true;
 }
 
 void QgsTransaction::onLayerDeleted()

diff --git a/src/core/qgstransaction.h b/src/core/qgstransaction.h
@@ -158,6 +158,11 @@ class CORE_EXPORT QgsTransaction : public QObject SIP_ABSTRACT
      */
     bool lastSavePointIsDirty() const { return mLastSavePointIsDirty; }
 
+///@cond PRIVATE
+    // For internal use only, or by QgsTransactionGroup
+    static QString connectionString( const QString &layerName ) SIP_SKIP;
+///@endcond
+
   signals:
 
     /**
@@ -188,6 +193,8 @@ class CORE_EXPORT QgsTransaction : public QObject SIP_ABSTRACT
 
     void setLayerTransactionIds( QgsTransaction *transaction );
 
+    static QString removeLayerIdOrName( const QString &str );
+
     virtual bool beginTransaction( QString &error, int statementTimeout ) = 0;
     virtual bool commitTransaction( QString &error ) = 0;
     virtual bool rollbackTransaction( QString &error ) = 0;

diff --git a/src/core/qgstransactiongroup.cpp b/src/core/qgstransactiongroup.cpp
@@ -33,8 +33,7 @@ bool QgsTransactionGroup::addLayer( QgsVectorLayer *layer )
   if ( !QgsTransaction::supportsTransaction( layer ) )
     return false;
 
-  QString connString = QgsDataSourceUri( layer->source() ).connectionInfo();
-
+  QString connString = QgsTransaction::connectionString( layer->source() );
   if ( mConnString.isEmpty() )
   {
     mConnString = connString;
@@ -74,6 +73,8 @@ void QgsTransactionGroup::onEditingStarted()
     return;
 
   mTransaction.reset( QgsTransaction::create( mConnString, mProviderKey ) );
+  if ( !mTransaction )
+    return;
 
   QString errorMsg;
   mTransaction->begin( errorMsg );

diff --git a/src/providers/ogr/CMakeLists.txt b/src/providers/ogr/CMakeLists.txt
@@ -9,6 +9,7 @@ SET (OGR_SRCS
     qgsgeopackagerasterwritertask.cpp
     qgsogrdbconnection.cpp
     qgsogrdbtablemodel.cpp
+    qgsogrtransaction.cpp
 )
 
 SET(OGR_MOC_HDRS
@@ -19,6 +20,7 @@ SET(OGR_MOC_HDRS
     qgsgeopackagerasterwritertask.h
     qgsogrdbconnection.h
     qgsogrdbtablemodel.h
+    qgsogrtransaction.h
 )
 
 IF (WITH_GUI)

diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -26,6 +26,7 @@
 #include "qgssettings.h"
 #include "qgsexception.h"
 #include "qgswkbtypes.h"
+#include "qgsogrtransaction.h"
 
 #include <QTextCodec>
 #include <QFile>
@@ -42,38 +43,51 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
   : QgsAbstractFeatureIteratorFromSource<QgsOgrFeatureSource>( source, ownSource, request )
   , mFilterFids( mRequest.filterFids() )
   , mFilterFidsIt( mFilterFids.constBegin() )
+  , mSharedDS( source->mSharedDS )
 {
-  //QgsDebugMsg( "Feature iterator of " + mSource->mLayerName + ": acquiring connection");
-  mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ) );
-  if ( !mConn->ds )
+  if ( mSharedDS )
   {
-    return;
-  }
-
-  if ( mSource->mLayerName.isNull() )
-  {
-    mOgrLayer = GDALDatasetGetLayer( mConn->ds, mSource->mLayerIndex );
+    mOgrLayer = mSharedDS->createSQLResultLayer( mSource->mEncoding, mSource->mLayerName, mSource->mLayerIndex );
+    if ( !mOgrLayer )
+    {
+      return;
+    }
   }
   else
   {
-    mOgrLayer = GDALDatasetGetLayerByName( mConn->ds, mSource->mLayerName.toUtf8().constData() );
-  }
-  if ( !mOgrLayer )
-  {
-    return;
-  }
+    //QgsDebugMsg( "Feature iterator of " + mSource->mLayerName + ": acquiring connection");
+    mConn = QgsOgrConnPool::instance()->acquireConnection( QgsOgrProviderUtils::connectionPoolId( mSource->mDataSource ) );
+    if ( !mConn->ds )
+    {
+      return;
+    }
 
-  if ( !mSource->mSubsetString.isEmpty() )
-  {
-    mOgrOrigLayer = mOgrLayer;
-    mOgrLayerWithFid = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, QString(), true, &mOrigFidAdded );
-    mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString, true, &mOrigFidAdded );
+    if ( mSource->mLayerName.isNull() )
+    {
+      mOgrLayer = GDALDatasetGetLayer( mConn->ds, mSource->mLayerIndex );
+    }
+    else
+    {
+      mOgrLayer = GDALDatasetGetLayerByName( mConn->ds, mSource->mLayerName.toUtf8().constData() );
+    }
     if ( !mOgrLayer )
     {
-      close();
       return;
     }
+
+    if ( !mSource->mSubsetString.isEmpty() )
+    {
+      mOgrOrigLayer = mOgrLayer;
+      mOgrLayerWithFid = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, QString(), true, &mOrigFidAdded );
+      mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString, true, &mOrigFidAdded );
+      if ( !mOgrLayer )
+      {
+        close();
+        return;
+      }
+    }
   }
+  QMutexLocker locker( mSharedDS ? &mSharedDS->mutex() : nullptr );
 
   if ( mRequest.destinationCrs().isValid() && mRequest.destinationCrs() != mSource->mCrs )
   {
@@ -237,6 +251,8 @@ bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature &fea
 
 bool QgsOgrFeatureIterator::fetchFeature( QgsFeature &feature )
 {
+  QMutexLocker locker( mSharedDS ? &mSharedDS->mutex() : nullptr );
+
   feature.setValid( false );
 
   if ( mClosed || !mOgrLayer )
@@ -286,6 +302,8 @@ bool QgsOgrFeatureIterator::fetchFeature( QgsFeature &feature )
 
 bool QgsOgrFeatureIterator::rewind()
 {
+  QMutexLocker locker( mSharedDS ? &mSharedDS->mutex() : nullptr );
+
   if ( mClosed || !mOgrLayer )
     return false;
 
@@ -299,6 +317,20 @@ bool QgsOgrFeatureIterator::rewind()
 
 bool QgsOgrFeatureIterator::close()
 {
+  if ( mSharedDS )
+  {
+    iteratorClosed();
+
+    if ( mOgrLayer )
+    {
+      mSharedDS->releaseResultSet( mOgrLayer );
+      mOgrLayer = nullptr;
+    }
+    mSharedDS.reset();
+    mClosed = true;
+    return true;
+  }
+
   if ( !mConn )
     return false;
 
@@ -444,7 +476,12 @@ QgsOgrFeatureSource::QgsOgrFeatureSource( const QgsOgrProvider *p )
   , mDriverName( p->mGDALDriverName )
   , mCrs( p->crs() )
   , mWkbType( p->wkbType() )
+  , mSharedDS( nullptr )
 {
+  if ( p->mTransaction )
+  {
+    mSharedDS = p->mTransaction->sharedDS();
+  }
   for ( int i = ( p->mFirstFieldIsFid ) ? 1 : 0; i < mFields.size(); i++ )
     mFieldsWithoutFid.append( mFields.at( i ) );
   QgsOgrConnPool::instance()->ref( QgsOgrProviderUtils::connectionPoolId( mDataSource ) );

diff --git a/src/providers/ogr/qgsogrfeatureiterator.h b/src/providers/ogr/qgsogrfeatureiterator.h
@@ -21,8 +21,12 @@
 
 #include <ogr_api.h>
 
+#include <memory>
+
 class QgsOgrFeatureIterator;
 class QgsOgrProvider;
+class QgsOgrDataset;
+using QgsOgrDatasetSharedPtr = std::shared_ptr< QgsOgrDataset>;
 
 class QgsOgrFeatureSource : public QgsAbstractFeatureSource
 {
@@ -45,6 +49,7 @@ class QgsOgrFeatureSource : public QgsAbstractFeatureSource
     QString mDriverName;
     QgsCoordinateReferenceSystem mCrs;
     QgsWkbTypes::Type mWkbType = QgsWkbTypes::Unknown;
+    QgsOgrDatasetSharedPtr mSharedDS = nullptr;
 
     friend class QgsOgrFeatureIterator;
     friend class QgsOgrExpressionCompiler;
@@ -87,6 +92,7 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsOgr
 
     QgsRectangle mFilterRect;
     QgsCoordinateTransform mTransform;
+    QgsOgrDatasetSharedPtr mSharedDS = nullptr;
 
     bool fetchFeatureWithId( QgsFeatureId id, QgsFeature &feature ) const;
 };