Skip to content

Commit

Permalink
Merge pull request #39257 from elpaso/rouault-ogrprovider_remove_usel…
Browse files Browse the repository at this point in the history 
…ess_resetreading

ogrprovider remove useless resetreading
  • Loading branch information
@elpaso
elpaso committed Oct 9, 2020
2 parents 7216045 + fd495d0 commit 5ddf9aa
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 30 deletions.
75 changes: 49 additions & 26 deletions src/core/providers/ogr/qgsogrfeatureiterator.cpp
View file
Expand Up @@ -41,12 +41,22 @@
///@cond PRIVATE


QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request )
QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request, QgsTransaction *transaction )
: QgsAbstractFeatureIteratorFromSource<QgsOgrFeatureSource>( source, ownSource, request )
, mSharedDS( source->mSharedDS )
, mFirstFieldIsFid( source->mFirstFieldIsFid )
, mFieldsWithoutFid( source->mFieldsWithoutFid )
{

/* When inside a transaction for GPKG/SQLite and fetching fid(s) we might be nested inside an outer fetching loop,
* (see GH #39178) so we need to skip all calls that might reset the reading (rewind) to avoid an endless loop in the
* outer fetching iterator that uses the same connection.
*/
mAllowResetReading = ! transaction ||
( source->mDriverName != QLatin1String( "GPKG" ) && source->mDriverName != QLatin1String( "SQLite" ) ) ||
( mRequest.filterType() != QgsFeatureRequest::FilterType::FilterFid
&& mRequest.filterType() != QgsFeatureRequest::FilterType::FilterFids );

for ( const auto &id : mRequest.filterFids() )
{
mFilterFids.insert( id );
Expand Down Expand Up @@ -85,7 +95,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
return;
}

if ( !mSource->mSubsetString.isEmpty() )
if ( mAllowResetReading && !mSource->mSubsetString.isEmpty() )
{
mOgrLayerOri = mOgrLayer;
mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString );
Expand Down Expand Up @@ -165,17 +175,20 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
}

// spatial query to select features
if ( !mFilterRect.isNull() )
if ( mAllowResetReading )
{
OGR_L_SetSpatialFilterRect( mOgrLayer, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilterRect( mOgrLayerOri, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
}
else
{
OGR_L_SetSpatialFilter( mOgrLayer, nullptr );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilter( mOgrLayerOri, nullptr );
if ( !mFilterRect.isNull() )
{
OGR_L_SetSpatialFilterRect( mOgrLayer, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilterRect( mOgrLayerOri, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
}
else
{
OGR_L_SetSpatialFilter( mOgrLayer, nullptr );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilter( mOgrLayerOri, nullptr );
}
}

if ( request.filterType() == QgsFeatureRequest::FilterExpression
Expand All @@ -201,33 +214,38 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
QStringLiteral( ") AND (" ) + whereClause +
QStringLiteral( ")" );
}
if ( OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( whereClause ).constData() ) == OGRERR_NONE )
{
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
}
else if ( !mSource->mSubsetString.isEmpty() )

if ( mAllowResetReading )
{
// OGR rejected the compiled expression. Make sure we restore the original subset string if set (and do the filtering on QGIS' side)
OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( mSource->mSubsetString ).constData() );
if ( OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( whereClause ).constData() ) == OGRERR_NONE )
{
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
}
else if ( !mSource->mSubsetString.isEmpty() )
{
// OGR rejected the compiled expression. Make sure we restore the original subset string if set (and do the filtering on QGIS' side)
OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( mSource->mSubsetString ).constData() );
}
}

}
else if ( mSource->mSubsetString.isEmpty() )
else if ( mSource->mSubsetString.isEmpty() && mAllowResetReading )
{
OGR_L_SetAttributeFilter( mOgrLayer, nullptr );
}

delete compiler;
}
else if ( mSource->mSubsetString.isEmpty() )
else if ( mSource->mSubsetString.isEmpty() && mAllowResetReading )
{
OGR_L_SetAttributeFilter( mOgrLayer, nullptr );
}


//start with first feature
rewind();

}

QgsOgrFeatureIterator::~QgsOgrFeatureIterator()
Expand All @@ -249,7 +267,7 @@ bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature &fea
gdal::ogr_feature_unique_ptr fet;

