Bug report #13641

editing a feature in a PostGIS layer does not work when the PK contains NULLs

Added by Sebastian Dietrich over 8 years ago. Updated over 8 years ago.

Status:Closed
Priority:Normal
Assignee:Jürgen Fischer
Category:Data Provider/PostGIS
Affected QGIS version:2.6.0 Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:No Resolution:
Crashes QGIS or corrupts data:No Copied to github as #:21677

Description

If the primary key for a feature in a PostGIS layer contains NULL values any edits will be ignored.

Associated revisions

Revision 55babc3e
Added by Jürgen Fischer over 8 years ago

postgres provider: support NULL in composite keys (fixes #13641)

Revision a3f41975
Added by Jürgen Fischer over 8 years ago

postgres provider: verify unique constraint it NOT NULL is not set on key columns (fixes #13641)

History

#1 Updated by Sebastian Dietrich over 8 years ago

To reproduce:
  • Create a table
    CREATE TABLE "TestTable" 
(
  "ID1" integer,
  "ID2" integer,
  "Value" character varying NOT NULL,
  CONSTRAINT "U_Test" UNIQUE ("ID1", "ID2")
)
  • Insert a tuple
    INSERT INTO "TestTable" 
VALUES
(1, NULL, 'abc')
  • Load this table in QGIS. (Note that it does not have a geometry column.)
  • Open the attribute table and edit the text, e.g. change it to xyz.
  • Save the changes and close the attribute table.
  • Reopen the attribute table and note that the text still reads abc.

#2 Updated by Sebastian Dietrich over 8 years ago

If you change the offending NULL value to something else you can successfully edit the feature.

UPDATE "TestTable" 
SET "ID2" = 1

#3 Updated by Jürgen Fischer over 8 years ago

  • Status changed from Open to Closed

#4 Updated by Sebastian Dietrich over 8 years ago

  • Status changed from Closed to Reopened

This doesn't appear to be the solution. Fill the table like this

INSERT INTO "TestTable" 
VALUES
(1, NULL, 'abc')
(1, NULL, 'def')

Load it into QGIS and have a look at the attribute table:
It looks different depending on whether you loaded the table using Select at ID or not, but it is wrong in both cases. It either shows two identical features or only one feature. And in any case editing one feature changes both tuples in the database, which could qualify as data corruption.

I think we have a bigger issue here because actually a UNIQUE constraint alone is not sufficient as a primary key. We have to check for every column within the UNIQUE constraint to be set to NOT NULL. Of course this would break many installations where the actual values are not NULL but the NOT NULL constraint is not set. So we probably don't want to do that until the next major release?

A more compatible solution could be to just throw an error if the user tries to edit a feature having NULL as part of a composite key. And maybe issue a warning when loading such features, because the layer might be incomplete or corrupted?

And as composite keys for views and queries is a new feature in the upcoming release, we should forbid NULL as part of such a composite key right from the start.

#5 Updated by Jürgen Fischer over 8 years ago

Sebastian Dietrich wrote:

And as composite keys for views and queries is a new feature in the upcoming release, we should forbid NULL as part of such a composite key right from the start.

It doesn't apply to views and queries - there uniqueData() is invoked and rejects the layer, if the tuples are not unique. If the values are unique (including NULL values) it works with the fix.

But QGIS wrongly assumes that a unique constraint on a table is enough to assure that there are only unique tuples and hence doesn't invoke uniqueData() to verify and in turn doesn't reject that constraint if there are duplicate keys due to NULL values.

I suppose that is a very rare issue - and changing that by invoking uniqueData() (like in https://gist.github.com/jef-n/665a69649db5e440865b) might slow down existing projects - that aren't even affected by this problem. So I'll leave that for 2.13.

#6 Updated by Jürgen Fischer over 8 years ago

  • Target version set to Future Release - High Priority

#7 Updated by Jürgen Fischer over 8 years ago

  • Status changed from Reopened to Closed

Also available in: Atom PDF