Skip to content

Commit 648672d

Browse files
author
Hugo Mercier
committedJan 14, 2019
Fix libpq access from different threads
This is a forward port from 05f949b, following PR #8700. In addition, two other fixes are added (that will be backported to 3.4): - PQexecNR, openCursor and closeCursor are protected - uniqueCursorName is also protected (I stumbled accross a bug where the cursor name was reused by two different threads)
·
ltr-3_40final-3_6_0
1 parent c263750 commit 648672d

File tree

5 files changed

+58
-37
lines changed

5 files changed

+58
-37
lines changed
 

‎src/providers/postgres/qgspostgresconn.cpp

Lines changed: 44 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,40 +1066,46 @@ 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+
int errorStatus = PQresultStatus( res );
1077+
if ( errorStatus != PGRES_COMMAND_OK && errorStatus != PGRES_TUPLES_OK )
10761078
{
1077-
QgsMessageLog::logMessage( tr( "Erroneous query: %1 returned %2 [%3]" )
1078-
.arg( query ).arg( errorStatus ).arg( PQresultErrorMessage( res ) ),
1079-
tr( "PostGIS" ) );
1080-
}
1081-
else
1082-
{
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;
10981103
}
10991104

11001105
bool QgsPostgresConn::openCursor( const QString &cursorName, const QString &sql )
11011106
{
1107+
QMutexLocker locker( &mLock ); // to protect access to mOpenCursors
1108+
11021109
if ( mOpenCursors++ == 0 && !mTransaction )
11031110
{
11041111
QgsDebugMsgLevel( QStringLiteral( "Starting read-only transaction: %1" ).arg( mPostgresqlVersion ), 4 );
@@ -1114,6 +1121,8 @@ bool QgsPostgresConn::openCursor( const QString &cursorName, const QString &sql
11141121

11151122
bool QgsPostgresConn::closeCursor( const QString &cursorName )
11161123
{
1124+
QMutexLocker locker( &mLock ); // to protect access to mOpenCursors
1125+
11171126
if ( !PQexecNR( QStringLiteral( "CLOSE %1" ).arg( cursorName ) ) )
11181127
return false;
11191128

@@ -1128,11 +1137,14 @@ bool QgsPostgresConn::closeCursor( const QString &cursorName )
11281137

11291138
QString QgsPostgresConn::uniqueCursorName()
11301139
{
1140+
QMutexLocker locker( &mLock ); // to protect access to mNextCursorId
11311141
return QStringLiteral( "qgis_%1" ).arg( ++mNextCursorId );
11321142
}
11331143

11341144
bool QgsPostgresConn::PQexecNR( const QString &query, bool retry )
11351145
{
1146+
QMutexLocker locker( &mLock ); // to protect access to mOpenCursors
1147+
11361148
QgsPostgresResult res( PQexec( query, false ) );
11371149

11381150
ExecStatusType errorStatus = res.PQresultStatus();
@@ -1194,11 +1206,15 @@ PGresult *QgsPostgresConn::PQgetResult()
11941206

11951207
PGresult *QgsPostgresConn::PQprepare( const QString &stmtName, const QString &query, int nParams, const Oid *paramTypes )
11961208
{
1209+
QMutexLocker locker( &mLock );
1210+
11971211
return ::PQprepare( mConn, stmtName.toUtf8(), query.toUtf8(), nParams, paramTypes );
11981212
}
11991213

12001214
PGresult *QgsPostgresConn::PQexecPrepared( const QString &stmtName, const QStringList &params )
12011215
{
1216+
QMutexLocker locker( &mLock );
1217+
12021218
const char **param = new const char *[ params.size()];
12031219
QList<QByteArray> qparam;
12041220

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

12231239
void QgsPostgresConn::PQfinish()
12241240
{
1241+
QMutexLocker locker( &mLock );
1242+
12251243
Q_ASSERT( mConn );
12261244
::PQfinish( mConn );
12271245
mConn = nullptr;
12281246
}
12291247

12301248
int QgsPostgresConn::PQstatus() const
12311249
{
1250+
QMutexLocker locker( &mLock );
1251+
12321252
Q_ASSERT( mConn );
12331253
return ::PQstatus( mConn );
12341254
}
12351255

12361256
QString QgsPostgresConn::PQerrorMessage() const
12371257
{
1258+
QMutexLocker locker( &mLock );
1259+
12381260
Q_ASSERT( mConn );
12391261
return QString::fromUtf8( ::PQerrorMessage( mConn ) );
12401262
}

‎src/providers/postgres/qgspostgresconn.h

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -244,16 +244,26 @@ 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+
/**
256+
* PQsendQuery is used for asynchronous queries (with PQgetResult)
257+
* Thread safety must be ensured by the caller by calling QgsPostgresConn::lock() and QgsPostgresConn::unlock()
258+
*/
259+
int PQsendQuery( const QString &query );
260+
261+
/**
262+
* PQgetResult is used for asynchronous queries (with PQsendQuery)
263+
* Thread safety must be ensured by the caller by calling QgsPostgresConn::lock() and QgsPostgresConn::unlock()
264+
*/
265+
PGresult *PQgetResult();
266+
257267
bool begin();
258268
bool commit();
259269
bool rollback();
@@ -425,7 +435,7 @@ class QgsPostgresConn : public QObject
425435

426436
bool mTransaction;
427437

428-
QMutex mLock;
438+
mutable QMutex mLock;
429439
};
430440

431441
// 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
@@ -4095,9 +4095,7 @@ QgsCoordinateReferenceSystem QgsPostgresProvider::crs() const
40954095
else
40964096
{
40974097
QgsPostgresConn *conn = connectionRO();
4098-
conn->lock();
40994098
QgsPostgresResult result( conn->PQexec( QStringLiteral( "SELECT proj4text FROM spatial_ref_sys WHERE srid=%1" ).arg( srid ) ) );
4100-
conn->unlock();
41014099
if ( result.PQresultStatus() == PGRES_TUPLES_OK )
41024100
{
41034101
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.