Data loss when discarding changes in a PostGIS layer

[qgib]

Author Name: Giovanni Manghi (@gioman)
Original Redmine Issue: 17185
Affected QGIS version: 2.18.13
Redmine category:data_provider/postgis
Assignee: Alessandro Pasotti

---

Import a shape in PostGIS with DB Manager or the QGIS Processing tool

Table is created with PK (not NULL) but not nextval()

Add the table and split one feature

Try save: error because the new feature does not get automatically a new value for the PK

Discard changes > one of the features resulting from the split is gone

---

Related issue(s): #15099 (duplicates), #25085 (relates)
Redmine related issue(s): 5475, 17186

---

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

• assigned_to_id removed Jürgen Fischer

[qgib]

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

---

Discarding features always implies data loss, doesn't it!?

[qgib]

Author Name: Luigi Pirelli (@luipir)

---

Split a feature is composed of two phases in this order:

1. change geometry of the original geom
2. add new features => error

the rollback remove added feature but not reset old geom because it's change was successfull in the edit buffer => more than a data loss is a not reversible data change.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Luigi Pirelli wrote:

more than a data loss is a not reversible data change.

user-side is data loss.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Jürgen Fischer wrote:

Discarding features always implies data loss, doesn't it!?

is not a new added geometry that is discarded, but a chunk of an existing geometry (split with the split features tool) that is lost.

[qgib]

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

---

Giovanni Manghi wrote:

Jürgen Fischer wrote:

Discarding features always implies data loss, doesn't it!?

is not a new added geometry that is discarded, but a chunk of an existing geometry (split with the split features tool) that is lost.

It's lost because the user decided to discard it. The user should fix the feature by assigning a valid key value and proceed with the commit.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

It's lost because the user decided to discarded it. The user should fix the feature by assigning a valid key value and proceed with the commit.

you have faith in users that they will click in the message bar and read the log ;) they usually don't, and just run away from the error as fast as they can (by undoing changes).

A fix for #25085 would make this issue much less important.

[qgib]

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

---

• relates was configured as DB Manager and QGIS/Processing import in PostGIS tools are not adding "nextval()" to primary key field #25085

[qgib]

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

---

• duplicates was configured as Split feature on spatialite layers with primary key error #16493

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

If this was already reported as #16493 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

---

• regression was changed from 0 to 1

[qgib]

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

---

Giovanni Manghi wrote:

If this was already reported as #16493 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

I don't consider it a bug. It's the expected behaviour: You have to specify a key value if you don't use a sequence. And there might by arbitrary other constraints that you have to adjust your attributes (or geometries) to to be able to commit.

[qgib]

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

---

• duplicates was configured as Problem to insert splitted geometries in postgis #15099

[qgib]

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

---

• duplicates removed Split feature on spatialite layers with primary key error #16493

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Jürgen Fischer wrote:

Giovanni Manghi wrote:

If this was already reported as #16493 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

I don't consider it a bug. It's the expected behaviour: You have to specify a key value if you don't use a sequence. And there might by arbitrary other constraints that you have to adjust your attributes (or geometries) to to be able to commit.

I think we agree we don't agree :)

my point of view is the following:

I do a whatever editing operation, then before any save I discard it. Is not correct that after discarding any unsaved change I have lost data.

[qgib]

Author Name: Luigi Pirelli (@luipir)

---

Jürgen Fischer wrote:

Giovanni Manghi wrote:

If this was already reported as #16493 and closed as fixed this means that is a regression (I "only" tested down to 2.8).

I don't consider it a bug. It's the expected behaviour: You have to specify a key value if you don't use a sequence. And there might by arbitrary other constraints that you have to adjust your attributes (or geometries) to to be able to commit.

may we have another API option to setup if to use default nextval or leave it to the user ?

[qgib]

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

---

Luigi Pirelli wrote:

may we have another API option to setup if to use default nextval or leave it to the user ?

