Navigation Menu

Skip to content

Commit

Permalink
[Backport release-3_12] PostgreSQL provider don't cast bigint PKs to …
Browse files Browse the repository at this point in the history 
…text (#35832)

* PostgreSQL provider don't cast bigint PKs to text

* Apply suggestions from code review

Fix merge conflicts

Co-Authored-By: José de Paula Rodrigues N. Assis <espinafre@gmail.com>

* Fix merge conflicts

Co-Authored-By: José de Paula Rodrigues N. Assis <espinafre@gmail.com>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Matthias Kuhn <matthias@opengis.ch>
Co-authored-by: José de Paula Rodrigues N. Assis <espinafre@gmail.com>
  • Loading branch information
@qgis-bot @github-actions
@m-kuhn @espinafre
4 people committed Apr 23, 2020
1 parent 72402e6 commit 566afd9
Show file tree
Hide file tree
Showing 8 changed files with 555 additions and 37 deletions.
11 changes: 9 additions & 2 deletions src/providers/postgres/qgspostgresconn.cpp
View file
Expand Up @@ -203,6 +203,7 @@ QgsPostgresConn::QgsPostgresConn( const QString &conninfo, bool readOnly, bool s
, mOpenCursors( 0 )
, mConnInfo( conninfo )
, mGeosAvailable( false )
, mProjAvailable( false )
, mTopologyAvailable( false )
, mGotPostgisVersion( false )
, mPostgresqlVersion( 0 )
Expand Down Expand Up @@ -1048,10 +1049,12 @@ QString QgsPostgresConn::postgisVersion() const
// apparently PostGIS 1.5.2 doesn't report capabilities in postgis_version() anymore
if ( mPostgisVersionMajor > 1 || ( mPostgisVersionMajor == 1 && mPostgisVersionMinor >= 5 ) )
{
result = PQexec( QStringLiteral( "SELECT postgis_geos_version()" ) );
result = PQexec( QStringLiteral( "SELECT postgis_geos_version(), postgis_proj_version()" ) );
mGeosAvailable = result.PQntuples() == 1 && !result.PQgetisnull( 0, 0 );
mProjAvailable = result.PQntuples() == 1 && !result.PQgetisnull( 0, 1 );
QgsDebugMsg( QStringLiteral( "geos:%1 proj:%2" )
.arg( mGeosAvailable ? result.PQgetvalue( 0, 0 ) : "none" ) );
.arg( mGeosAvailable ? result.PQgetvalue( 0, 0 ) : "none" )
.arg( mProjAvailable ? result.PQgetvalue( 0, 1 ) : "none" ) );
}
else
{
Expand Down Expand Up @@ -1620,6 +1623,10 @@ QString QgsPostgresConn::fieldExpression( const QgsField &fld, QString expr )
{
return QStringLiteral( "st_astext(%1)" ).arg( expr );
}
else if ( type == QLatin1String( "int8" ) )
{
return expr;
}
//TODO: add support for hstore
//TODO: add support for json/jsonb
else
Expand Down
4 changes: 4 additions & 0 deletions src/providers/postgres/qgspostgresconn.h
View file
Expand Up @@ -51,6 +51,7 @@ enum QgsPostgresPrimaryKeyType
{
PktUnknown,
PktInt,
PktInt64,
PktUint64,
PktTid,
PktOid,
Expand Down Expand Up @@ -391,6 +392,9 @@ class QgsPostgresConn : public QObject
//! GEOS capability
mutable bool mGeosAvailable;

//! PROJ capability
mutable bool mProjAvailable;

//! Topology capability
mutable bool mTopologyAvailable;

Expand Down
33 changes: 25 additions & 8 deletions src/providers/postgres/qgspostgresfeatureiterator.cpp
View file
Expand Up @@ -611,6 +611,7 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString &whereClause, long
break;

case PktInt:
case PktInt64:
case PktUint64:
query += delim + QgsPostgresConn::quotedIdentifier( mSource->mFields.at( mSource->mPrimaryKeyAttrs.at( 0 ) ).name() );
delim = ',';
Expand Down Expand Up @@ -663,7 +664,6 @@ bool QgsPostgresFeatureIterator::declareCursor( const QString &whereClause, long
return true;
}


bool QgsPostgresFeatureIterator::getFeature( QgsPostgresResult &queryResult, int row, QgsFeature &feature )
{
feature.initAttributes( mSource->mFields.count() );
Expand Down Expand Up @@ -744,20 +744,37 @@ bool QgsPostgresFeatureIterator::getFeature( QgsPostgresResult &queryResult, int
break;

case PktInt:
case PktUint64:
fid = mConn->getBinaryInt( queryResult, row, col++ );
if ( !subsetOfAttributes || fetchAttributes.contains( mSource->mPrimaryKeyAttrs.at( 0 ) ) )
{
feature.setAttribute( mSource->mPrimaryKeyAttrs[0], fid );
}
if ( mSource->mPrimaryKeyType == PktInt )
// NOTE: this needs be done _after_ the setAttribute call
// above as we want the attribute value to be 1:1 with
// database value
fid = QgsPostgresUtils::int32pk_to_fid( fid );
break;

case PktUint64:
case PktInt64:
{
QVariantList pkVal;

int idx = mSource->mPrimaryKeyAttrs.at( 0 );
QgsField fld = mSource->mFields.at( idx );

QVariant v = QgsPostgresProvider::convertValue( fld.type(), fld.subType(), QString::number( mConn->getBinaryInt( queryResult, row, col ) ), fld.typeName() );
pkVal << v;

if ( !subsetOfAttributes || fetchAttributes.contains( idx ) )
{
// NOTE: this needs be done _after_ the setAttribute call
// above as we want the attribute value to be 1:1 with
// database value
fid = QgsPostgresUtils::int32pk_to_fid( fid );
feature.setAttribute( idx, v );
}
break;
col++;

fid = mSource->mShared->lookupFid( pkVal );
}
break;

case PktFidMap:
{
Expand Down
83 changes: 59 additions & 24 deletions src/providers/postgres/qgspostgresprovider.cpp
View file
Expand Up @@ -76,11 +76,12 @@ QgsPostgresProvider::pkType( const QgsField &f ) const
switch ( f.type() )
{
case QVariant::LongLong:
// unless we can guarantee all values are unsigned
// (in which case we could use pktUint64)
// we'll have to use a Map type.
// See https://github.com/qgis/QGIS/issues/22258
return PktFidMap; // pktUint64
// PostgreSQL doesn't have native "unsigned" types.
// Unsigned primary keys are emulated by the serial/bigserial
// pseudo-types, in which autogenerated values are always > 0;
// however, the database accepts manually inserted 0 and negative values
// in these fields.
return PktInt64;

case QVariant::Int:
return PktInt;
Expand Down Expand Up @@ -275,11 +276,12 @@ QgsPostgresProvider::QgsPostgresProvider( QString const &uri, const ProviderOpti
key = QStringLiteral( "tid" );
break;
case PktInt:
case PktUint64:
Q_ASSERT( mPrimaryKeyAttrs.size() == 1 );
Q_ASSERT( mPrimaryKeyAttrs[0] >= 0 && mPrimaryKeyAttrs[0] < mAttributeFields.count() );
key = mAttributeFields.at( mPrimaryKeyAttrs.at( 0 ) ).name();
break;
case PktInt64:
case PktUint64:
case PktFidMap:
{
QString delim;
Expand Down Expand Up @@ -447,11 +449,12 @@ QString QgsPostgresProvider::pkParamWhereClause( int offset, const char *alias )
break;

case PktInt:
case PktUint64:
Q_ASSERT( mPrimaryKeyAttrs.size() == 1 );
whereClause = QStringLiteral( "%3%1=$%2" ).arg( quotedIdentifier( field( mPrimaryKeyAttrs[0] ).name() ) ).arg( offset ).arg( aliased );
break;

case PktInt64:
case PktUint64:
case PktFidMap:
{
QString delim;
Expand Down Expand Up @@ -488,7 +491,6 @@ void QgsPostgresProvider::appendPkParams( QgsFeatureId featureId, QStringList &p
switch ( mPrimaryKeyType )
{
case PktOid:
case PktUint64:
params << QString::number( featureId );
break;

Expand All @@ -500,6 +502,8 @@ void QgsPostgresProvider::appendPkParams( QgsFeatureId featureId, QStringList &p
params << QStringLiteral( "'(%1,%2)'" ).arg( FID_TO_NUMBER( featureId ) >> 16 ).arg( FID_TO_NUMBER( featureId ) & 0xffff );
break;

case PktInt64:
case PktUint64:
case PktFidMap:
{
QVariantList pkVals = mShared->lookupKey( featureId );
Expand Down Expand Up @@ -559,10 +563,22 @@ QString QgsPostgresUtils::whereClause( QgsFeatureId featureId, const QgsFields &
whereClause = QStringLiteral( "%1=%2" ).arg( QgsPostgresConn::quotedIdentifier( fields.at( pkAttrs[0] ).name() ) ).arg( FID2PKINT( featureId ) );
break;

case PktInt64:
case PktUint64:
{
Q_ASSERT( pkAttrs.size() == 1 );
whereClause = QStringLiteral( "%1=%2" ).arg( QgsPostgresConn::quotedIdentifier( fields.at( pkAttrs[0] ).name() ) ).arg( featureId );
break;
QVariantList pkVals = sharedData->lookupKey( featureId );
if ( !pkVals.isEmpty() )
{
QgsField fld = fields.at( pkAttrs[0] );
whereClause = conn->fieldExpression( fld );
if ( !pkVals[0].isNull() )
whereClause += '=' + pkVals[0].toString();
else
whereClause += QLatin1String( " IS NULL" );
}
}
break;

case PktFidMap:
{
Expand Down Expand Up @@ -609,7 +625,6 @@ QString QgsPostgresUtils::whereClause( const QgsFeatureIds &featureIds, const Qg
{
case PktOid:
case PktInt:
case PktUint64:
{
QString expr;

Expand All @@ -619,25 +634,49 @@ QString QgsPostgresUtils::whereClause( const QgsFeatureIds &featureIds, const Qg
QString delim;
expr = QStringLiteral( "%1 IN (" ).arg( ( pkType == PktOid ? QStringLiteral( "oid" ) : QgsPostgresConn::quotedIdentifier( fields.at( pkAttrs[0] ).name() ) ) );

const auto constFeatureIds = featureIds;
for ( const QgsFeatureId featureId : constFeatureIds )
for ( const QgsFeatureId featureId : qgis::as_const( featureIds ) )
{
expr += delim + FID_TO_STRING( ( pkType == PktOid ? featureId : pkType == PktUint64 ? featureId : FID2PKINT( featureId ) ) );
expr += delim + FID_TO_STRING( ( pkType == PktOid ? featureId : FID2PKINT( featureId ) ) );
delim = ',';
}
expr += ')';
}

return expr;
}
case PktInt64:
case PktUint64:
{
QString expr;

//simple primary key, so prefer to use an "IN (...)" query. These are much faster then multiple chained ...OR... clauses
if ( !featureIds.isEmpty() )
{
QString delim;
expr = QStringLiteral( "%1 IN (" ).arg( QgsPostgresConn::quotedIdentifier( fields.at( pkAttrs[0] ).name() ) );

for ( const QgsFeatureId featureId : qgis::as_const( featureIds ) )
{
QVariantList pkVals = sharedData->lookupKey( featureId );
if ( !pkVals.isEmpty() )
{
QgsField fld = fields.at( pkAttrs[0] );
expr += delim + pkVals[0].toString();
delim = ',';
}
}
expr += ')';
}

return expr;
}
case PktFidMap:
case PktTid:
case PktUnknown:
{
//complex primary key, need to build up where string
QStringList whereClauses;
const auto constFeatureIds = featureIds;
for ( const QgsFeatureId featureId : constFeatureIds )
for ( const QgsFeatureId featureId : qgis::as_const( featureIds ) )
{
whereClauses << whereClause( featureId, fields, conn, pkType, pkAttrs, sharedData );
}
Expand Down Expand Up @@ -2207,7 +2246,7 @@ bool QgsPostgresProvider::addFeatures( QgsFeatureList &flist, Flags flags )
bool skipSinglePKField = false;
bool overrideIdentity = false;

if ( ( mPrimaryKeyType == PktInt || mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktUint64 ) )
if ( ( mPrimaryKeyType == PktInt || mPrimaryKeyType == PktInt64 || mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktUint64 ) )
{
if ( mPrimaryKeyAttrs.size() == 1 &&
defaultValueClause( mPrimaryKeyAttrs[0] ).startsWith( "nextval(" ) )
Expand Down Expand Up @@ -2362,7 +2401,7 @@ bool QgsPostgresProvider::addFeatures( QgsFeatureList &flist, Flags flags )

if ( !( flags & QgsFeatureSink::FastInsert ) )
{
if ( mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktInt || mPrimaryKeyType == PktUint64 )
if ( mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktInt || mPrimaryKeyType == PktInt64 || mPrimaryKeyType == PktUint64 )
{
insert += QLatin1String( " RETURNING " );

Expand Down Expand Up @@ -2443,17 +2482,13 @@ bool QgsPostgresProvider::addFeatures( QgsFeatureList &flist, Flags flags )
if ( !( flags & QgsFeatureSink::FastInsert ) )
{
// update feature ids
if ( mPrimaryKeyType == PktInt || mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktUint64 )
if ( mPrimaryKeyType == PktInt || mPrimaryKeyType == PktInt64 || mPrimaryKeyType == PktFidMap || mPrimaryKeyType == PktUint64 )
{
for ( QgsFeatureList::iterator features = flist.begin(); features != flist.end(); ++features )
{
QgsAttributes attrs = features->attributes();

if ( mPrimaryKeyType == PktUint64 )
{
features->setId( STRING_TO_FID( attrs.at( mPrimaryKeyAttrs.at( 0 ) ) ) );
}
else if ( mPrimaryKeyType == PktInt )
if ( mPrimaryKeyType == PktInt )
{
features->setId( PKINT2FID( STRING_TO_FID( attrs.at( mPrimaryKeyAttrs.at( 0 ) ) ) ) );
}
Expand Down
2 changes: 1 addition & 1 deletion src/providers/postgres/qgspostgresprovider.h
View file
Expand Up @@ -503,7 +503,7 @@ class QgsPostgresUtils

static QString andWhereClauses( const QString &c1, const QString &c2 );

static const qint64 INT32PK_OFFSET = 4294967296;
static const qint64 INT32PK_OFFSET = 4294967296; // 2^32

// We shift negative 32bit integers to above the max 32bit
// positive integer to support the whole range of int32 values
Expand Down

0 comments on commit 566afd9

Please sign in to comment.