Skip to content

Commit

Permalink
postgres provider: verify uniqueness of parent table primary keys (fixes
Browse files Browse the repository at this point in the history 
 #13947)
  • Loading branch information
@jef-n
jef-n committed Dec 4, 2015
1 parent 7829497 commit fc7fea5
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/providers/postgres/qgspostgresprovider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1164,10 +1164,15 @@ bool QgsPostgresProvider::determinePrimaryKey()
QString sql;
if ( !mIsQuery )
{
sql = QString( "SELECT count(*) FROM pg_inherits WHERE inhparent=%1::regclass" ).arg( quotedValue( mQuery ) );
QgsDebugMsg( QString( "Checking whether %1 is a parent table" ).arg( sql ) );
QgsPostgresResult res( connectionRO()->PQexec( sql ) );
bool isParentTable( res.PQntuples() == 0 || res.PQgetvalue( 0, 0 ).toInt() > 0 );

sql = QString( "SELECT indexrelid FROM pg_index WHERE indrelid=%1::regclass AND (indisprimary OR indisunique) ORDER BY CASE WHEN indisprimary THEN 1 ELSE 2 END LIMIT 1" ).arg( quotedValue( mQuery ) );
QgsDebugMsg( QString( "Retrieving first primary or unique index: %1" ).arg( sql ) );

QgsPostgresResult res( connectionRO()->PQexec( sql ) );
res = connectionRO()->PQexec( sql );
QgsDebugMsg( QString( "Got %1 rows." ).arg( res.PQntuples() ) );

QStringList log;
Expand Down Expand Up @@ -1220,7 +1225,6 @@ bool QgsPostgresProvider::determinePrimaryKey()
QgsMessageLog::logMessage( tr( "The table has no column suitable for use as a key. QGIS requires a primary key, a PostgreSQL oid column or a ctid for tables." ), tr( "PostGIS" ) );
}
}

}
else if ( type == "v" || type == "m" ) // the relation is a view
{
Expand Down Expand Up @@ -1276,9 +1280,9 @@ bool QgsPostgresProvider::determinePrimaryKey()

mPrimaryKeyType = ( mPrimaryKeyAttrs.size() == 1 && isInt ) ? pktInt : pktFidMap;

if ( mightBeNull && !mUseEstimatedMetadata && !uniqueData( primaryKey ) )
if ( ( mightBeNull || isParentTable ) && !mUseEstimatedMetadata && !uniqueData( primaryKey ) )

This comment has been minimized.

Sign in to view

Copy link
@elpaso

elpaso Dec 7, 2015

Contributor

@jef-n the uniqueData test is skipped when mUseEstimatedMetadata, unfortunately this will not catch the problem when use estimated metadata is set. Shouldn't we test with
if ( ( mightBeNull || isParentTable ) && (!mUseEstimatedMetadata || !uniqueData( primaryKey ) ) )
?

This comment has been minimized.

Sign in to view

Copy link
@jef-n

jef-n Dec 7, 2015

Author Member

No, mUseEstimatedMetadata is there to avoid the expensive query

This comment has been minimized.

Sign in to view

Copy link
@elpaso

elpaso Dec 8, 2015

Contributor

What if we always check uniqueData in case isParentTable ? In my test case with 1.8x10^6 records it takes less than 2 seconds.
If we don't check for uniqeData we might end up with duplicated ids.

This comment has been minimized.

Sign in to view

Copy link
@jef-n

jef-n Dec 8, 2015

Author Member

Why not disable mUseEstimatedMetadata on that layer? And uniqueData will only verify that the ids are unique once - that might still change, if something new is added or something existing is changed. IMHO there should be something in place on the database side to prevent that this happens in the first place.

This comment has been minimized.

Sign in to view

Copy link
@elpaso

elpaso Dec 8, 2015

Contributor

I fully agree with you that the root problem is bad design of the DB. What I'm thinking of is to make QGIS more robust to handle not unique QgsFeature ids. The problem showed up here: https://github.com/elpaso/QGIS/blob/bugfix-12926-attributetable-speedup/src/gui/attributetable/qgsattributetablemodel.cpp#L238 (that crashes on Q_ASSERT if debug is on). I've fixed that with: https://github.com/elpaso/QGIS/blob/bugfix-12926-attributetable-speedup/src/gui/attributetable/qgsattributetablemodel.cpp#L405 that checks for existing fid in hash before adding.
There are probably other places where we rely on unique QgsFeature ids.
Checking for unique ids on layer load is IMHO better than nothing.
{
QgsMessageLog::logMessage( tr( "Ignoring key candidate because of NULL values" ), tr( "PostGIS" ) );
QgsMessageLog::logMessage( tr( "Ignoring key candidate because of NULL values or inheritance" ), tr( "PostGIS" ) );
mPrimaryKeyType = pktUnknown;
mPrimaryKeyAttrs.clear();
}
Expand Down

0 comments on commit fc7fea5

Please sign in to comment.