DBManager PostgreSQL backend using core APIs instead of psycopg2

[strk]

The DBManager's SQL window gets stuck when the connection to the (PostgreSQL) database is interrupted (for example on backend restart). This PR is aimed at providing a solution, either as automatic-reconnect or user-requested reconnect action.

See #31994

[elpaso]

@strk please keep in mind that the roadmap is to eventually get rid of all provider specific connectors implemented in python and use the QgsAbstractDatabaseProviderConnection C++ implementation for all DB manager operations. This is not yet possible because the QgsAbstractDatabaseProviderConnection is not complete and because not all providers implement it but any PR that will make this switch more difficult should not be accepted.

So, just to remind you that the connection (low level DB connection) status should be IMHO handled in the provider and not in this plugin and not in QGIS core either, what I think could be an acceptable approach is for the QgsAbstractDatabaseProviderConnection to throw a specific exception in case of (low-level) connection errors and let client code act on those.

@nyalldawson any opinion on this?

[nyalldawson]

@elpaso agreed!

[strk]

You mean we should stop using PsycoPg as a whole ?

[elpaso]

You mean we should stop using PsycoPg as a whole ?

Yes. This topic has been discussed a lot, and the ultimate goal is port DB manager to C++ (or better, drop it altogether and replace with the C++ implementation we are already using in the QGIS browser panel), of course this won't happen overnight until we do not reach feature parity but at least let's try to make the steps in the right direction.

