Opened 9 years ago

Closed 9 years ago

#3744 closed enhancement (fixed)

PG driver doesn't retrieve the FID of a newly inserted feature

Reported by: Even Rouault Owned by: Even Rouault
Priority: normal Milestone: 1.8.0
Component: default Version: unspecified
Severity: normal Keywords: pg
Cc: warmerdam

Description (last modified by Even Rouault)

Currently the PG driver doesn't retrieve the FID of a newly inserted feature as raised on the mailing list : http://lists.osgeo.org/pipermail/gdal-dev/2010-September/025912.html

There was some historical commented code in the driver that attempted to retrieve the OID of the feature to use it as FID, but this doesn't work as nowadays OID are not used unless you ask for it at table creation.

The nice way to get the FID is to add "RETURN " + pszFIDColumnName clause at the end of the INSERT command (Postgresql >= 8.2)

This works very well, but there's a trade-off. Currently, the driver tries to set the FID if the feature has already one. I fail to see a good use case for that (and the code coverage analysis of the autotest suite shows that it is an untested code path), but who knows ? maybe someone relies on this. The side effect of that behaviour with the new retrieval of the FID is that the following snippet won't work :

feat = ogr.Feature(lyr.GetLayerDefn())
for i in range(10):
    feat.SetField('foo', 'bar')
    lyr.CreateFeature(feat)

Now you'll need to do :

feat = ogr.Feature(lyr.GetLayerDefn())
for i in range(10):
    feat.SetFID(-1)
    feat.SetField('foo', 'bar')
    lyr.CreateFeature(feat)

The autotest suite indeed needs this change in 2 places...

So what I'm about to do is to display a warning when I detect this situation to give a hint to the users on what they must correct. I also propose a configuration option OGR_PG_RETRIEVE_FID that could be set to NO for people wanting the old behaviour.

Another possibility is to just remove the code that tries to set the FID when it is not null in the INSERT command.

Any thought ? Patch attached.

(Note: the FID retrieval only works for regular CreateFeature?() via INSERT, not in the COPY mode of course)

Attachments (1)

ogrpg_get_fid.patch (5.6 KB) - added by Even Rouault 9 years ago.

Download all attachments as: .zip

Change History (6)

Changed 9 years ago by Even Rouault

Attachment: ogrpg_get_fid.patch added

comment:1 Changed 9 years ago by Even Rouault

Description: modified (diff)

comment:2 Changed 9 years ago by bishop

May be this patch helps? http://trac.osgeo.org/gdal/ticket/2614

comment:3 Changed 9 years ago by Even Rouault

Bishop,

Ah, your patch looks like an alternative implementation to mine, right ? A drawback I see is that it requires an extra SQL command. On the plus side, I guess it doesn't require postgres >= 8.2

But the backward compatibility problem I raised is still the same.

comment:4 Changed 9 years ago by bishop

I think your code more simple and quick, and option OGR_PG_RETRIEVE_FID idea is great. But may be the new feature ID set to -1 on default? If anybody wants to set other ID, he/she should set it manual in code.

    feat.SetFID(99)

comment:5 Changed 9 years ago by Even Rouault

Resolution: fixed
Status: newclosed

Commited in trunk in r21037. Test adapted in r21038

Note: See TracTickets for help on using tickets.