Skip to content
Permalink
Browse files
Merge pull request #2069 from manisandro/ogrpool
Add connection pool for OGR provider
  • Loading branch information
mhugent committed May 26, 2015
2 parents f31abe4 + b4f4663 commit 72c9830178e3903b77a36d30f3e69a5e0ca6533e
@@ -37,6 +37,8 @@
* - void qgsConnectionPool_ConnectionCreate(QString name, T& c) ... create a new connection
* - void qgsConnectionPool_ConnectionDestroy(T c) ... destroy the connection
* - QString qgsConnectionPool_ConnectionToName(T c) ... lookup connection's name (path)
* - void qgsConnectionPool_InvalidateConnection(T c) ... flag a connection as invalid
* - bool qgsConnectionPool_ConnectionIsValid(T c) ... return whether a connection is valid
*
* Because of issues with templates and QObject's signals and slots, this class only provides helper functions for QObject-related
* functionality - the place which uses the template is resonsible for:
@@ -87,6 +89,11 @@ class QgsConnectionPoolGroup
if ( !conns.isEmpty() )
{
Item i = conns.pop();
if ( !qgsConnectionPool_ConnectionIsValid( i.c ) )
{
qgsConnectionPool_ConnectionDestroy( i.c );
qgsConnectionPool_ConnectionCreate( connInfo, i.c );
}

// no need to run if nothing can expire
if ( conns.isEmpty() )
@@ -95,6 +102,8 @@ class QgsConnectionPoolGroup
QMetaObject::invokeMethod( expirationTimer->parent(), "stopExpirationTimer" );
}

acquiredConns.append( i.c );

return i.c;
}
}
@@ -108,11 +117,14 @@ class QgsConnectionPoolGroup
return 0;
}

acquiredConns.append( c );
return c;
}

void release( T conn )
{
acquiredConns.removeAll( conn );

connMutex.lock();
Item i;
i.c = conn;
@@ -130,6 +142,18 @@ class QgsConnectionPoolGroup
sem.release(); // this can unlock a thread waiting in acquire()
}

void invalidateConnections()
{
connMutex.lock();
foreach ( Item i, conns )
{
qgsConnectionPool_InvalidateConnection( i.c );
}
foreach ( T c, acquiredConns )
qgsConnectionPool_InvalidateConnection( c );
connMutex.unlock();
}

protected:

void initTimer( QObject* parent )
@@ -174,6 +198,7 @@ class QgsConnectionPoolGroup

QString connInfo;
QStack<Item> conns;
QList<T> acquiredConns;
QMutex connMutex;
QSemaphore sem;
QTimer* expirationTimer;
@@ -232,8 +257,6 @@ class QgsConnectionPool

protected:
T_Groups mGroups;

private:
QMutex mMutex;
};

@@ -1,7 +1,7 @@

SET (OGR_SRCS qgsogrprovider.cpp qgsogrdataitems.cpp qgsogrfeatureiterator.cpp qgsogrgeometrysimplifier.cpp)
SET (OGR_SRCS qgsogrprovider.cpp qgsogrdataitems.cpp qgsogrfeatureiterator.cpp qgsogrgeometrysimplifier.cpp qgsogrconnpool.cpp)

SET(OGR_MOC_HDRS qgsogrprovider.h qgsogrdataitems.h)
SET(OGR_MOC_HDRS qgsogrprovider.h qgsogrdataitems.h qgsogrconnpool.h)

########################################################
# Build
@@ -0,0 +1,41 @@
/***************************************************************************
qgsogrconnpool.cpp
---------------------
begin : May 2015
copyright : (C) 2015 by Sandro Mani
email : smani at sourcepole dot ch
***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/

#include "qgsogrconnpool.h"


QgsOgrConnPool* QgsOgrConnPool::instance()
{
static QgsOgrConnPool sInstance;
return &sInstance;
}

QgsOgrConnPool::QgsOgrConnPool() : QgsConnectionPool<QgsOgrConn*, QgsOgrConnPoolGroup>()
{
QgsDebugCall;
}

QgsOgrConnPool::~QgsOgrConnPool()
{
QgsDebugCall;
}

void QgsOgrConnPool::invalidateHandles( const QString& connInfo )
{
mMutex.lock();
if ( mGroups.contains( connInfo ) )
mGroups[connInfo]->invalidateConnections();
mMutex.unlock();
}
@@ -0,0 +1,93 @@
/***************************************************************************
qgsogrconnpool.h
---------------------
begin : May 2015
copyright : (C) 2015 by Sandro Mani
email : smani at sourcepole dot ch
***************************************************************************
* *
* This program is free software; you can redistribute it and/or modify *
* it under the terms of the GNU General Public License as published by *
* the Free Software Foundation; either version 2 of the License, or *
* (at your option) any later version. *
* *
***************************************************************************/

