Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[postgres] Fix checkPrimaryKeyUnicity option
This provider option was linked to the project level option
"Trust layer metadata..." which was implemented
to speed up loading of large dataset by trusting extent
read from metadata to avoid costly operations to determine
the layer extent.

Check PK unicity on the other hand has only effect on views
and query layers and it is useful as an independent
option to prevent loading of layers that have no PK (or the
wrong one).

But the operation of determine unicity of a values in a column
can also be costly, so better to get control back to the user.

Legacy default is preserved (the project-level "Trust..." option).

Fixes #21839

Funded by RAAB.nl
  • Loading branch information
elpaso committed Apr 24, 2019
1 parent 51d8286 commit 38c91e4
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 46 deletions.
2 changes: 2 additions & 0 deletions src/app/qgsprojectproperties.cpp
Expand Up @@ -995,6 +995,8 @@ void QgsProjectProperties::apply()
// Set the project title
QgsProject::instance()->setTitle( title() );
QgsProject::instance()->setPresetHomePath( QDir::fromNativeSeparators( mProjectHomeLineEdit->text() ) );

// DB-related options
QgsProject::instance()->setAutoTransaction( mAutoTransaction->isChecked() );
QgsProject::instance()->setEvaluateDefaultValues( mEvaluateDefaultValues->isChecked() );
QgsProject::instance()->setTrustLayerMetadata( mTrustProjectCheckBox->isChecked() );
Expand Down
26 changes: 1 addition & 25 deletions src/core/qgsvectorlayer.cpp
Expand Up @@ -1573,24 +1573,8 @@ QString QgsVectorLayer::loadDefaultStyle( bool &resultFlag )
bool QgsVectorLayer::setDataProvider( QString const &provider, const QgsDataProvider::ProviderOptions &options )
{
mProviderKey = provider;

// primary key unicity is tested at construction time, so it has to be set
// before initializing postgres provider
QString checkUnicityKey = QStringLiteral( "checkPrimaryKeyUnicity" );
QString dataSource = mDataSource;
if ( provider.compare( QLatin1String( "postgres" ) ) == 0 )
{
QgsDataSourceUri uri( dataSource );

if ( uri.hasParam( checkUnicityKey ) )
uri.removeParam( checkUnicityKey );

uri.setParam( checkUnicityKey, mReadExtentFromXml ? "0" : "1" );
dataSource = uri.uri( false );
}

delete mDataProvider;
mDataProvider = qobject_cast<QgsVectorDataProvider *>( QgsProviderRegistry::instance()->createProvider( provider, dataSource, options ) );
mDataProvider = qobject_cast<QgsVectorDataProvider *>( QgsProviderRegistry::instance()->createProvider( provider, mDataSource, options ) );
if ( !mDataProvider )
{
mValid = false;
Expand Down Expand Up @@ -1649,15 +1633,7 @@ bool QgsVectorLayer::setDataProvider( QString const &provider, const QgsDataProv
if ( !lName.isEmpty() )
setName( lName );
}

QgsDebugMsgLevel( QStringLiteral( "Beautified layer name %1" ).arg( name() ), 3 );

// deal with unnecessary schema qualification to make v.in.ogr happy
// and remove unnecessary key
QgsDataSourceUri dataProviderUri( mDataProvider->dataSourceUri() );
if ( dataProviderUri.hasParam( checkUnicityKey ) )
dataProviderUri.removeParam( checkUnicityKey );
mDataSource = dataProviderUri.uri( false );
}
else if ( mProviderKey == QLatin1String( "osm" ) )
{
Expand Down
24 changes: 19 additions & 5 deletions src/providers/postgres/qgspgtablemodel.cpp
Expand Up @@ -20,6 +20,7 @@
#include "qgslogger.h"
#include "qgsapplication.h"
#include "qgssettings.h"
#include "qgsproject.h"

#include <climits>

Expand All @@ -39,7 +40,7 @@ QgsPgTableModel::QgsPgTableModel()
headerLabels << tr( "Sql" );
setHorizontalHeaderLabels( headerLabels );
setHeaderData( 8, Qt::Orientation::Horizontal, tr( "Disable 'Fast Access to Features at ID' capability to force keeping the attribute table in memory (e.g. in case of expensive views)." ), Qt::ToolTipRole );
setHeaderData( 9, Qt::Orientation::Horizontal, tr( "Enable check for primary key unicity when loading the features. This may slow down loading for large tables." ), Qt::ToolTipRole );
setHeaderData( 9, Qt::Orientation::Horizontal, tr( "Enable check for primary key unicity when loading views and materialized views. This option can make loading of large datasets significantly slower." ), Qt::ToolTipRole );
}

void QgsPgTableModel::addTableEntry( const QgsPostgresLayerProperty &layerProperty )
Expand Down Expand Up @@ -127,8 +128,21 @@ void QgsPgTableModel::addTableEntry( const QgsPostgresLayerProperty &layerProper

QStandardItem *checkPkUnicityItem = new QStandardItem( QString() );
checkPkUnicityItem->setFlags( checkPkUnicityItem->flags() | Qt::ItemIsUserCheckable );
checkPkUnicityItem->setCheckState( Qt::Unchecked );
checkPkUnicityItem->setToolTip( headerData( 9, Qt::Orientation::Horizontal, Qt::ToolTipRole ).toString() );

// Legacy: default value is determined by project option to trust layer's metadata
// TODO: remove this default from QGIS 4 and leave default value to false
// checkPkUnicity has only effect on views and materialized views, so we can safely disable it
if ( layerProperty.isView || layerProperty.isMaterializedView )
{
checkPkUnicityItem->setCheckState( QgsProject::instance( )->trustLayerMetadata() ? Qt::CheckState::Unchecked : Qt::CheckState::Checked );
checkPkUnicityItem->setToolTip( headerData( 9, Qt::Orientation::Horizontal, Qt::ToolTipRole ).toString() );
}
else
{
checkPkUnicityItem->setCheckState( Qt::CheckState::Unchecked );
checkPkUnicityItem->setFlags( checkPkUnicityItem->flags() & ~ Qt::ItemIsEnabled );
checkPkUnicityItem->setToolTip( tr( "This option is only available for views and materialized views." ) );
}

QStandardItem *sqlItem = new QStandardItem( layerProperty.sql );

Expand Down Expand Up @@ -384,7 +398,7 @@ QString QgsPgTableModel::layerURI( const QModelIndex &index, const QString &conn

bool selectAtId = itemFromIndex( index.sibling( index.row(), DbtmSelectAtId ) )->checkState() == Qt::Checked;
QString sql = index.sibling( index.row(), DbtmSql ).data( Qt::DisplayRole ).toString();
bool checkPkUnicity = itemFromIndex( index.sibling( index.row(), DbtmCheckPkUnicity ) )->checkState() == Qt::Checked;
bool checkPrimaryKeyUnicity = itemFromIndex( index.sibling( index.row(), DbtmCheckPkUnicity ) )->checkState() == Qt::Checked;

QgsDataSourceUri uri( connInfo );

Expand All @@ -402,7 +416,7 @@ QString QgsPgTableModel::layerURI( const QModelIndex &index, const QString &conn
uri.setWkbType( wkbType );
uri.setSrid( srid );
uri.disableSelectAtId( !selectAtId );
uri.setParam( QStringLiteral( "checkPrimaryKeyUnicity" ), QString( checkPkUnicity ) );
uri.setParam( QStringLiteral( "checkPrimaryKeyUnicity" ), checkPrimaryKeyUnicity ? QLatin1Literal( "1" ) : QLatin1Literal( "0" ) );

QgsDebugMsg( QStringLiteral( "returning uri %1" ).arg( uri.uri( false ) ) );
return uri.uri( false );
Expand Down
2 changes: 0 additions & 2 deletions src/providers/postgres/qgspostgresconn.h
Expand Up @@ -82,8 +82,6 @@ struct QgsPostgresLayerProperty
bool isView = false;
bool isMaterializedView = false;
QString tableComment;
bool checkPkUnicity = false;


// TODO: rename this !
int size() const { Q_ASSERT( types.size() == srids.size() ); return types.size(); }
Expand Down
3 changes: 1 addition & 2 deletions src/providers/postgres/qgspostgresprovider.cpp
Expand Up @@ -1323,7 +1323,7 @@ bool QgsPostgresProvider::determinePrimaryKey()

QStringList log;

// no primary or unique indizes found
// no primary or unique indices found
if ( res.PQntuples() == 0 )
{
QgsDebugMsg( QStringLiteral( "Relation has no primary key -- investigating alternatives" ) );
Expand Down Expand Up @@ -1601,7 +1601,6 @@ bool QgsPostgresProvider::uniqueData( const QString &quotedColNames )
pushError( unique.PQresultErrorMessage() );
return false;
}

return unique.PQntuples() == 1 && unique.PQgetvalue( 0, 0 ).startsWith( 't' );
}

Expand Down
24 changes: 12 additions & 12 deletions src/ui/qgsprojectpropertiesbase.ui
Expand Up @@ -236,7 +236,7 @@
</sizepolicy>
</property>
<property name="currentIndex">
<number>8</number>
<number>4</number>
</property>
<widget class="QWidget" name="mProjOptsGeneral">
<layout class="QVBoxLayout" name="verticalLayout_6">
Expand Down Expand Up @@ -265,8 +265,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>590</width>
<height>777</height>
<width>671</width>
<height>795</height>
</rect>
</property>
<layout class="QVBoxLayout" name="verticalLayout_8">
Expand Down Expand Up @@ -863,8 +863,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>603</width>
<height>161</height>
<width>685</width>
<height>780</height>
</rect>
</property>
<layout class="QVBoxLayout" name="verticalLayout_7">
Expand Down Expand Up @@ -938,8 +938,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>290</width>
<height>543</height>
<width>685</width>
<height>780</height>
</rect>
</property>
<layout class="QVBoxLayout" name="verticalLayout_12">
Expand Down Expand Up @@ -1378,7 +1378,7 @@
<item row="3" column="0">
<widget class="QCheckBox" name="mTrustProjectCheckBox">
<property name="toolTip">
<string>Speed up project loading by skipping data checks. Useful in qgis server context or project with huge database views or materialized views.</string>
<string>Speed up project loading by skipping data checks in Postgres layers. Useful in QGIS server context or project with huge database views or materialized views.</string>
</property>
<property name="text">
<string>Trust project when data source has no metadata</string>
Expand Down Expand Up @@ -1514,8 +1514,8 @@
<rect>
<x>0</x>
<y>0</y>
<width>178</width>
<height>54</height>
<width>167</width>
<height>55</height>
</rect>
</property>
<layout class="QVBoxLayout" name="verticalLayout_17">
Expand Down Expand Up @@ -1575,9 +1575,9 @@
<property name="geometry">
<rect>
<x>0</x>
<y>-975</y>
<y>0</y>
<width>671</width>
<height>2682</height>
<height>2732</height>
</rect>
</property>
<layout class="QVBoxLayout" name="verticalLayout_13">
Expand Down
15 changes: 15 additions & 0 deletions tests/src/python/test_provider_postgres.py
Expand Up @@ -1097,6 +1097,21 @@ def testReadExtentOnTable(self):

self.assertEqual(vl2.extent(), originalExtent)

def testCheckPkUnicityOnView(self):
# vector layer based on view

# This is valid
vl0 = QgsVectorLayer(self.dbconn + ' checkPrimaryKeyUnicity=\'0\' sslmode=disable key=\'pk\' srid=0 type=POINT table="qgis_test"."b21839_pk_unicity_view" (geom) sql=', 'test', 'postgres')
self.assertTrue(vl0.isValid())

# This is NOT valid
vl0 = QgsVectorLayer(self.dbconn + ' checkPrimaryKeyUnicity=\'1\' sslmode=disable key=\'an_int\' srid=0 type=POINT table="qgis_test"."b21839_pk_unicity_view" (geom) sql=', 'test', 'postgres')
self.assertFalse(vl0.isValid())

# This is valid because the default is to not check unicity
vl0 = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'an_int\' srid=0 type=POINT table="qgis_test"."b21839_pk_unicity_view" (geom) sql=', 'test', 'postgres')
self.assertFalse(vl0.isValid())

def testNotify(self):
vl0 = QgsVectorLayer(self.dbconn + ' sslmode=disable key=\'pk\' srid=4326 type=POLYGON table="qgis_test"."some_poly_data" (geom) sql=', 'test', 'postgres')
vl0.dataProvider().setListening(True)
Expand Down
38 changes: 38 additions & 0 deletions tests/testdata/provider/testdata_pg.sql
Expand Up @@ -535,3 +535,41 @@ INSERT INTO qgis_test.check_constraints VALUES (
4, -- a
3 -- b
)


---------------------------------------------
--
-- Table and view for tests on checkPrimaryKeyUnicity
--


DROP TABLE IF EXISTS qgis_test.b21839_pk_unicity CASCADE;
CREATE TABLE qgis_test.b21839_pk_unicity
(
pk serial NOT NULL,
an_int integer NOT NULL,
geom geometry(Point),
CONSTRAINT b21839_pk_unicity_pkey PRIMARY KEY (pk)
)
WITH (
OIDS=FALSE
);


INSERT INTO qgis_test.b21839_pk_unicity(
pk, an_int, geom)
VALUES (1, 1, ST_GeomFromText('point( 1 1)'));


INSERT INTO qgis_test.b21839_pk_unicity(
pk, an_int, geom)
VALUES (2, 1, ST_GeomFromText('point( 1 3)'));



CREATE OR REPLACE VIEW qgis_test.b21839_pk_unicity_view AS
SELECT b21839_pk_unicity.pk,
b21839_pk_unicity.an_int,
b21839_pk_unicity.geom
FROM qgis_test.b21839_pk_unicity;

0 comments on commit 38c91e4

Please sign in to comment.