[OGR provider] Remove calls to ResetReading() in changeGeometryValues() and changeGeometryValues() in situations where this is safe to do #39244
Conversation
…() and changeGeometryValues() in situations where this is safe to do Such calls will cause issues in the context of transaction groups where feature iterators will share the same connection handle.
| @@ -2489,6 +2509,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. | |||
There was a problem hiding this comment.
Maybe move this duplicate code out to a function?
There was a problem hiding this comment.
yeah, waiting for @elpaso confirmation that it actually helps for the issue we discussed by email
There was a problem hiding this comment.
Thanks, I'll try that tomorrow and it will certainly help but I'm 100% sure that's not enough: QgsOgrFeatureIterator constructor is calling reset in several places through OGRGeoPackageTableLayer::SetSpatialFilter, OGRGeoPackageTableLayer::SetSpatialFilterRect and OGRGeoPackageTableLayer::SetAttributeFilter they all call ResetReading and these methods are invoked in the QgsOgrFeatureIterator constructor.
There was a problem hiding this comment.
... and the initial rewind call of course.
There was a problem hiding this comment.
Should we disable geopackage transaction support until this is sorted out?
There was a problem hiding this comment.
@rouault I can confirm that this patch does not fix the issue. The QgsOgrFeatureIterator ctor needs patches too, I'll try to work on it.
There was a problem hiding this comment.
@elpaso For my information, could you explain or reexplain me what is the issue with QgsOgrFeatureIterator issuing ResetReading() at the beginning of the iteration ? Of course, it is problematic if ResetReading() is called after iteration has started, but not at its beginning. I probably miss something.
In the scenario you pointed in #39178 (comment), if changeAttributeValue() no longer issues a ResetReading(), we should be safe, shouldn't we ?
self.assertTrue(vl.startEditing())
for f in vl.getFeatures():
self.assertTrue(vl.changeAttributeValue(1, 1, 'new value'))
There was a problem hiding this comment.
@rouault no, the fact is that the last line of code here creates a new (nested) iterator
QgsVectorLayerUndoPassthroughCommandChangeAttribute::QgsVectorLayerUndoPassthroughCommandChangeAttribute( QgsVectorLayerEditBuffer *buffer, QgsFeatureId fid, int field, const QVariant &newValue )
: QgsVectorLayerUndoPassthroughCommand( buffer, QObject::tr( "change attribute value" ) )
, mFid( fid )
, mField( field )
, mNewValue( newValue )
, mOldValue( mBuffer->L->getFeature( mFid ).attribute( field ) )A commit worth more than a thousand words, this fixes the issue in the test: 505ef99
There was a problem hiding this comment.
no, the fact is that the last line of code here creates a new (nested) iterator
That was the part I was missing, thanks.
|
Superceeded by #39257 |
Such calls will cause issues in the context of transaction groups where
feature iterators will share the same connection handle.
CC @elpaso