The fact the we (you included) are spending a large share of our time (and the donor's money) fixing and patching a constantly broken DB manager is a living proof of the rightfulness of the decision, a great part of the functionality is now available, well tested and well maintained in C++.

[strk]

Ok, thanks for the heads up.
I'm trying to see how to replace psycopg2 with calls to https://qgis.org/api/classQgsAbstractDatabaseProviderConnection.html

@Elpas It looks like you did some work in that direction for the gpkg dbplugin so is it a good idea to try at following that path ? One thing I see is that the "model" of the DBManager DBPlugin is to have a concept of a "cursor", which is what you call .execute() on (and .close). This concept is not present in the abstract connection provider, just a plan single-run executeSQL (no transaction?).

[elpaso]

Ok, thanks for the heads up.
I'm trying to see how to replace psycopg2 with calls to https://qgis.org/api/classQgsAbstractDatabaseProviderConnection.html

@Elpas It looks like you did some work in that direction for the gpkg dbplugin so is it a good idea to try at following that path ?

Yes, that was just an initial migration to the new API in order to test its concepts but you can certainly start from there.

One thing I see is that the "model" of the DBManager DBPlugin is to have a concept of a "cursor", which is what you call .execute() on (and .close). This concept is not present in the abstract connection provider, just a plan single-run executeSQL (no transaction?).

Yeah, transactions are missing, we briefly thought about that and we decided it wasn't a priority for the class scope.

Do you see a solid use case for that?

[strk]

I don't yet see a solid use case, other than migrating incrementally (users might be already using that cursor concept). Is actually not just transactions, but also partial fetches from a cursor (_fetchone) that are missing. Maybe those could be implemented by creating an actual cursor in the database by subclasses...

[elpaso]

I don't yet see a solid use case, other than migrating incrementally (users might be already using that cursor concept). Is actually not just transactions, but also partial fetches from a cursor (_fetchone) that are missing. Maybe those could be implemented by creating an actual cursor in the database by subclasses...

fetchone is just a normal execSql where you just pick the first entry from the first row, no need to implement that.

[strk]

it still needs to be implemented, in DBConnector subclasses, unless DBConnector base class as a whole implements that (and drops the "cursor" concept)

[strk]

I hate python. The ability to use any member/method drives to such a mess!
The problem is that DBConnector, which is supposedly an abstract class, is using psycopg2 specific models, like:

    def _execute(self, cursor, sql):
        if cursor is None:
            cursor = self._get_cursor()
        try:
            cursor.execute(sql)

The PostgreSQL specific subclass probably used to have that kind of code, but it was removed at some point in time:

    # moved into the parent class: DbConnector._execute()
    # def _execute(self, cursor, sql):
    #       pass

Following git history it looks like such "move" was already present in the very first inclusion of code in core, as it is found in 3c5b3bb (2012 commit from @brushtyler )

[nyalldawson]

I hate python. The ability to use any member/method drives to such a mess!

That's exactly why the plan is to move all this code outside of python. We had the same issue with processing too.

Python is just a bad choice for large, complex projects (unless you soak them with near 100% test coverage, including all GUI functionality)

[strk]

@elpaso another thing missing in the new API is obtaining the names of returned columns, for a generic query executor (as needed by the SQL window of DBManager). In psycopg2 terms this is cursor's description method: http://initd.org/psycopg/docs/cursor.html

The executeSQL method of the new API only rerutns QList< QList< QVariant > > which do not include names and types of returned values

[elpaso]

True, any idea? We could add an argument to return column names in the first row?

[strk]

True, any idea? We could add an argument to return column names in the first row?

it's not just names, here's the list of currently expected information:

• name
• type_code
• display_size
• internal_size
• precision
• scale
• null_ok

[strk]

I've faked those records for the moment, pretending all fields are strings with a length of 10 (both internal and display). It seems to work. SQL Window can query the database, even upon restart of backend. This is as of 8d4469901327dcb9bedce6c708ebd7046dcb3921. I'm sure there are lots of places that are broken with this change though, so it'll take some time to adapt more users (for sure topoviewer will need adaptation)

[strk]

Getting name in first row could be good enough for a start, but I wonder if we really need cursors

[elpaso]

How about a getTableInformation?

[strk]

How about a getTableInformation?

It's not only about table, but about a query result. Could be a complex select. In most cases it'd take running the query, or at least _preparing_ it, or defining a cursor, in order to obtain that information, so a separate function might be problematic from a performance point of view, unless the separate function returns some handle to subsequently fetch rows from the thing (that's what a "cursor" is in psycopg2).

[elpaso]

I would say that if this is a PG specific implementation it's probably acceptable to have a custom implementation in the PG plugin connector.

But I think that getting column information from a query is a valid use case for the abstract connection API, always thinking about all supported DB backends, not limited to PG.

[elpaso]

Do you need to import this? It's abstract, you get the concrete instance out of the factory.

[strk]

I removed the import completely, with c01e34bf12

[strk]

Is there a python function to map the QVariant resultset values into typed python variables ?

[elpaso]

Is there a python function to map the QVariant resultset values into typed python variables ?

You don't need it: SIP automatically takes care of the conversion, if it doesn't then you must write native conversion code like we've done in code for many Qgs types.

[strk]

It looks like I'm getting some QVariant typed values in the resultset. Debugging further to find out which ones, could be just some missing conversion we have to add. Probably for NULL ?

XXX col of rec of resultset valued NULL is typed <class 'PyQt5.QtCore.QVariant'>
TypeError: setText(self, str): argument 1 has unexpected type 'QVariant'

[elpaso]

I'm missing the context :) But there is some conversion code from DB returned values in several places, take a look to the provider implementation of execSql (possibly the private implementation)

[strk]

I confirm my problem was with NULL. By adding special code in my CursorProxy to deal with NULL (converting it to None) fixes my case. I guess this should be done at lower levels, will try to find the culprit.

[strk]

@elpaso see 73f22499e8ebfa0a22dab213831ff9b57de91ee6 for context.
Basically the value returned by QgsAbstractDatabaseProviderConnection.executeSql() (PostgreSQL impmlementation, in this case) contains some values that are typed "QVariant", and seem to be NULL values. The actual database-side datatype of that NULL I still don't know, because that info, as we said, does not get down out of executeSQL

[elpaso]

@strk maybe I forgot to check for QVariant NULL here? https://github.com/qgis/QGIS/blob/master/src/providers/postgres/qgspostgresproviderconnection.cpp#L246

[strk]

Thanks @m-kuhn , I've sent an email to qgis-dev

[strk]

Ok, so as anticipated, creating a QgsVectorLayer makes operations far slower as a new layer has to be created for each and every query being executed, including "service" ones. For instance, this I have in the logs, with current state of this PR:

XXX CursorAdapter[0x7f59bb3f26a0]: execute called with sql SELECT has_database_privilege(current_database(), 'CREATE'), has_database_privilege(current_database(), 'TEMP')
XXX CursorAdapter[0x7f59bb3f26a0]: execute returned 1 rows
src/providers/postgres/qgspostgresprovider.cpp:100 : (QgsPostgresProvider) [0ms] URI: port=5432 key='__rid__' checkPrimaryKeyUnicity='1' table="(SELECT row_number() OVER () AS __rid__, * FROM (SELECT has_database_privilege(current_database(), 'CREATE'), has_database_privilege(current_database(), 'TEMP')) as foo)" 

Basically, some code part of DBManager wants to check for a capability, and runs a query, the query executor creates a VectorLayer to find out names of fields, which triggers lots of other queries.

[strk]

NOTE: lazily fetching field names when needed might not be possible as current code does things like:
return [x[0] for x in c.description] with c being the Cursor, so unless Python allows me to deifine a getter I won't be able to leave the current calling code untouched.

[roya0045]

NOTE: lazily fetching field names when needed might not be possible as current code does things like:
return [x[0] for x in c.description] with c being the Cursor, so unless Python allows me to deifine a getter I won't be able to leave the current calling code untouched.

Do you mean a generator (using 'yield' ) ?

[strk]

Thanks @roya0045 for the suggestion but I found a simpler way (@property). Now speed seems much better :)

[strk]

I think you can get all this information from the QgsField instance instead of hardcoding.

@elpaso with 8ca08ec I've got precision and lenght information from the QgsField instance, but I'm not sure about "displaySize" vs. "internalSize" (I guess displaySize should be left hard-coded or somehow lower?) and I dunno how to extract a mapping between the data type returned by QgsField::type and the python type name...

[strk]

Well, I've used the .type() method and didn't get errors so I think we're fine to go.

The only missing piece here would be the chunk-based access, but now I'm not sure it is worth the trouble because I could confirm that creating a VectorLayer for each and every query is really killer for performance.

@palmerj what do you think ? Is the SQL Window of DBManager used for inspecting data in big tables ?

[palmerj]

Is the SQL Window of DBManager used for inspecting data in big tables ?

How do you mean? Showing many rows in the grid or running a query against a large table (hence the longer time for setup if using QgsVectorLayer)? We often do both (or would like to)

[strk]

Showing many rows in the grid or running a query against a large table (hence the longer time for setup if using QgsVectorLayer)? We often do both (or would like to)

I suspect running a query against a large table could still be fast (QgsVectorLayer supposedly fetching rows only on demand, and current code not fetching rows at all using it). But the current code also runs the query via the abstract connection new core API which returns all rows at once.

[elpaso]

LGTM