Add feedback to executeSql

Fixes #38092 by adding an optional QgsFeedback argument to the executeSql method and by implementing the PQCancel method in the PG provider internals. While the cancellation works well for all supported provider while fetching results in the loop, the cancellation of a running query is now implemented for the postgres provider connection only because the GPKG and GDAL both rely on GDALDatasetExecuteSQL which cannot be interrupted. This PR also introduce a few optimizations in the PG DB-Manager code that should probably fix also other "slowness" issues that were reported after 3.x during PG query execution. A small UX change in th SQL dialog makes it evident to the user that a cancellation request has been sent to the backend: the button text is changed to "Cancellation requested, please wait..." so that for provider connections that are not able to interrupt the running query and must wait for the fetching loop to exit from the exeuteSql call the user knows that something is happening and that a cancellation request has been successfully sent.
qgis · Sep 19, 2020 · d54c310 · d54c310
1 parent 73626bd
commit d54c310
Show file tree

Hide file tree

Showing 19 changed files with 210 additions and 34 deletions.
diff --git a/python/core/auto_generated/qgsabstractdatabaseproviderconnection.sip.in b/python/core/auto_generated/qgsabstractdatabaseproviderconnection.sip.in
@@ -8,6 +8,7 @@
 
 
 
+
 class QgsAbstractDatabaseProviderConnection : QgsAbstractProviderConnection
 {
 %Docstring
@@ -427,9 +428,9 @@ Raises a QgsProviderConnectionException if any errors are encountered.
 :raises :: py:class:`QgsProviderConnectionException`
 %End
 
-    virtual QList<QList<QVariant>> executeSql( const QString &sql ) const throw( QgsProviderConnectionException );
+    virtual QList<QList<QVariant>> executeSql( const QString &sql, QgsFeedback *feedback = 0 ) const throw( QgsProviderConnectionException );
 %Docstring
-Executes raw ``sql`` and returns the (possibly empty) list of results in a multi-dimensional array.
+Executes raw ``sql`` and returns the (possibly empty) list of results in a multi-dimensional array, optionally ``feedback`` can be provided.
 Raises a QgsProviderConnectionException if any errors are encountered.
 
 :raises :: py:class:`QgsProviderConnectionException`

diff --git a/python/plugins/db_manager/db_plugins/postgis/connector.py b/python/plugins/db_manager/db_plugins/postgis/connector.py
@@ -44,6 +44,7 @@
     QgsDataSourceUri,
     QgsProviderRegistry,
     QgsProviderConnectionException,
+    QgsFeedback,
 )
 
 from ..connector import DBConnector
@@ -68,12 +69,13 @@ def _debug(self, msg):
         pass
         # print("XXX CursorAdapter[" + hex(id(self)) + "]: " + msg)
 
-    def __init__(self, connection, sql=None):
+    def __init__(self, connection, sql=None, feedback=None):
         self._debug("Created with sql: " + str(sql))
         self.connection = connection
         self.sql = sql
         self.result = None
         self.cursor = 0
+        self.feedback = feedback
         self.closed = False
         if (self.sql is not None):
             self._execute()
@@ -103,7 +105,7 @@ def _execute(self, sql=None):
             return
         self._debug("execute called with sql " + self.sql)
         try:
-            self.result = self._toStrResultSet(self.connection.executeSql(self.sql))
+            self.result = self._toStrResultSet(self.connection.executeSql(self.sql, feedback=self.feedback))
         except QgsProviderConnectionException as e:
             raise DbError(e, self.sql)
         self._debug("execute returned " + str(len(self.result)) + " rows")
@@ -119,7 +121,7 @@ def description(self):
             uri = QgsDataSourceUri(self.connection.uri())
 
             # TODO: make this part provider-agnostic
-            uri.setTable('(SELECT row_number() OVER () AS __rid__, * FROM (' + self.sql + ') as foo)')
+            uri.setTable('(SELECT row_number() OVER () AS __rid__, * FROM (' + self.sql + ' LIMIT 1) as foo)')
             uri.setKeyColumn('__rid__')
             # TODO: fetch provider name from connection (QgsAbstractConnectionProvider)
             # TODO: re-use the VectorLayer for fetching rows in batch mode
