QGIS » QGIS Application

Tim Sutton wrote:

Note that the issue is actually that the pkey is duplicated and thus the insert fails.

is this a regression that is present also in the code freezed for the 1.8 release? if yes please tag this as blocker.

It seems a problem when generating the gid param value for the insert statement. AFAIU the attributes are by default the same as the original feature. When committing the provider should set an appropriate value for the new gid.
Looking at the code, in previous versions (e.g. branch 1.7.4) it seems it was done using a mPrimaryKeyDefault which was parametrized to be the next value beyond the maximun (as here http://bit.ly/I9I3rb). In the master branch I only see that the return value from QgsPostgresProvider::paramValue is always the original value if the default value is null (http://bit.ly/I9ImlM).

If the insert of the new features fails, the user receives a descriptive error message and still has the possibility to adapt the primary key values, then commit again.

While I see it can be unconvenient, it is questionable if this is a bug at all. A table can have all sorts of constraints on it (not only on the pkey), and it is a difficult requirement for QGIS to detect that automatically. Furthermore, some users require that it should be possible to modify the pkey value in the attribute dialog. Therefore, the pkey is handled just like a normal attribute.

Even if you say "do not save" when you leave the edit session, some data are lost

I guess you already said 'do save', otherwise you would not have the pkey unique problem.

When I insert a new feature, the PK attribute is automatically filled with the following string:

nextval('testsplit_gid_seq'::regclass)

Then, if I try to split the geometry, the attribute window doesn't show up, because this operation is supposed to copy the same attributes values in the new feature. But, applying changes raises the following error:

Could not commit changes to layer testsplit

Errors: ERROR: 1 feature(s) not added.
  SUCCESS: 1 geometries were changed.

  Provider errors:
  PostGIS error while adding features: ERROR:  duplicate key value violates unique constraint "testsplit_pkey" 

The problem is that the original geometry has been already modified. If I toggle editing off, also refusing to commit changes, the original feature is modified, and the other part (splitted) is lost.

Being impossible for Qgis to judge how to manage PKs, it should be left to the backend to manage it, i.e. let PG apply the changes, automatically increasing the PK from the sequence, and return the value back (eventually).

Otherwise a new window should be opened, leaving the user to enter the values, perhaps suggesting the same string as during the insertion of a new feature.

Giovanni Allegri wrote:

Being impossible for Qgis to judge how to manage PKs, it should be left to the backend to manage it, i.e. let PG apply the changes, automatically increasing the PK from the sequence, and return the value back (eventually).

That would happen if the pk attribute would have the default value in it. Now it has a concrete value from the splitted feature in it. Currently there is no way to know which attributes make up the primary key outside of the provider. And so the split tool has no way to decide which attributes to change to their default value, which would in turn be replaced by the postgres provider.

The offending pk can however still be changed and committed after you have seen the error. No data should be lost unless you explicitly discard it.

Otherwise a new window should be opened, leaving the user to enter the values, perhaps suggesting the same string as during the insertion of a new feature.

There's also no way to tell which features have problems outside of the provider. But as said you can still change the offending feature (via the attribute table or the edit form).

Now it has a concrete value in it. Currently there is no way to know which attributes make up the primary key outside of the provider

Actually this is a provider concern. The provider has the ability to know what kind of PK it is, hasn'it? So I imagine it could remove the duplicated value from PK in case the PK is a sequence, and let the RDBMS manage the correct insertion.

Giovanni Allegri wrote:

Actually this is a provider concern. The provider has the ability to know what kind of PK it is, hasn'it? So I imagine it could remove the duplicated value from PK in case the PK is a sequence, and let the RDBMS manage the correct insertion.

It could, but that might also be considered a bug - if someone wants to set an explicit key (see #5608).

It could, but that might also be considered a bug - if someone wants to set an explicit key (see #5608).

Mmm, you're right, but whatever is being chosen I don't think that the current implementation is correct from an user experience perspective. If you don't open the attribute table and change the PK value manually, you obtain data corruption as soon as you try to save the editing as it is. If my table has a serial PK, I would naturally expect go on and save, without having to think about the fact that the provider doesn't take a decision and tries to do something wrong (i.e. try to save with an existing PK value).

If we want keep the responsability out of the provider, I think we should advice the user about that, i.e. suggest to set the PK value BEFORE committing to DB, not AFTER the geometry has been written, otherwise we loose data and consistency.

Giovanni Allegri wrote:

If we want keep the responsability out of the provider, I think we should advice the user about that, i.e. suggest to set the PK value BEFORE committing to DB, not AFTER the geometry has been written, otherwise we loose data and consistency.

Split is the opposite of merge. When merging the use gets to decide which values of the original features the new merged feature should have. I think there should be something similar for the split case, where the user can decide which attributes the new features should have. They should start with the values of the original feature, but allow changes including a reset to the default value (ie. nextval('testsplit_gid_seq'::regclass) in this case).

But I think that would actually be a new feature and not just a fix.

I agree with you Jurgen. Anyway, I think that a good design is to resume from a commit if an error happens. In this case the error makes the original feature being changed and losing the other part. I see it as a unique transaction, which should commit only if it success in its integrity.

Giovanni Allegri wrote:

I agree with you Jürgen. Anyway, I think that a good design is to resume from a commit if an error happens. In this case the error makes the original feature being changed and losing the other part. I see it as a unique transaction, which should commit only if it success in its integrity.

Adding of features, deleting of features and changing of attributes and of geometries are independent operations on provider level and are therefore handled in separate database transactions in the postgres provider. Therefore the split "transaction" actually consists of several database transactions and the successful transactions are removed from the current edit session and cannot be rolled back, while the unsuccessful transactions are rolled back and stay in the current edit session.

Ok, but I think that (if it would be possible/paid/sponsored/etc) it should be different. A split operation should be treated as a single transaction. It's like for a money exchange: you would never treat the withdrawal an the accounting as two independent transactions (I hope!) :)

1. leave the attributes of the splitted feature untouched and require the use to use the merge attributes tool, if he wants the new feature to have the same attributes.
2. leave attributes marked as hidden or readonly out of the attribute assignment to the new splitted off feature (the user could flag the pk attribute as hidden/readonly in that case)

The contextual menu patch can be a useful tool, anyway I would open the form dialog at the end of the split operation, as it happens when inserting a new feature.

Question: what about adding a capability to QgsVectorDataProvider to know the PK of a vector layer? In case the dataprovider supports it, when saving the edits a warning could be raised to aknowledge the user that a suplicated PK has been detected. The user will be free to discard the saveing or force the commit.

Another possible solution would be to show QgsMergeAttributesDialog after a split operation (the attribute form is less suitable since the split operation may create an arbitrary number of new features). That would not change the API. It would need some changes in the merge dialog to favor the default value of an attribute and some small changes in the split tool (which both should be possible in pre-release state).
What do think about it? (I could do the implementation, then Jürgen may fix the node tool)

Marco Hugentobler wrote:

Another possible solution would be to show QgsMergeAttributesDialog after a split operation (the attribute form is less suitable since the split operation may create an arbitrary number of new features). That would not change the API. It would need some changes in the merge dialog to favor the default value of an attribute and some small changes in the split tool (which both should be possible in pre-release state).
What do think about it? (I could do the implementation, then Jürgen may fix the node tool)

Hey, that's my idea (see comment 15). I'd even think that it doesn't need a change in API.

Anyway, fine with me.

(see comment 15)

Ah true, the comment list is too long to keep the overview :-)

I agree that having a dialog similar to QgsMergeAttributesDialog after the split operation would be simpler approach for the user. Obviously the dialog should be adapted to the different function.

I think that two key points for future developments are emerged from this ticket:

• having an improved transaction management, to be able to aggregate multiple operations in one single transaction, so that an error on one of them causes the rollback of all of thems
• be able to be informed by data providers in case their data sources have PKs/unique values fields. It would help to inform/prevent the user from committing operations that would cause constraints violations.