Merge pull request #8700 from mhugo/fix_postgres_transaction_lock

Fix libpq access from different threads
qgis · Dec 21, 2018 · 05f949b · 05f949b
2 parents d79c212 + 507b4cb
commit 05f949b
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 37 deletions.
diff --git a/src/providers/postgres/qgspostgresconn.cpp b/src/providers/postgres/qgspostgresconn.cpp
@@ -211,6 +211,7 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
   , mNextCursorId( 0 )
   , mShared( shared )
   , mTransaction( transaction )
+  , mLock( QMutex::Recursive )
 {
   QgsDebugMsg( QStringLiteral( "New PostgreSQL connection for " ) + conninfo );
 
@@ -1065,33 +1066,37 @@ PGresult *QgsPostgresConn::PQexec( const QString &query, bool logError ) const
   }
 
   QgsDebugMsgLevel( QStringLiteral( "Executing SQL: %1" ).arg( query ), 3 );
-  PGresult *res = ::PQexec( mConn, query.toUtf8() );
-
-  if ( res )
+  PGresult *res = nullptr;
   {
-    int errorStatus = PQresultStatus( res );
-    if ( errorStatus != PGRES_COMMAND_OK && errorStatus != PGRES_TUPLES_OK )
+    QMutexLocker locker( &mLock );
+    res = ::PQexec( mConn, query.toUtf8() );
+
+    if ( res )
     {
-      if ( logError )
-      {
-        QgsMessageLog::logMessage( tr( "Erroneous query: %1 returned %2 [%3]" )
-                                   .arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ),
-                                   tr( "PostGIS" ) );
-      }
-      else
+      int errorStatus = PQresultStatus( res );
+      if ( errorStatus != PGRES_COMMAND_OK && errorStatus != PGRES_TUPLES_OK )
       {
-        QgsDebugMsg( QStringLiteral( "Not logged erroneous query: %1 returned %2 [%3]" )
-                     .arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ) );
+        if ( logError )
+        {
+          QgsMessageLog::logMessage( tr( "Erroneous query: %1 returned %2 [%3]" )
+                                     .arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ),
+                                     tr( "PostGIS" ) );
+        }
+        else
+        {
+          QgsDebugMsg( QStringLiteral( "Not logged erroneous query: %1 returned %2 [%3]" )
+                       .arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ) );
+        }
       }
     }
-  }
-  else if ( logError )
-  {
-    QgsMessageLog::logMessage( tr( "Query failed: %1\nError: no result buffer" ).arg( query ), tr( "PostGIS" ) );
-  }
-  else
-  {
-    QgsDebugMsg( QStringLiteral( "Not logged query failed: %1\nError: no result buffer" ).arg( query ) );
+    else if ( logError )
+    {
+      QgsMessageLog::logMessage( tr( "Query failed: %1\nError: no result buffer" ).arg( query ), tr( "PostGIS" ) );
+    }
+    else
+    {
+      QgsDebugMsg( QStringLiteral( "Not logged query failed: %1\nError: no result buffer" ).arg( query ) );
+    }
   }
 
   return res;
