Discarding edits freezes qgis for a long time

[qgib]

Author Name: Giovanni Manghi (@gioman)
Original Redmine Issue: 9509
Affected QGIS version: master
Redmine category:vectors

---

This is a follow up of #17922

Open a fair large vector like http://idena.navarra.es/descargas/CARTO1_Lin_6CNivelD.zip

with the field calculator update a column with whatever you want, then toggle editing and discard changes: QGIS freezes for a looooong time.

On the other hand with the provided dataset if you choose to save edits it takes just a few seconds to save.

---

Related issue(s): #17922 (relates), #18109 (duplicates)
Redmine related issue(s): 9315, 9519

---

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Fixed in changeset "52616b6549bf43eee3cccc79f73aa890da5f3fab".

---

• status_id was changed from Open to Closed

[qgib]

Author Name: Salvatore Larosa (@slarosa)

---

For me it still not fixed. I am getting always a QGIS freezing on rollback action.

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Salvatore Larosa wrote:

For me it still not fixed. I am getting always a QGIS freezing on rollback action.

It's faster. But the changes need to be rolled back one by one and therefore the time it takes to rollback is somewhat the same as to do the changes in the first place.

[qgib]

Author Name: Matthias Kuhn (@m-kuhn)

---

It's not quite the same, as when doing changes the first time via the field calculator, one iterator is opened and looped while in a rollback, there is one iterator per feature.

Quote from #18109

As far as I can tell, there are feature requests made for every single feature which was updated before (so all in the case mentioned above).
I could imagine a possible approach would be to batch undo the affected rows and perform a single request with setFilterFids() to still only request the required rows, but with less roundtrips.

---

• status_id was changed from Closed to Reopened

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Matthias Kuhn wrote:

It's not quite the same, as when doing changes the first time via the field calculator, one iterator is opened and looped while in a rollback, there is one iterator per feature.

Not anymore. That was the case when the previous values were not stored in the first undo command - therefore the original value had to be retrieved from the provider using a iterator. Now that only happens if an attribute change happens when the previous value isn't already available (which is not the case in the field calculator).

Still all subscribers to the attributeValueChanged signal must be notified that the value was changed back (BTW the dualview form view doesn't update in that case).

[qgib]

Author Name: Matthias Kuhn (@m-kuhn)

---

Jürgen Fischer wrote:

Not anymore. That was the case when the previous values were not stored in the first undo command - therefore the original value had to be retrieved from the provider using a iterator. Now that only happens if an attribute change happens when the previous value isn't already available (which is not the case in the field calculator).

So the problem is the field calculator not saving the previous values?

Still all subscribers to the attributeValueChanged signal must be notified that the value was changed back (BTW the dualview form view doesn't update in that case).

I can take care of that. I think introducing an afterRollback signal should do the trick.

[qgib]

Author Name: Matthias Kuhn (@m-kuhn)

---

Ah, the form view and not the table view.

/me should read more carefully.

I will take care of this for 2.4 with the new editor components.

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Matthias Kuhn wrote:

Jürgen Fischer wrote:

Not anymore. That was the case when the previous values were not stored in the first undo command - therefore the original value had to be retrieved from the provider using a iterator. Now that only happens if an attribute change happens when the previous value isn't already available (which is not the case in the field calculator).

So the problem is the field calculator not saving the previous values?

IMHO there is no problem anymore. The rollback is faster now, as it doesn't need to reload the values from the provider, but it still needs time because it needs to signal each undo - and that's by design.

We only have the attributeValueChange signal and subscribers can currently assume that they will be notified for each change. We could introduce a bulkAttributeValueChange signal (and undo command), that tells subscribers that all the values of an attribute changed (maybe include fid ranges or fid sets) and define that attributeValueChange doesn't cover bulk changes. But that would be a feature.

[qgib]

Author Name: Matthias Kuhn (@m-kuhn)

---

I am not at home to test now but when I tested yesterday it seemed to me that the field calculator is still faster than the rollback - and they should emit the same amount of signals. Maybe the decrease in performance is related to debug output the rollback produces.

bulkAttributeValueChange signal would be fine, but on the other hand I experienced that caching values retrieved from attributeChanged signals and postponing heavy operations to editCommandEnded() works very well. So the signals themselves seem to be a minor problem as long as their implementations are not performance-killers. The only remaining problem with this approach I can think of is that if several consumers cache the values it's a waste of memory.

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Matthias Kuhn wrote:

I am not at home to test now but when I tested yesterday it seemed to me that the field calculator is still faster than the rollback - and they should emit the same amount of signals. Maybe the decrease in performance is related to debug output the rollback produces.

Before or after 52616b6? I didn't do benchmarking either, but the test expression to update and undo I used was trivial.

[qgib]

Author Name: Matthias Kuhn (@m-kuhn)

---

Tested again with current master.

I noticed, that when updating a column, the rollback is fast. But when I add a newly calculated column that's what takes so long to rollback.

In the latter case the debug output I mentioned is:

src/providers/postgres/qgspostgresconn.cpp: 825: (openCursor) Starting read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 839: (closeCursor) Committing read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 825: (openCursor) Starting read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 839: (closeCursor) Committing read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 825: (openCursor) Starting read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 839: (closeCursor) Committing read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 825: (openCursor) Starting read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 839: (closeCursor) Committing read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 825: (openCursor) Starting read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 839: (closeCursor) Committing read-only transaction
src/providers/postgres/qgspostgresconn.cpp: 825: (openCursor) Starting read-only transaction

[qgib]

Author Name: Jürgen Fischer (@jef-n)

---

Fixed in changeset "5bee17218db3576a6a172eec5ef6d555fc023b4d".

---

• status_id was changed from Reopened to Closed