#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,2,0)
if ( !QgsOgrProviderUtils::canDriverShareSameDatasetAmongLayers( mSource->mDriverName ) )
if ( mAllowResetReading && !QgsOgrProviderUtils::canDriverShareSameDatasetAmongLayers( mSource->mDriverName ) )
{
OGRLayerH nextFeatureBelongingLayer;
bool found = false;
Expand Down Expand Up @@ -381,6 +399,10 @@ bool QgsOgrFeatureIterator::fetchFeature( QgsFeature &feature )

void QgsOgrFeatureIterator::resetReading()
{
if ( ! mAllowResetReading )
{
return;
}
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,2,0)
if ( !QgsOgrProviderUtils::canDriverShareSameDatasetAmongLayers( mSource->mDriverName ) )
{
Expand Down Expand Up @@ -561,6 +583,7 @@ QgsOgrFeatureSource::QgsOgrFeatureSource( const QgsOgrProvider *p )
{
if ( p->mTransaction )
{
mTransaction = p->mTransaction;
mSharedDS = p->mTransaction->sharedDS();
}
for ( int i = ( p->mFirstFieldIsFid ) ? 1 : 0; i < mFields.size(); i++ )
Expand All @@ -575,7 +598,7 @@ QgsOgrFeatureSource::~QgsOgrFeatureSource()

QgsFeatureIterator QgsOgrFeatureSource::getFeatures( const QgsFeatureRequest &request )
{
return QgsFeatureIterator( new QgsOgrFeatureIterator( this, false, request ) );
return QgsFeatureIterator( new QgsOgrFeatureIterator( this, false, request, mTransaction ) );
}

///@endcond
8 changes: 7 additions & 1 deletion src/core/providers/ogr/qgsogrfeatureiterator.h
View file
Expand Up @@ -56,6 +56,7 @@ class QgsOgrFeatureSource final: public QgsAbstractFeatureSource
QgsCoordinateReferenceSystem mCrs;
QgsWkbTypes::Type mWkbType = QgsWkbTypes::Unknown;
QgsOgrDatasetSharedPtr mSharedDS = nullptr;
QgsTransaction *mTransaction = nullptr;

friend class QgsOgrFeatureIterator;
friend class QgsOgrExpressionCompiler;
Expand All @@ -64,7 +65,7 @@ class QgsOgrFeatureSource final: public QgsAbstractFeatureSource
class QgsOgrFeatureIterator final: public QgsAbstractFeatureIteratorFromSource<QgsOgrFeatureSource>
{
public:
QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request );
QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool ownSource, const QgsFeatureRequest &request, QgsTransaction *transaction );

~QgsOgrFeatureIterator() override;

Expand Down Expand Up @@ -102,6 +103,11 @@ class QgsOgrFeatureIterator final: public QgsAbstractFeatureIteratorFromSource<Q
bool mFirstFieldIsFid = false;
QgsFields mFieldsWithoutFid;

/* This flag tells the iterator when to skip all calls that might reset the reading (rewind),
* to be used when the request is for a single fid or for a list of fids and we are inside
* a transaction for SQLITE-based layers */
bool mAllowResetReading = true;

bool fetchFeatureWithId( QgsFeatureId id, QgsFeature &feature ) const;

void resetReading();
Expand Down
46 changes: 43 additions & 3 deletions src/core/providers/ogr/qgsogrprovider.cpp
View file
Expand Up @@ -1340,7 +1340,7 @@ void QgsOgrProviderUtils::setRelevantFields( OGRLayerH ogrLayer, int fieldCount,

QgsFeatureIterator QgsOgrProvider::getFeatures( const QgsFeatureRequest &request ) const
{
return QgsFeatureIterator( new QgsOgrFeatureIterator( static_cast<QgsOgrFeatureSource *>( featureSource() ), true, request ) );
return QgsFeatureIterator( new QgsOgrFeatureIterator( static_cast<QgsOgrFeatureSource *>( featureSource() ), true, request, mTransaction ) );
}


Expand Down Expand Up @@ -2301,6 +2301,22 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_

const bool inTransaction = startTransaction();

// Some drivers may need to call ResetReading() after GetFeature(), such
// as GPKG in GDAL < 2.3.0 to avoid letting the database in a locked state.
// But this is undesirable in general, so don't do this when we know that
// we don't need to.
bool mayNeedResetReadingAfterGetFeature = true;
if ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) )
{
mayNeedResetReadingAfterGetFeature = false;
}
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,3,0)
else if ( mGDALDriverName == QLatin1String( "GPKG" ) )
{
mayNeedResetReadingAfterGetFeature = false;
}
#endif

