Skip to content

Commit 05f949b

Browse files
author
Hugo Mercier
authoredDec 21, 2018
Merge pull request #8700 from mhugo/fix_postgres_transaction_lock
Fix libpq access from different threads
·
final-3_4_15final-3_4_3
2 parents d79c212 + 507b4cb commit 05f949b

File tree

5 files changed

+47
-37
lines changed

5 files changed

+47
-37
lines changed
 

‎src/providers/postgres/qgspostgresconn.cpp

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
211211
, mNextCursorId( 0 )
212212
, mShared( shared )
213213
, mTransaction( transaction )
214+
, mLock( QMutex::Recursive )
214215
{
215216
QgsDebugMsg( QStringLiteral( "New PostgreSQL connection for " ) + conninfo );
216217

@@ -1065,33 +1066,37 @@ PGresult *QgsPostgresConn::PQexec( const QString &query, bool logError ) const
10651066
}
10661067

10671068
QgsDebugMsgLevel( QStringLiteral( "Executing SQL: %1" ).arg( query ), 3 );
1068-
PGresult *res = ::PQexec( mConn, query.toUtf8() );
1069-
1070-
if ( res )
1069+
PGresult *res = nullptr;
10711070
{
1072-
int errorStatus = PQresultStatus( res );
1073-
if ( errorStatus != PGRES_COMMAND_OK && errorStatus != PGRES_TUPLES_OK )
1071+
QMutexLocker locker( &mLock );
1072+
res = ::PQexec( mConn, query.toUtf8() );
1073+
1074+
if ( res )
10741075
{
1075-
if ( logError )
1076-
{
1077-
QgsMessageLog::logMessage( tr( "Erroneous query: %1 returned %2 [%3]" )
1078-
.arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ),
1079-
tr( "PostGIS" ) );
1080-
}
1081-
else
1076+
int errorStatus = PQresultStatus( res );
1077+
if ( errorStatus != PGRES_COMMAND_OK && errorStatus != PGRES_TUPLES_OK )
10821078
{
1083-
QgsDebugMsg( QStringLiteral( "Not logged erroneous query: %1 returned %2 [%3]" )
1084-
.arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ) );
1079+
if ( logError )
1080+
{
1081+
QgsMessageLog::logMessage( tr( "Erroneous query: %1 returned %2 [%3]" )
1082+
.arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ),
1083+
tr( "PostGIS" ) );
1084+
}
1085+
else
1086+
{
1087+
QgsDebugMsg( QStringLiteral( "Not logged erroneous query: %1 returned %2 [%3]" )
1088+
.arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ) );
1089+
}
10851090
}
10861091
}
1087-
}
1088-
else if ( logError )
1089-
{
1090-
QgsMessageLog::logMessage( tr( "Query failed: %1\nError: no result buffer" ).arg( query ), tr( "PostGIS" ) );
1091-
}
1092-
else
1093-
{
1094-
QgsDebugMsg( QStringLiteral( "Not logged query failed: %1\nError: no result buffer" ).arg( query ) );
1092+
else if ( logError )
1093+
{
1094+
QgsMessageLog::logMessage( tr( "Query failed: %1\nError: no result buffer" ).arg( query ), tr( "PostGIS" ) );
1095+
}
1096+
else
1097+
{
1098+
QgsDebugMsg( QStringLiteral( "Not logged query failed: %1\nError: no result buffer" ).arg( query ) );
1099+
}
10951100
}
10961101

10971102
return res;
@@ -1194,11 +1199,15 @@ PGresult *QgsPostgresConn::PQgetResult()
11941199

11951200
PGresult *QgsPostgresConn::PQprepare( const QString &stmtName, const QString &query, int nParams, const Oid *paramTypes )
11961201
{
1202+
QMutexLocker locker( &mLock );
1203+
11971204
return ::PQprepare( mConn, stmtName.toUtf8(), query.toUtf8(), nParams, paramTypes );
11981205
}
11991206

