[OGR provider] Check return value of commitTransaction() in addFeatures() and deleteFeatures(); call rollback when an error was detected before

[rouault]

Calling rollback() didn't seem to be strictly needed, but makes more sense than calling commit() when we know that something wrong happened.

[nyalldawson]

Wouldn't this mean that an error encountered while adding one feature from a batch would mean that NO features are written?

[rouault]

Wouldn't this mean that an error encountered while adding one feature from a batch would mean that NO features are written?

Yes, and that was already the case since this is done in a transaction (for drivers that support transactions). My understanding is that the PostgreSQL provider does the same.

[nyalldawson]

Yes, and that was already the case since this is done in a transaction (for drivers that support transactions).

Are you referring to a "qgis" transaction or a transaction on the underlying backend?

My understanding is that the PostgreSQL provider does the same.

@elpaso I recall that you've investigated this same question recently -- what was your findings regarding this? Do we add/update/delete what we can and then return false, or abort adding/updating/deleting all affected features?

[rouault]

Are you referring to a "qgis" transaction or a transaction on the underlying backend?

A OGR underlying backend transaction

Digging into history points to #4257 where the motivation was speed

[m-kuhn]

For reference, we plan to come up with a QEP regarding transaction/data consistency soonish.

[nyalldawson]

@m-kuhn @elpaso should we merge?

[m-kuhn]

+1
How risky do you see this, i.e. would it be worth to wait for after release (in 3 days)?

[elpaso]

Yes, and that was already the case since this is done in a transaction (for drivers that support transactions).

Are you referring to a "qgis" transaction or a transaction on the underlying backend?

My understanding is that the PostgreSQL provider does the same.

@elpaso I recall that you've investigated this same question recently -- what was your findings regarding this? Do we add/update/delete what we can and then return false, or abort adding/updating/deleting all affected features?

@nyalldawson I think you mean #39178

The issue there was that while you are inside a QGIS transaction (group) and start a getFeatures loop, if you change attributes or geometry a getFeature is called, a new iterator is created and OGR provider did call a GDAL GetFeature function which reset the internal GDAL feature counter to 1, so you had and endless loop. Also, it wasn't possible to use a different connection than the one where the transaction was initiated: pretty bad bug, but I don't see it related to this PR.

So, I'm +1 to merge.