#ifndef QGSOGRCONNPOOL_H
#define QGSOGRCONNPOOL_H

#include "qgsconnectionpool.h"
#include <ogr_api.h>


struct QgsOgrConn
{
QString path;
OGRDataSourceH ds;
bool valid;
};

inline QString qgsConnectionPool_ConnectionToName( QgsOgrConn* c )
{
return c->path;
}

inline void qgsConnectionPool_ConnectionCreate( QString connInfo, QgsOgrConn*& c )
{
c = new QgsOgrConn;
c->ds = OGROpen( connInfo.toUtf8().constData(), false, NULL );
c->path = connInfo;
c->valid = true;
}

inline void qgsConnectionPool_ConnectionDestroy( QgsOgrConn* c )
{
OGR_DS_Destroy( c->ds );
delete c;
}

inline void qgsConnectionPool_InvalidateConnection( QgsOgrConn* c )
{
c->valid = false;
}

inline bool qgsConnectionPool_ConnectionIsValid( QgsOgrConn* c )
{
return c->valid;
}

class QgsOgrConnPoolGroup : public QObject, public QgsConnectionPoolGroup<QgsOgrConn*>
{
Q_OBJECT

public:
QgsOgrConnPoolGroup( QString name ) : QgsConnectionPoolGroup<QgsOgrConn*>( name ) { initTimer( this ); }

protected slots:
void handleConnectionExpired() { onConnectionExpired(); }
void startExpirationTimer() { expirationTimer->start(); }
void stopExpirationTimer() { expirationTimer->stop(); }

protected:
Q_DISABLE_COPY( QgsOgrConnPoolGroup )

};

/** Ogr connection pool - singleton */
class QgsOgrConnPool : public QgsConnectionPool<QgsOgrConn*, QgsOgrConnPoolGroup>
{
public:
static QgsOgrConnPool* instance();

void invalidateHandles( const QString& connInfo );

protected:
Q_DISABLE_COPY( QgsOgrConnPool )

private:
QgsOgrConnPool();
~QgsOgrConnPool();
};


#endif // QGSOGRCONNPOOL_H
@@ -35,27 +35,26 @@

QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool ownSource, const QgsFeatureRequest& request )
: QgsAbstractFeatureIteratorFromSource<QgsOgrFeatureSource>( source, ownSource, request )
, ogrDataSource( 0 )
, ogrLayer( 0 )
, mSubsetStringSet( false )
, mGeometrySimplifier( NULL )
{
mFeatureFetched = false;

ogrDataSource = OGROpen( TO8F( mSource->mFilePath ), false, NULL );
mConn = QgsOgrConnPool::instance()->acquireConnection( mSource->mFilePath );

if ( mSource->mLayerName.isNull() )
{
ogrLayer = OGR_DS_GetLayer( ogrDataSource, mSource->mLayerIndex );
ogrLayer = OGR_DS_GetLayer( mConn->ds, mSource->mLayerIndex );
}
else
{
ogrLayer = OGR_DS_GetLayerByName( ogrDataSource, TO8( mSource->mLayerName ) );
ogrLayer = OGR_DS_GetLayerByName( mConn->ds, TO8( mSource->mLayerName ) );
}

if ( !mSource->mSubsetString.isEmpty() )
{
ogrLayer = QgsOgrUtils::setSubsetString( ogrLayer, ogrDataSource, mSource->mEncoding, mSource->mSubsetString );
ogrLayer = QgsOgrUtils::setSubsetString( ogrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString );
mSubsetStringSet = true;
}

@@ -215,13 +214,13 @@ bool QgsOgrFeatureIterator::close()

if ( mSubsetStringSet )
{
OGR_DS_ReleaseResultSet( ogrDataSource, ogrLayer );
OGR_DS_ReleaseResultSet( mConn->ds, ogrLayer );
}

OGR_DS_Destroy( ogrDataSource );
QgsOgrConnPool::instance()->releaseConnection( mConn );
mConn = 0;

mClosed = true;
ogrDataSource = 0;
return true;
}

@@ -16,6 +16,7 @@
#define QGSOGRFEATUREITERATOR_H

#include "qgsfeatureiterator.h"
#include "qgsogrconnpool.h"

#include <ogr_api.h>

@@ -71,7 +72,7 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsOgr

bool mFeatureFetched;

OGRDataSourceH ogrDataSource;
QgsOgrConn* mConn;
OGRLayerH ogrLayer;