@@ -228,6 +230,8 @@ def __init__(self, uri, connection):
         self._checkGeometryColumnsTable()
         self._checkRasterColumnsTable()
 
+        self.feedback = None
+
     def _connectionInfo(self):
         return str(self.uri().connectionInfo(True))
 
@@ -306,6 +310,8 @@ def _checkRasterColumnsTable(self):
     def cancel(self):
         if self.connection:
             self.connection.cancel()
+        if self.core_connection:
+            self.feedback.cancel()
 
     def getInfo(self):
         c = self._execute(None, u"SELECT version()")
@@ -1166,7 +1172,8 @@ def _execute(self, cursor, sql):
         if cursor is not None:
             cursor._execute(sql)
             return cursor
-        return CursorAdapter(self.core_connection, sql)
+        self.feedback = QgsFeedback()
+        return CursorAdapter(self.core_connection, sql, feedback=self.feedback)
 
     def _executeSql(self, sql):
         return self.core_connection.executeSql(sql)

diff --git a/python/plugins/db_manager/dlg_sql_window.py b/python/plugins/db_manager/dlg_sql_window.py
@@ -381,6 +381,7 @@ def updateUiWhileSqlExecution(self, status):
 
     def executeSqlCanceled(self):
         self.btnCancel.setEnabled(False)
+        self.btnCancel.setText(self.tr("Cancelling, please wait ..."))
         self.modelAsync.cancel()
 
     def executeSqlCompleted(self):
@@ -406,6 +407,8 @@ def executeSqlCompleted(self):
                 self.uniqueModel.clear()
                 self.geomCombo.clear()
 
+            self.btnCancel.setText(self.tr("Cancel"))
+
     def executeSql(self):
         sql = self._getExecutableSqlQuery()
         if sql == "":

diff --git a/src/core/providers/ogr/qgsgeopackageproviderconnection.cpp b/src/core/providers/ogr/qgsgeopackageproviderconnection.cpp
@@ -164,10 +164,10 @@ void QgsGeoPackageProviderConnection::renameVectorTable( const QString &schema,
   }
 }
 
-QList<QList<QVariant>> QgsGeoPackageProviderConnection::executeSql( const QString &sql ) const
+QList<QList<QVariant>> QgsGeoPackageProviderConnection::executeSql( const QString &sql, QgsFeedback *feedback ) const
 {
   checkCapability( Capability::ExecuteSql );
-  return executeGdalSqlPrivate( sql );
+  return executeGdalSqlPrivate( sql, feedback );
 }
 
 void QgsGeoPackageProviderConnection::vacuum( const QString &schema, const QString &name ) const
@@ -354,26 +354,46 @@ void QgsGeoPackageProviderConnection::setDefaultCapabilities()
   };
 }
 
