[postgres] Avoid deadlocks in transactional editing

Can currently be triggered by using the field calculator to update a selection. While an iterator is active and the connection is locked, an update statement waits unsuccessfully for the same (locked) connection. This commit only locks the connection while an iterator is actually fetching data and not for its entire lifetime.
qgis · Apr 14, 2016 · 2e515a2 · 2e515a2
1 parent 07fcf24
commit 2e515a2
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 6 deletions.
diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -46,7 +46,6 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
   else
   {
     mConn = source->mTransactionConnection;
-    mConn->lock();
     mIsTransactionConnection = true;
   }
 
@@ -227,6 +226,8 @@ bool QgsPostgresFeatureIterator::fetchFeature( QgsFeature& feature )
   {
     QString fetch = QString( "FETCH FORWARD %1 FROM %2" ).arg( mFeatureQueueSize ).arg( mCursorName );
     QgsDebugMsgLevel( QString( "fetching %1 features." ).arg( mFeatureQueueSize ), 4 );
+
+    lock();
     if ( mConn->PQsendQuery( fetch ) == 0 ) // fetch features asynchronously
     {
       QgsMessageLog::logMessage( QObject::tr( "Fetching from cursor %1 failed\nDatabase error: %2" ).arg( mCursorName, mConn->PQerrorMessage() ), QObject::tr( "PostGIS" ) );
@@ -257,6 +258,7 @@ bool QgsPostgresFeatureIterator::fetchFeature( QgsFeature& feature )
         getFeature( queryResult, row, mFeatureQueue.back() );
       } // for each row in queue
     }
+    unlock();
   }
 
   if ( mFeatureQueue.empty() )
@@ -319,13 +321,28 @@ bool QgsPostgresFeatureIterator::prepareOrderBy( const QList<QgsFeatureRequest::
   return mOrderByCompiled;
 }
 
+void QgsPostgresFeatureIterator::lock()
+{
+  if ( mIsTransactionConnection )
+    mConn->lock();
+}
+
+void QgsPostgresFeatureIterator::unlock()
+{
+  if ( mIsTransactionConnection )
+    mConn->unlock();
+}
+
 bool QgsPostgresFeatureIterator::rewind()
 {
   if ( mClosed )
     return false;
 
   // move cursor to first record
+
+  lock();
   mConn->PQexecNR( QString( "move absolute 0 in %1" ).arg( mCursorName ) );
+  unlock();
   mFeatureQueue.clear();
   mFetched = 0;
   mLastFetch = false;
@@ -338,16 +355,14 @@ bool QgsPostgresFeatureIterator::close()
   if ( !mConn )
     return false;
 
+  lock();
   mConn->closeCursor( mCursorName );
+  unlock();
 
   if ( !mIsTransactionConnection )
   {
     QgsPostgresConnPool::instance()->releaseConnection( mConn );
   }
-  else
-  {
-    mConn->unlock();
-  }
   mConn = nullptr;
 
   while ( !mFeatureQueue.empty() )
@@ -598,15 +613,17 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString& whereClause, long
   if ( !orderBy.isEmpty() )
     query += QString( " ORDER BY %1 " ).arg( orderBy );
 
+  lock();
   if ( !mConn->openCursor( mCursorName, query ) )
   {
-
+    unlock();
     // reloading the fields might help next time around
     // TODO how to cleanly force reload of fields?  P->loadFields();
     if ( closeOnFail )
       close();
     return false;
   }
+  unlock();
 
   mLastFetch = false;
   return true;

diff --git a/src/providers/postgres/qgspostgresfeatureiterator.h b/src/providers/postgres/qgspostgresfeatureiterator.h
@@ -126,6 +126,9 @@ class QgsPostgresFeatureIterator : public QgsAbstractFeatureIteratorFromSource<Q
 
     virtual bool prepareOrderBy( const QList<QgsFeatureRequest::OrderByClause> &orderBys ) override;
 
+    inline void lock();
+    inline void unlock();
+
     bool mExpressionCompiled;
     bool mOrderByCompiled;
     bool mLastFetch;