bool mSubsetStringSet;
@@ -741,6 +741,7 @@ OGRwkbGeometryType QgsOgrProvider::getOgrGeomType( OGRLayerH ogrLayer )

void QgsOgrProvider::loadFields()
{
QgsOgrConnPool::instance()->invalidateHandles( filePath() );
//the attribute fields need to be read again when the encoding changes
mAttributeFields.clear();

@@ -2522,6 +2523,8 @@ void QgsOgrProvider::recalculateFeatureCount()
{
OGR_L_SetSpatialFilter( ogrLayer, filter );
}

QgsOgrConnPool::instance()->invalidateHandles( filePath() );
}

OGRwkbGeometryType QgsOgrProvider::ogrWkbSingleFlatten( OGRwkbGeometryType type )
@@ -36,6 +36,15 @@ inline void qgsConnectionPool_ConnectionDestroy( QgsPostgresConn* c )
c->unref(); // will delete itself
}

inline void qgsConnectionPool_InvalidateConnection( QgsPostgresConn* c )
{
}

inline bool qgsConnectionPool_ConnectionIsValid( QgsPostgresConn* c )
{
return true;
}


class QgsPostgresConnPoolGroup : public QObject, public QgsConnectionPoolGroup<QgsPostgresConn*>
{
@@ -35,6 +35,15 @@ inline void qgsConnectionPool_ConnectionDestroy( QgsSqliteHandle* c )
QgsSqliteHandle::closeDb( c ); // will delete itself
}

inline void qgsConnectionPool_InvalidateConnection( QgsSqliteHandle* c )
{
}

inline bool qgsConnectionPool_ConnectionIsValid( QgsSqliteHandle* c )
{
return true;
}


class QgsSpatiaLiteConnPoolGroup : public QObject, public QgsConnectionPoolGroup<QgsSqliteHandle*>
{

13 comments on commit 72c9830

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jun 29, 2015

@manisandro I've been trying to track down a regression on OSX which is causing attribute changes made via QgsOgrProvider::changeAttributeValues to get lost. It's only visible on OSX, and I've bisected it back to this commit. You can see the failure in the results of the zonalstatistics test, eg http://dash.orfeo-toolbox.org/testDetails.php?test=30531850&build=184378 (that's the first failure following the ogr pool commit).

I'll keep digging - but do you have any ideas what could be causing this?

@manisandro

This comment has been minimized.

Copy link
Member

@manisandro manisandro replied Jun 29, 2015

@nyalldawson Oh, damn. Just to make sure, is this master? In particular, is 100de72 included?

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jun 29, 2015

@manisandro it's still present in master, even with that fix

@gioman

This comment has been minimized.

Copy link
Contributor

@gioman gioman replied Jun 29, 2015

Does this cause to have the edited attributes "lost" on save (they load if you reload the layer or the project)? If yes I have seen all day on QGIS 2.10 on Windows from OSGeo4W.

@manisandro

This comment has been minimized.

Copy link
Member

@manisandro manisandro replied Jun 29, 2015

@nyalldawson Could you try adding

QgsOgrConnPool::instance()->invalidateConnections( filePath() );

at the end of QgsOgrProvider::changeAttributeValues, before the return true? If that solves the issue, it appears that the internal state of the ogr connection handle also depends on the actual attribute values...

@gioman Yes that sounds like the same issue...

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jun 29, 2015

@manisandro yep, adding that line fixes it

@manisandro

This comment has been minimized.

Copy link
Member

@manisandro manisandro replied Jun 29, 2015

@nyalldawson Ok... Can you commit that change?

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jun 29, 2015

@manisandro will do- what about the other methods, like QgsOgrProvider::changeGeometryValues? Will they also need the fix?

@manisandro

This comment has been minimized.

Copy link
Member

@manisandro manisandro replied Jun 29, 2015

@nyalldawson At this point I'd say yes, anything which changes the dataset. Situation should be:

  • add/deleteFeatures are taken care of via recalculateFeatureCount
  • add/deleteAttributes are taken care of via loadFields
  • changeAttributeValues and changeGeometryValues need the fix

I think that should cover it?

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jun 29, 2015

@manisandro

This comment has been minimized.

Copy link
Member

@manisandro manisandro replied Jun 29, 2015

@nyalldawson Yes, thanks a lot.

@nyalldawson

This comment has been minimized.

Copy link
Collaborator

@nyalldawson nyalldawson replied Jul 6, 2015

@manisandro Do you think http://hub.qgis.org/issues/13082 could be related to this work?

@manisandro

This comment has been minimized.

Copy link
Member

@manisandro manisandro replied Jul 6, 2015

@nyalldawson Fear so, yes. Pull request with suggested fix: #2197.

Please sign in to comment.