Skip to content

Commit

Permalink
Fix libpq access from different threads
Browse files Browse the repository at this point in the history
  • Loading branch information
Hugo Mercier committed Dec 20, 2018
1 parent 5f8870a commit 507b4cb
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 37 deletions.
59 changes: 37 additions & 22 deletions src/providers/postgres/qgspostgresconn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 );

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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 ) );
}
Expand Down
14 changes: 10 additions & 4 deletions src/providers/postgres/qgspostgresconn.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -425,7 +431,7 @@ class QgsPostgresConn : public QObject

bool mTransaction;

QMutex mLock;
mutable QMutex mLock;
};

// clazy:excludeall=qstring-allocations
Expand Down
7 changes: 0 additions & 7 deletions src/providers/postgres/qgspostgresfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -401,9 +399,7 @@ bool QgsPostgresFeatureIterator::close()
if ( !mConn )
return false;

lock();
mConn->closeCursor( mCursorName );
unlock();

if ( !mIsTransactionConnection )
{
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions src/providers/postgres/qgspostgresprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) );
Expand Down
2 changes: 0 additions & 2 deletions src/providers/postgres/qgspostgrestransaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
{
Expand Down

0 comments on commit 507b4cb

Please sign in to comment.