Bug report #14262

Overflow on primary key with negative values; cannot save edits

Added by Sandro Santilli about 1 year ago. Updated 10 months ago.

Status:Closed Start Date:02/09/2016
Priority:Normal Due date:
Assigned to:Sandro Santilli % Done:

100%

Category:PostGIS Data Provider
Target version:Version 2.16
Platform: Pull Request or Patch supplied:Yes
Platform version: Affected version:2.14.3
Status info: Causes crash or corruption:No
Resolution:fixed/implemented Tag:

Description

Spin off of #13958

Create a simple PostGIS table:


CREATE table my_lines(gid integer primary key, col integer);
SELECT AddGeometryColumn('my_lines', 'geom', 4326, 'MULTILINESTRING', 2);

INSERT INTO my_lines(gid, col, geom)
SELECT -1, 2, 'SRID=4326;MULTILINESTRING((0 0, 1 1))';

Note that the primary key has a negative value -1, which should be completely fine and previous versions of QGIS were OK with this.

This table can be added to QGIS, which shows the simple geometry on the main canvas normally, and the Layer properties correctly describes both gid and col fields as int4.

However, the attribute table shows the values of gid and col as ERROR, and clicking the feature with the Identify Features tool show gid of 4294967295, which is the overflow version of an unsigned integer value -1. Perhaps the primary key was internally cast in QGIS to unsigned long?

Editing the layer, e.g. with the Node tool fail on save, with this message:

src/providers/postgres/qgspostgresprovider.cpp: 2322: (changeGeometryValues) [1279ms] entering.
src/providers/postgres/qgspostgresprovider.cpp: 2395: (changeGeometryValues) [0ms] updating: UPDATE "public"."my_lines" SET "geom"=st_multi(st_geom
fromwkb($1::bytea,4326)) WHERE "gid"=$2
src/providers/postgres/qgspostgresprovider.cpp: 2405: (changeGeometryValues) [1ms] iterating over the map of changed geometries...
src/providers/postgres/qgspostgresprovider.cpp: 2411: (changeGeometryValues) [0ms] iterating over feature id 4294967295
src/core/qgsvectordataprovider.cpp: 560: (pushError) [0ms] PostGIS error while changing geometry values: ERROR:  value "4294967295" is out of range
 for type integer

src/providers/postgres/qgspostgresprovider.cpp: 2501: (changeGeometryValues) [0ms] leaving.
src/core/qgsmessagelog.cpp: 45: (logMessage) [1ms] 2016-02-09T17:20:03 [1] Commit errors:
  ERROR: 1 geometries not changed.

  Provider errors:
      PostGIS error while changing geometry values: ERROR:  value "4294967295" is out of range for type integer

Affected: master branch 2.14.0dev (b9726d7285733c27d42456c115e28d5a37f3e0be)

Note that 2.8.3 crashes on first edit attempt. The crash is fixed in current master.

History

Updated by Giovanni Manghi about 1 year ago

  • Status changed from Open to Feedback

if it worked ok on previous qgis releases this should be tagged as blocker and have 2.14 has target. Cheers!

Updated by Sandro Santilli about 1 year ago

As mentioned, it crashed with 2.8.4. I hadn't tested any previous release, but 2.8 being an LTR I wouldn't go any earlier

Updated by Giovanni Manghi about 1 year ago

Sandro Santilli wrote:

As mentioned, it crashed with 2.8.4. I hadn't tested any previous release, but 2.8 being an LTR I wouldn't go any earlier

I'm not speaking about the crash, but about the subject of this ticket ("Note that the primary key has a negative value -1, which should be completely fine and previous versions of QGIS were OK with this.").

Updated by Sandro Santilli about 1 year ago

Mike Toews did not specify which previous version worked. I did test with 2.8.4 and the feature id is still reported as an unsigned and attribute values reported as ERROR. Do you confirm, Giovanni ?

Updated by Sandro Santilli about 1 year ago

I just tried 2.6.1-Brighton and it also displays ERROR in attribute values, max unsigned integer on identify and crashes on edit.

Updated by Sandro Santilli about 1 year ago

2.4.0-Chugiak same behavior as 2.6.1-Brighton and 2.8.4

Updated by Sandro Santilli about 1 year ago

It looks like QgsPostgresConn::getBinaryInt is responsible of converting the signed integer to an unsigned one:

qint64 QgsPostgresConn::getBinaryInt( QgsPostgresResult &queryResult, int row, int col )
{                                                                               
  quint64 oid; 

Jurgen, what's the rationale for that ?

Updated by Sandro Santilli about 1 year ago

  • Status changed from Feedback to In Progress
  • Assigned to set to Sandro Santilli
  • Target version changed from Future Release - High Priority to Version 2.14
  • % Done changed from 0 to 80
  • Pull Request or Patch supplied changed from No to Yes

Updated by Sandro Santilli about 1 year ago

Saving edits is handled by not treating integer fields in any special way by
https://github.com/qgis/QGIS/pull/2797

Updated by Sandro Santilli about 1 year ago

  • Category set to PostGIS Data Provider

Updated by Sandro Santilli about 1 year ago

Saves can be edited with https://github.com/qgis/QGIS/pull/2797, that is using a field map based feature id for the case of integer primary key.

Updated by Sandro Santilli 10 months ago

Current focus for this bug is here: https://github.com/qgis/QGIS/pull/3036

Updated by Sandro Santilli 10 months ago

  • Status changed from In Progress to Closed
  • Target version changed from Version 2.14 to Version 2.16
  • % Done changed from 80 to 100
  • Resolution set to fixed/implemented
  • Affected version changed from master to 2.14.3

Fixed as of d1cac84 -- I suspect this didn't work in 2.8 either.

Updated by Sandro Santilli 10 months ago

Question: should this be backported to 2.14 ?

Updated by Sandro Santilli 10 months ago

  • Status changed from Closed to Reopened

Will test against 2.14, but given 2.14 is the LTR, it'd be best fixed there (please correct me if I'm wrong)

Updated by Sandro Santilli 10 months ago

  • Status changed from Reopened to Closed

Tested: the crash is fixed in 2.14(.3) while the "cannot save edits" part is only fixed in 2.16.
As we aim for stability, I'd not backport the ability to edit (or save edits) in 2.14. Feel free to reopen if you think it should be done instead.

Also available in: Atom