-QList<QVariantList> QgsGeoPackageProviderConnection::executeGdalSqlPrivate( const QString &sql ) const
+QList<QVariantList> QgsGeoPackageProviderConnection::executeGdalSqlPrivate( const QString &sql, QgsFeedback *feedback ) const
 {
-  QString errCause;
   QList<QVariantList> results;
+
+  if ( feedback && feedback->isCanceled() )
+  {
+    return results;
+  }
+
+  QString errCause;
   gdal::ogr_datasource_unique_ptr hDS( GDALOpenEx( uri().toUtf8().constData(), GDAL_OF_VECTOR | GDAL_OF_UPDATE, nullptr, nullptr, nullptr ) );
   if ( hDS )
   {
+
+    if ( feedback && feedback->isCanceled() )
+    {
+      return results;
+    }
+
     OGRLayerH ogrLayer( GDALDatasetExecuteSQL( hDS.get(), sql.toUtf8().constData(), nullptr, nullptr ) );
     if ( ogrLayer )
     {
       gdal::ogr_feature_unique_ptr fet;
       QgsFields fields;
       while ( fet.reset( OGR_L_GetNextFeature( ogrLayer ) ), fet )
       {
+
+        if ( feedback && feedback->isCanceled() )
+        {
+          break;
+        }
+
         QVariantList row;
+
         // Try to get the right type for the returned values
         if ( fields.isEmpty() )
         {
           fields = QgsOgrUtils::readOgrFields( fet.get(), QTextCodec::codecForName( "UTF-8" ) );
         }
+
         if ( ! fields.isEmpty() )
         {
           QgsFeature f { QgsOgrUtils::readOgrFeature( fet.get(), fields, QTextCodec::codecForName( "UTF-8" ) ) };

diff --git a/src/core/providers/ogr/qgsgeopackageproviderconnection.h b/src/core/providers/ogr/qgsgeopackageproviderconnection.h
@@ -38,7 +38,7 @@ class QgsGeoPackageProviderConnection : public QgsAbstractDatabaseProviderConnec
     void dropVectorTable( const QString &schema, const QString &name ) const override;
     void dropRasterTable( const QString &schema, const QString &name ) const override;
     void renameVectorTable( const QString &schema, const QString &name, const QString &newName ) const override;
-    QList<QList<QVariant>> executeSql( const QString &sql ) const override;
+    QList<QList<QVariant>> executeSql( const QString &sql, QgsFeedback *feedback = nullptr ) const override;
     void vacuum( const QString &schema, const QString &name ) const override;
     void createSpatialIndex( const QString &schema, const QString &name, const QgsAbstractDatabaseProviderConnection::SpatialIndexOptions &options = QgsAbstractDatabaseProviderConnection::SpatialIndexOptions() ) const override;
     bool spatialIndexExists( const QString &schema, const QString &name, const QString &geometryColumn ) const override;
@@ -52,7 +52,7 @@ class QgsGeoPackageProviderConnection : public QgsAbstractDatabaseProviderConnec
 
     void setDefaultCapabilities();
     //! Use GDAL to execute SQL
-    QList<QVariantList> executeGdalSqlPrivate( const QString &sql ) const;
+    QList<QVariantList> executeGdalSqlPrivate( const QString &sql, QgsFeedback *feedback = nullptr ) const;
 
 };
 

diff --git a/src/core/qgsabstractdatabaseproviderconnection.cpp b/src/core/qgsabstractdatabaseproviderconnection.cpp
@@ -133,7 +133,7 @@ void QgsAbstractDatabaseProviderConnection::renameSchema( const QString &, const
   checkCapability( Capability::RenameSchema );
 }
 
-QList<QList<QVariant>> QgsAbstractDatabaseProviderConnection::executeSql( const QString & ) const
+QList<QList<QVariant>> QgsAbstractDatabaseProviderConnection::executeSql( const QString &, QgsFeedback * ) const
 {
   checkCapability( Capability::ExecuteSql );
   return QList<QList<QVariant>>();

diff --git a/src/core/qgsabstractdatabaseproviderconnection.h b/src/core/qgsabstractdatabaseproviderconnection.h
@@ -25,6 +25,8 @@
 
 #include <QObject>
 
+class QgsFeedback;
+
 /**
  * The QgsAbstractDatabaseProviderConnection class provides common functionality
  * for DB based connections.
@@ -452,11 +454,11 @@ class CORE_EXPORT QgsAbstractDatabaseProviderConnection : public QgsAbstractProv
     virtual void renameSchema( const QString &name, const QString &newName ) const SIP_THROW( QgsProviderConnectionException );
 
     /**
-     * Executes raw \a sql and returns the (possibly empty) list of results in a multi-dimensional array.
+     * Executes raw \a sql and returns the (possibly empty) list of results in a multi-dimensional array, optionally \a feedback can be provided.
      * Raises a QgsProviderConnectionException if any errors are encountered.
      * \throws QgsProviderConnectionException
      */
-    virtual QList<QList<QVariant>> executeSql( const QString &sql ) const SIP_THROW( QgsProviderConnectionException );
+    virtual QList<QList<QVariant>> executeSql( const QString &sql, QgsFeedback *feedback = nullptr ) const SIP_THROW( QgsProviderConnectionException );
 
     /**
      * Vacuum the database table with given \a schema and \a name (schema is ignored if not supported by the backend).

diff --git a/src/providers/mssql/qgsmssqlproviderconnection.cpp b/src/providers/mssql/qgsmssqlproviderconnection.cpp
@@ -213,16 +213,23 @@ void QgsMssqlProviderConnection::dropSchema( const QString &schemaName,  bool fo
                      .arg( QgsMssqlProvider::quotedIdentifier( schemaName ) ) );
 }
 
-QList<QVariantList> QgsMssqlProviderConnection::executeSql( const QString &sql ) const
+QList<QVariantList> QgsMssqlProviderConnection::executeSql( const QString &sql, QgsFeedback *feedback ) const
 {
   checkCapability( Capability::ExecuteSql );
-  return executeSqlPrivate( sql );
+  return executeSqlPrivate( sql, true, feedback );
 }
 
-QList<QVariantList> QgsMssqlProviderConnection::executeSqlPrivate( const QString &sql, bool resolveTypes ) const
+QList<QVariantList> QgsMssqlProviderConnection::executeSqlPrivate( const QString &sql, bool resolveTypes, QgsFeedback *feedback ) const
 {
-  const QgsDataSourceUri dsUri { uri() };
   QList<QVariantList> results;
+
+  if ( feedback && feedback->isCanceled() )
+  {
+    return results;
+  }
+
+  const QgsDataSourceUri dsUri { uri() };
+
   // connect to database
   QSqlDatabase db = QgsMssqlConnection::getDatabase( dsUri.service(), dsUri.host(), dsUri.database(), dsUri.username(), dsUri.password() );
 
@@ -234,6 +241,12 @@ QList<QVariantList> QgsMssqlProviderConnection::executeSqlPrivate( const QString
   }
   else
   {
+
+    if ( feedback && feedback->isCanceled() )
+    {
+      return results;
+    }
+
     //qDebug() << "MSSQL QUERY:" << sql;
     QSqlQuery q = QSqlQuery( db );
     q.setForwardOnly( true );
@@ -251,7 +264,7 @@ QList<QVariantList> QgsMssqlProviderConnection::executeSqlPrivate( const QString
     {
       const QSqlRecord rec { q.record() };
       const int numCols { rec.count() };
-      while ( q.next() )
+      while ( q.next() && ( ! feedback || ! feedback->isCanceled() ) )
       {
         QVariantList row;
         for ( int col = 0; col < numCols; ++col )

diff --git a/src/providers/mssql/qgsmssqlproviderconnection.h b/src/providers/mssql/qgsmssqlproviderconnection.h
@@ -41,7 +41,7 @@ class QgsMssqlProviderConnection : public QgsAbstractDatabaseProviderConnection
     void dropVectorTable( const QString &schema, const QString &name ) const override;
     void createSchema( const QString &name ) const override;
     void dropSchema( const QString &name, bool force = false ) const override;
-    QList<QVariantList> executeSql( const QString &sql ) const override;
+    QList<QVariantList> executeSql( const QString &sql, QgsFeedback *feedback ) const override;
     QList<QgsAbstractDatabaseProviderConnection::TableProperty> tables( const QString &schema,
         const TableFlags &flags = nullptr ) const override;
     QStringList schemas( ) const override;
@@ -52,7 +52,7 @@ class QgsMssqlProviderConnection : public QgsAbstractDatabaseProviderConnection
 
   private:
 
-    QList<QVariantList> executeSqlPrivate( const QString &sql, bool resolveTypes = true ) const;
+    QList<QVariantList> executeSqlPrivate( const QString &sql, bool resolveTypes = true, QgsFeedback *feedback = nullptr ) const;
     void setDefaultCapabilities();
     void dropTablePrivate( const QString &schema, const QString &name ) const;
     void renameTablePrivate( const QString &schema, const QString &name, const QString &newName ) const;

diff --git a/src/providers/postgres/qgspostgresconn.cpp b/src/providers/postgres/qgspostgresconn.cpp
@@ -1338,6 +1338,22 @@ PGresult *QgsPostgresConn::PQexec( const QString &query, bool logError, bool ret
 
 }
 
+int QgsPostgresConn::PQCancel()
+{
+  // No locker: this is supposed to be thread safe
+  int result = 0;
+  auto cancel = ::PQgetCancel( mConn ) ;
+  if ( cancel )
+  {
+    char errbuf[255];
+    result = ::PQcancel( cancel, errbuf, 255 );
+    if ( ! result )
+      QgsDebugMsgLevel( QStringLiteral( "Cancelling query error:" ).arg( errbuf ), 3 );
+  }
+  ::PQfreeCancel( cancel );
+  return result;
+}
+
 bool QgsPostgresConn::openCursor( const QString &cursorName, const QString &sql )
 {
   QMutexLocker locker( &mLock ); // to protect access to mOpenCursors

diff --git a/src/providers/postgres/qgspostgresconn.h b/src/providers/postgres/qgspostgresconn.h
@@ -250,6 +250,7 @@ class QgsPostgresConn : public QObject
 
     // run a query and check for errors, thread-safe
     PGresult *PQexec( const QString &query, bool logError = true, bool retry = true ) const;
+    int PQCancel();
     void PQfinish();
     QString PQerrorMessage() const;
     int PQstatus() const;

diff --git a/src/providers/postgres/qgspostgresproviderconnection.cpp b/src/providers/postgres/qgspostgresproviderconnection.cpp
@@ -192,24 +192,52 @@ void QgsPostgresProviderConnection::renameSchema( const QString &name, const QSt
                      .arg( QgsPostgresConn::quotedIdentifier( newName ) ) );
 }
 
-QList<QVariantList> QgsPostgresProviderConnection::executeSql( const QString &sql ) const
+QList<QVariantList> QgsPostgresProviderConnection::executeSql( const QString &sql, QgsFeedback *feedback ) const
 {
   checkCapability( Capability::ExecuteSql );
-  return executeSqlPrivate( sql );
+  return executeSqlPrivate( sql, true, feedback );
 }
 
-QList<QVariantList> QgsPostgresProviderConnection::executeSqlPrivate( const QString &sql, bool resolveTypes ) const
+QList<QVariantList> QgsPostgresProviderConnection::executeSqlPrivate( const QString &sql, bool resolveTypes, QgsFeedback *feedback ) const
 {
-  const QgsDataSourceUri dsUri { uri() };
   QList<QVariantList> results;
+
+  // Check feedback first!
+  if ( feedback && feedback->isCanceled() )
+  {
+    return results;
+  }
+
+  const QgsDataSourceUri dsUri { uri() };
   QgsPostgresConn *conn = QgsPostgresConnPool::instance()->acquireConnection( dsUri.connectionInfo( false ) );
   if ( !conn )
   {
     throw QgsProviderConnectionException( QObject::tr( "Connection failed: %1" ).arg( uri() ) );
   }
   else
   {
+
+    if ( feedback && feedback->isCanceled() )
+    {
+      return results;
+    }
+
+    // This is gross but I tried with both conn and a context QObject without success: the lambda is never called.
+    QMetaObject::Connection moConn;
+    if ( feedback )
+    {
+      moConn = QObject::connect( feedback, &QgsFeedback::canceled, [ &conn ]
+      {
+        conn->PQCancel();
+      } );
+    }
+
     QgsPostgresResult res( conn->PQexec( sql ) );
+    if ( feedback )
+    {
+      QObject::disconnect( moConn );
+    }
+
     QString errCause;
     if ( conn->PQstatus() != CONNECTION_OK || ! res.result() )
     {
@@ -236,6 +264,10 @@ QList<QVariantList> QgsPostgresProviderConnection::executeSqlPrivate( const QStr
       {
         for ( int rowIdx = 0; rowIdx < res.PQnfields(); rowIdx++ )
         {
+          if ( feedback && feedback->isCanceled() )
+          {
+            break;
+          }
           const Oid oid { res.PQftype( rowIdx ) };
           QList<QVariantList> typeRes { executeSqlPrivate( QStringLiteral( "SELECT typname FROM pg_type WHERE oid = %1" ).arg( oid ), false ) };
           // Set the default to string
@@ -292,6 +324,10 @@ QList<QVariantList> QgsPostgresProviderConnection::executeSqlPrivate( const QStr
       }
       for ( int rowIdx = 0; rowIdx < res.PQntuples(); rowIdx++ )
       {
+        if ( feedback && feedback->isCanceled() )
+        {
+          break;
+        }
         QVariantList row;
         for ( int colIdx = 0; colIdx < res.PQnfields(); colIdx++ )
         {