for ( QgsChangedAttributesMap::const_iterator it = attr_map.begin(); it != attr_map.end(); ++it )
{
QgsFeatureId fid = it.key();
Expand All @@ -2315,7 +2331,11 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
pushError( tr( "Feature %1 for attribute update not found." ).arg( fid ) );
continue;
}
mOgrLayer->ResetReading(); // needed for SQLite-based to clear iterator

if ( mayNeedResetReadingAfterGetFeature )
{
mOgrLayer->ResetReading();
}

QgsLocaleNumC l;

Expand Down Expand Up @@ -2484,6 +2504,22 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )

const bool inTransaction = startTransaction();

// Some drivers may need to call ResetReading() after GetFeature(), such
// as GPKG in GDAL < 2.3.0 to avoid letting the database in a locked state.
// But this is undesirable in general, so don't do this when we know that
// we don't need to.
bool mayNeedResetReadingAfterGetFeature = true;
if ( mGDALDriverName == QLatin1String( "ESRI Shapefile" ) )
{
mayNeedResetReadingAfterGetFeature = false;
}
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,3,0)
else if ( mGDALDriverName == QLatin1String( "GPKG" ) )
{
mayNeedResetReadingAfterGetFeature = false;
}
#endif

for ( QgsGeometryMap::const_iterator it = geometry_map.constBegin(); it != geometry_map.constEnd(); ++it )
{
gdal::ogr_feature_unique_ptr theOGRFeature( mOgrLayer->GetFeature( FID_TO_NUMBER( it.key() ) ) );
Expand All @@ -2492,7 +2528,11 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
pushError( tr( "OGR error changing geometry: feature %1 not found" ).arg( it.key() ) );
continue;
}
mOgrLayer->ResetReading(); // needed for SQLite-based to clear iterator

if ( mayNeedResetReadingAfterGetFeature )
{
mOgrLayer->ResetReading(); // needed for SQLite-based to clear iterator, which could let the database in a locked state otherwise
}

OGRGeometryH newGeometry = nullptr;
QByteArray wkb = it->asWkb();
Expand Down
32 changes: 32 additions & 0 deletions tests/src/python/test_provider_ogr_gpkg.py
View file
Expand Up @@ -1768,6 +1768,38 @@ def testTransactionGroup(self):
self.assertFalse(vl1_1.isEditable())
self.assertFalse(vl1_2.isEditable())

@unittest.skipIf(int(gdal.VersionInfo('VERSION_NUM')) < GDAL_COMPUTE_VERSION(2, 3, 0), "GDAL 2.3 required")
def testTransactionGroupIterator(self):
"""Test issue GH #39178: the bug is that this test hangs
forever in an endless loop"""

project = QgsProject()
project.setAutoTransaction(True)
tmpfile = os.path.join(
self.basetestpath, 'tempGeoPackageTransactionGroupIterator.gpkg')
ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile)
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint)
lyr.CreateField(ogr.FieldDefn('str_field', ogr.OFTString))

f = ogr.Feature(lyr.GetLayerDefn())
f.SetGeometry(ogr.CreateGeometryFromWkt('POINT (1 1)'))
f.SetField('str_field', 'one')
lyr.CreateFeature(f)

del lyr
del ds

vl = QgsVectorLayer(tmpfile + '|layername=test', 'test', 'ogr')
project.addMapLayers([vl])

self.assertTrue(vl.startEditing())

for f in vl.getFeatures():
self.assertTrue(vl.changeAttributeValue(1, 1, 'new value'))

# Test that QGIS sees the new changes
self.assertEqual(next(vl.getFeatures()).attribute(1), 'new value')


if __name__ == '__main__':
unittest.main()

0 comments on commit 5ddf9aa

Please sign in to comment.