@@ -1194,11 +1199,15 @@ PGresult *QgsPostgresConn::PQgetResult()
 
 PGresult *QgsPostgresConn::PQprepare( const QString &stmtName, const QString &query, int nParams, const Oid *paramTypes )
 {
+  QMutexLocker locker( &mLock );
+
   return ::PQprepare( mConn, stmtName.toUtf8(), query.toUtf8(), nParams, paramTypes );
 }
 
 PGresult *QgsPostgresConn::PQexecPrepared( const QString &stmtName, const QStringList &params )
 {
+  QMutexLocker locker( &mLock );
+
   const char **param = new const char *[ params.size()];
   QList<QByteArray> qparam;
 
@@ -1222,19 +1231,25 @@ PGresult *QgsPostgresConn::PQexecPrepared( const QString &stmtName, const QStrin
 
 void QgsPostgresConn::PQfinish()
 {
+  QMutexLocker locker( &mLock );
+
   Q_ASSERT( mConn );
   ::PQfinish( mConn );
   mConn = nullptr;
 }
 
 int QgsPostgresConn::PQstatus() const
 {
+  QMutexLocker locker( &mLock );
+
   Q_ASSERT( mConn );
   return ::PQstatus( mConn );
 }
 
 QString QgsPostgresConn::PQerrorMessage() const
 {
+  QMutexLocker locker( &mLock );
+
   Q_ASSERT( mConn );
   return QString::fromUtf8( ::PQerrorMessage( mConn ) );
 }

diff --git a/src/providers/postgres/qgspostgresconn.h b/src/providers/postgres/qgspostgresconn.h
@@ -244,16 +244,22 @@ class QgsPostgresConn : public QObject
     // libpq wrapper
     //
 
-    // run a query and check for errors
+    // run a query and check for errors, thread-safe
     PGresult *PQexec( const QString &query, bool logError = true ) const;
     void PQfinish();
     QString PQerrorMessage() const;
-    int PQsendQuery( const QString &query );
     int PQstatus() const;
-    PGresult *PQgetResult();
     PGresult *PQprepare( const QString &stmtName, const QString &query, int nParams, const Oid *paramTypes );
     PGresult *PQexecPrepared( const QString &stmtName, const QStringList &params );
 
+    //! PQsendQuery is used for asynchronous queries (with PQgetResult)
+    //! Thread safety must be ensured by the caller by calling QgsPostgresConn::lock() and QgsPostgresConn::unlock()
+    int PQsendQuery( const QString &query );
+
+    //! PQgetResult is used for asynchronous queries (with PQsendQuery)
+    //! Thread safety must be ensured by the caller by calling QgsPostgresConn::lock() and QgsPostgresConn::unlock()
+    PGresult *PQgetResult();
+
     bool begin();
     bool commit();
     bool rollback();
@@ -425,7 +431,7 @@ class QgsPostgresConn : public QObject
 
     bool mTransaction;
 
-    QMutex mLock;
+    mutable QMutex mLock;
 };
 
 // clazy:excludeall=qstring-allocations

diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -386,9 +386,7 @@ bool QgsPostgresFeatureIterator::rewind()
 
   // move cursor to first record
 
-  lock();
   mConn->PQexecNR( QStringLiteral( "move absolute 0 in %1" ).arg( mCursorName ) );
-  unlock();
   mFeatureQueue.clear();
   mFetched = 0;
   mLastFetch = false;
@@ -401,9 +399,7 @@ bool QgsPostgresFeatureIterator::close()
   if ( !mConn )
     return false;
 
-  lock();
   mConn->closeCursor( mCursorName );
-  unlock();
 
   if ( !mIsTransactionConnection )
   {
@@ -651,17 +647,14 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString &whereClause, long
   if ( !orderBy.isEmpty() )
     query += QStringLiteral( " 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/qgspostgresprovider.cpp b/src/providers/postgres/qgspostgresprovider.cpp
@@ -4104,9 +4104,7 @@ QgsCoordinateReferenceSystem QgsPostgresProvider::crs() const
     else
     {
       QgsPostgresConn *conn = connectionRO();
-      conn->lock();
       QgsPostgresResult result( conn->PQexec( QStringLiteral( "SELECT proj4text FROM spatial_ref_sys WHERE srid=%1" ).arg( srid ) ) );
-      conn->unlock();
       if ( result.PQresultStatus() == PGRES_TUPLES_OK )
       {
         srs = QgsCoordinateReferenceSystem::fromProj4( result.PQgetvalue( 0, 0 ) );

diff --git a/src/providers/postgres/qgspostgrestransaction.cpp b/src/providers/postgres/qgspostgrestransaction.cpp
@@ -71,9 +71,7 @@ bool QgsPostgresTransaction::executeSql( const QString &sql, QString &errorMsg,
   }
 
   QgsDebugMsg( QStringLiteral( "Transaction sql: %1" ).arg( sql ) );
-  mConn->lock();
   QgsPostgresResult r( mConn->PQexec( sql, true ) );
-  mConn->unlock();
   if ( r.PQresultStatus() == PGRES_BAD_RESPONSE ||
        r.PQresultStatus() == PGRES_FATAL_ERROR )
   {