Opened 13 years ago

Closed 13 years ago

#1889 closed defect (fixed)

Postgres primary key logic disabled, needs to be re-enabled

Reported by: warmerdam Owned by: Mateusz Łoskot
Priority: normal Milestone: 1.4.4
Component: OGR_SF Version: 1.4.2
Severity: normal Keywords: postgresql postgis constraints PK
Cc: darkblueB

Description

Mateusz,

The automatic recognition of primary keys as FIDs in the PG driver is currently (trunk and 1.4.x) broken and this makes it impossible to do some operations on tables (such as DeleteFeature?()) that weren't created by OGR (thus using the recognisable ogc_fid name as fid).

    /* TODO make changes corresponded to Frank issues
       
    sprintf ( szCommand,
              "SELECT a.attname "
              "FROM pg_attribute a, pg_constraint c, pg_class cl "
              "WHERE c.contype='p' AND c.conrelid=cl.oid "
              "AND a.attnum = c.conkey[1] AND a.attrelid=cl.oid "
              "AND cl.relname = '%s'",
              pszTableIn );

    hResult = PQexec(hPGConn, szCommand );

    if ( hResult && PQntuples( hResult ) == 1 && PQgetisnull( hResult,0,0 ) == false )
    {
        sprintf( szPrimaryKey, "%s", PQgetvalue(hResult,0,0) );
        CPLDebug( "OGR_PG", "Primary key name (FID): %s", szPrimaryKey );
    }
    else
    {
        CPLError( CE_Failure, CPLE_AppDefined,
                  "%s", PQerrorMessage(hPGConn) );
        CPLError( CE_Warning, CPLE_AppDefined,
                  "Unable to detect table primary key. Use default 'ogc_fid'");
    }*/

I don't know what Frank issues the comment refers to off hand, though it could be that we need to confirm the primary key is an integer field. Once this logic has been appropriately reenable we need to add a test to the postgres testsuite and ensure it is operational on at least one buildbot platform (likely telascience-full).

Change History (14)

comment:1 Changed 13 years ago by Mateusz Łoskot

Status: newassigned

Frank,

Could you refer to the comment:

I don't know what Frank issues the comment refers to off hand(...) ?

comment:2 Changed 13 years ago by Mateusz Łoskot

Keywords: postgresql postgis added; postgres removed
Priority: normalhigh

comment:3 Changed 13 years ago by warmerdam

The code is currently commented out with this message:

    /* TODO make changes corresponded to Frank issues

I don't know what Frank issues refers to. Was it you that commented out the code with this remark?

comment:4 Changed 13 years ago by Mateusz Łoskot

Frank, yes I'm referring to this comment but I'm not the author of it and also I didn't commented this code. I tried to find author using Trac but seems it's very old and not available in Trac. Usually, I mark my comments with mloskot.

Anyway, I don't know what issues are referred here :-(

comment:5 Changed 13 years ago by Mateusz Łoskot

The comment was made by Oleg Semykin and thanks to Andrey we've managed get some clarifications about his notes. Here are Oleg's comments forwarded by Andrey:

There should be a code to determine whether we have a single field as a primary key. If so the commented out code will work. Sometimes we can have more than one field as a key and there should be some more code to handle this case. And that was a FrankW concern.

and Frank's additional comments about multi-column PK support, in future:

I don't see how we can ever support multi-field primary keys without major changes to OGR. We just have to recognise when the pk will not map to our 32bit integer fid, and fallback to assigning fids.

comment:6 Changed 13 years ago by Mateusz Łoskot

Keywords: constraints PK added
Resolution: fixed
Status: assignedclosed

I've fixed this issue (r12501) and currently single-column Primary Key should be detected and mapped to FID correctly. If multi-column PK is detected, PG driver will map virtual ogc_fid to feature ID values.

comment:7 Changed 13 years ago by Mateusz Łoskot

One additional note, Primary Key is detected as supported and mapped to FID values if both assertions are true:

  1. PK for a table consists of single column only
  1. Data type of column used as single-column PK is of one of the following types: smallint (16-bit), integer (32-bit) or serial (32-bit) (see Integer Types in PostgreSQL manual)

Also, if multi-column PK is detected, appropriate warning is printed (point2 is name of table used in this example):

Warning 1: Multi-column primary key detected but not supported.
Warning 1: Unable to detect single-column primary key for 'point2'. Use default 'ogc_fid'

I hope it makes sense.

comment:8 Changed 13 years ago by Mateusz Łoskot

The fix has been ported to the branches/1.4 (r12510)

comment:9 Changed 13 years ago by warmerdam

Milestone: 1.4.31.4.4
Priority: highnormal
Resolution: fixed
Status: closedreopened

Mateusz,

We need to remove the warning issued when there is no primary key:

Warning 1: Unable to detect single-column primary key for
'Gebaeudeadressen__Strassenstueck'. Use default 'ogc_fid'

This generates huge amounts of confusing noise when accessing substantial databases or databases loaded with OGR.

comment:10 Changed 13 years ago by Mateusz Łoskot

Resolution: fixed
Status: reopenedclosed

Fixed in trunk (r12650)

comment:11 Changed 13 years ago by Ari Jolma

The current fix r12501 uses ANY(pg_index.indkey) where indkey is int2vector. Support for ANY on int2vector is very recent in PostgreSQL. It is not in 8.0 but is in 8.2. Would there be any other way to implement this functionality than depending on such new feature?

Ari

comment:12 Changed 13 years ago by warmerdam

Resolution: fixed
Status: closedreopened

It is very desirable for the PG driver to work with reasonably old Postgres' - at least back to 8.0, if not back into the 7.x'es (even if with moderately reduced functionality). Reopening accordingly.

comment:13 Changed 13 years ago by Ari Jolma

According to this: http://archives.postgresql.org/pgsql-sql/2005-08/msg00148.php there is no nice SQL solution, but there is a solution.

comment:14 Changed 13 years ago by Mateusz Łoskot

Resolution: fixed
Status: reopenedclosed

Applied patch to the trunk (r12931) and branches/1.4 (r12932)

Note: See TracTickets for help on using tickets.