The point is that there is no nextval in this case (see #25085) - otherwise it would be used (merge already doesn't copy attributes with default values; see #15099)

[qgib]

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

---

Giovanni Manghi wrote:

my point of view is the following:

I do a whatever editing operation, then before any save I discard it. Is not correct that after discarding any unsaved change I have lost data.

If you would just "discard", it would work like that. But you "commit" - which only partly succeeds and then "discard" the parts that didn't succeed. You wouldn't loose data if you would just update the failed parts and commit them. I agree that partly commits are not ideal and that deducing what actually needs to be done to fix the remaining parts might not be easy. But we still handle adding and deleting, changing of attributes and geometries as separate operations in the provider interface (and hence they run in separate transactions and cannot rolled back cleanly; see "QgsVectorLayer::commitChanges":http://qgis.org/api/classQgsVectorLayer.html#afab34baf331320a8c3212993f5fccfa1).

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Jürgen Fischer wrote:

Giovanni Manghi wrote:

my point of view is the following:

I do a whatever editing operation, then before any save I discard it. Is not correct that after discarding any unsaved change I have lost data.

If you would just "discard", it would work like that. But you "commit" - which only partly succeeds and then "discard" the parts that didn't succeed. You wouldn't loose data if you would just update the failed parts and commit them. I agree that partly commits are not ideal and that deducing what actually needs to be done to fix the remaining parts might not be easy. But we still handle adding and deleting, changing of attributes and geometries as separate operations in the provider interface (and hence they run in separate transactions and cannot rolled back cleanly; see "QgsVectorLayer::commitChanges":http://qgis.org/api/classQgsVectorLayer.html#afab34baf331320a8c3212993f5fccfa1).

ok, thanks I understand better the technicality of what is going on here. But I think that we all agree that from a user point of view this is an unexpected thing. Also is not what we tell people about how editing works (if I'm not wrong in docs we tell people that changes are saved only when hitting the "save" button, regardless of the tool they use).

[qgib]

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

---

Giovanni Manghi wrote:

(if I'm not wrong in docs we tell people that changes are saved only when hitting the "save" button, regardless of the tool they use).

as said that is what's happening. just that the save partly fails and the failing parts are kept and the successful parts can't be undone.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Jürgen Fischer wrote:

Giovanni Manghi wrote:

(if I'm not wrong in docs we tell people that changes are saved only when hitting the "save" button, regardless of the tool they use).

as said that is what's happening. just that the save partly fails and the failing parts are kept and the successful parts can't be undone.

so this would be won't fix because impossible to fix? If yes I would add a ticket to remember me to document this behavior.

[qgib]

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

---

Giovanni Manghi wrote:

so this would be won't fix because impossible to fix? If yes I would add a ticket to remember me to document this behavior.

Of course it's not impossible - but the current behavior is a result of the current provider design (see #25084-17 and #15099 (comment)), so it wouldn't be a small change.

[qgib]

Author Name: Giovanni Manghi (@gioman)

---

Of course it's not impossible - but the current behavior is a result of the current provider design (see #25084-17 and #15099 (comment)), so it wouldn't be a small change.

We should leave this open then. I will add a note to docs to document this. As said, fixing #25085 would makes this slightly less relevant.

[qgib]

Author Name: Alessandro Pasotti (@elpaso)

---

• assigned_to_id was configured as Alessandro Pasotti

[qgib]

Author Name: Alessandro Pasotti (@elpaso)

---

I canot reproduce the first step in master: importing the table from a shapefile creates the sequence (nextval correctly) also from DB manager

---

• status_id was changed from Open to Feedback

[qgib]

Author Name: Alessandro Pasotti (@elpaso)

---

Split features work correctly with a shapefile-imported PG table on both 2.x and master.

---

• status_id was changed from Feedback to Closed
• resolution was changed from to worksforme

[Brent-Edwards]

For what it's worth, I just got bit by this one running 3.10.2 and was fortunately able to recover lost data from an archived version of the table.

Here's my setup:

QGIS version
3.10.2-A Coruña
QGIS code revision
d4cd3cf
Compiled against Qt
5.11.2
Running against Qt
5.11.2
Compiled against GDAL/OGR
3.0.3
Running against GDAL/OGR
3.0.3
Compiled against GEOS
3.8.0-CAPI-1.13.1
Running against GEOS
3.8.0-CAPI-1.13.1
Compiled against SQLite
3.29.0
Running against SQLite
3.29.0
PostgreSQL Client Version
11.5
SpatiaLite Version
4.3.0
QWT Version
6.1.3
QScintilla2 Version
2.10.8
Compiled against PROJ
6.3.0
Running against PROJ
Rel. 6.3.0, January 1st, 2020
OS Version
Windows 10 (10.0)
Active python plugins
changeDataSource;
crayfish;
gmsh;
latlontools;
Mergin;
mmqgis;
profiletool;
QMetaTiles;
quick_map_services;
shapetools;
splitmultipart;
SVG2ColoR;
timemanager;
valuetool;
VectorMCDA;
VoGisProfilTool;
db_manager;
MetaSearch;
processing

[gioman]

For what it's worth, I just got bit by this one running 3.10.2 and was fortunately able to recover lost data from an archived version of the table.

@Brent-Edwards it would be bad if this issue has resurfaced. As this is ticket is very old and closed I suggest to open a new one, and describe in detail how to replicate it. Thanks.