12001207
PGresult *QgsPostgresConn::PQexecPrepared( const QString &stmtName, const QStringList &params )
12011208
{
1209+
QMutexLocker locker( &mLock );
1210+
12021211
const char **param = new const char *[ params.size()];
12031212
QList<QByteArray> qparam;
12041213

@@ -1222,19 +1231,25 @@ PGresult *QgsPostgresConn::PQexecPrepared( const QString &stmtName, const QStrin
12221231

12231232
void QgsPostgresConn::PQfinish()
12241233
{
1234+
QMutexLocker locker( &mLock );
1235+
12251236
Q_ASSERT( mConn );
12261237
::PQfinish( mConn );
12271238
mConn = nullptr;
12281239
}
12291240

12301241
int QgsPostgresConn::PQstatus() const
12311242
{
1243+
QMutexLocker locker( &mLock );
1244+
12321245
Q_ASSERT( mConn );
12331246
return ::PQstatus( mConn );
12341247
}
12351248

12361249
QString QgsPostgresConn::PQerrorMessage() const
12371250
{
1251+
QMutexLocker locker( &mLock );
1252+
12381253
Q_ASSERT( mConn );
12391254
return QString::fromUtf8( ::PQerrorMessage( mConn ) );
12401255
}

‎src/providers/postgres/qgspostgresconn.h

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -244,16 +244,22 @@ class QgsPostgresConn : public QObject
244244
// libpq wrapper
245245
//
246246

247-
// run a query and check for errors
247+
// run a query and check for errors, thread-safe
248248
PGresult *PQexec( const QString &query, bool logError = true ) const;
249249
void PQfinish();
250250
QString PQerrorMessage() const;
251-
int PQsendQuery( const QString &query );
252251
int PQstatus() const;
253-
PGresult *PQgetResult();
254252
PGresult *PQprepare( const QString &stmtName, const QString &query, int nParams, const Oid *paramTypes );
255253
PGresult *PQexecPrepared( const QString &stmtName, const QStringList &params );
256254

255+
//! PQsendQuery is used for asynchronous queries (with PQgetResult)
256+
//! Thread safety must be ensured by the caller by calling QgsPostgresConn::lock() and QgsPostgresConn::unlock()
257+
int PQsendQuery( const QString &query );
258+
259+
//! PQgetResult is used for asynchronous queries (with PQsendQuery)
260+
//! Thread safety must be ensured by the caller by calling QgsPostgresConn::lock() and QgsPostgresConn::unlock()
261+
PGresult *PQgetResult();
262+
257263
bool begin();
258264
bool commit();
259265
bool rollback();
@@ -425,7 +431,7 @@ class QgsPostgresConn : public QObject
425431

426432
bool mTransaction;
427433

428-
QMutex mLock;
434+
mutable QMutex mLock;
429435
};
430436

431437
// clazy:excludeall=qstring-allocations

‎src/providers/postgres/qgspostgresfeatureiterator.cpp

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -386,9 +386,7 @@ bool QgsPostgresFeatureIterator::rewind()
386386

387387
// move cursor to first record
388388

389-
lock();
390389
mConn->PQexecNR( QStringLiteral( "move absolute 0 in %1" ).arg( mCursorName ) );
391-
unlock();
392390
mFeatureQueue.clear();
393391
mFetched = 0;
394392
mLastFetch = false;
@@ -401,9 +399,7 @@ bool QgsPostgresFeatureIterator::close()
401399
if ( !mConn )
402400
return false;
403401

404-
lock();
405402
mConn->closeCursor( mCursorName );
406-
unlock();
407403

408404
if ( !mIsTransactionConnection )
409405
{
@@ -651,17 +647,14 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString &whereClause, long
651647
if ( !orderBy.isEmpty() )
652648
query += QStringLiteral( " ORDER BY %1 " ).arg( orderBy );
653649

654-
lock();
655650
if ( !mConn->openCursor( mCursorName, query ) )
656651
{
657-
unlock();
658652
// reloading the fields might help next time around
659653
// TODO how to cleanly force reload of fields? P->loadFields();
660654
if ( closeOnFail )
661655
close();
662656
return false;
663657
}
664-
unlock();
665658

666659
mLastFetch = false;
667660
return true;

‎src/providers/postgres/qgspostgresprovider.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4104,9 +4104,7 @@ QgsCoordinateReferenceSystem QgsPostgresProvider::crs() const
41044104
else
41054105
{
41064106
QgsPostgresConn *conn = connectionRO();
4107-
conn->lock();
41084107
QgsPostgresResult result( conn->PQexec( QStringLiteral( "SELECT proj4text FROM spatial_ref_sys WHERE srid=%1" ).arg( srid ) ) );
4109-
conn->unlock();
41104108
if ( result.PQresultStatus() == PGRES_TUPLES_OK )
41114109
{
41124110
srs = QgsCoordinateReferenceSystem::fromProj4( result.PQgetvalue( 0, 0 ) );

‎src/providers/postgres/qgspostgrestransaction.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,7 @@ bool QgsPostgresTransaction::executeSql( const QString &sql, QString &errorMsg,
7171
}
7272

7373
QgsDebugMsg( QStringLiteral( "Transaction sql: %1" ).arg( sql ) );
74-
mConn->lock();
7574
QgsPostgresResult r( mConn->PQexec( sql, true ) );
76-
mConn->unlock();
7775
if ( r.PQresultStatus() == PGRES_BAD_RESPONSE ||
7876
r.PQresultStatus() == PGRES_FATAL_ERROR )
7977
{

0 commit comments

Comments
 (0)
Please sign in to comment.