Mentat

This issue stems from #4251, where a workaround for a specific transaction handling bug was found and recommended. Based on the videocall discussion a more streamlined approach is required, possibly by managing the transaction lifetime by limiting the life of a Session object.
Following the official documentation [1], the upcoming version of SQLAlchemy (v1.4), currently in beta, includes a Python's context manager based workflow with Session as a transaction manager and Engine as some form of a connection manager.
Based on [1], the best approach would be to keep a reference to Engine, instantiate a Session from it using a context manager and perform the data-focused actions inside that context. This is perhaps better documented by a code excerpt from [1]:

# create session and add objects
with Session(engine) as session, session.begin():
    session.add(some_object)
    session.add(some_other_object)
# inner context calls session.commit(), if there were no exceptions
# outer context calls session.close()

The current version 1.3 does not support the context manager approach while supporting the Engine/Session distinction, so a similar solution is possible [2], but the user (programmer) must both keep a reference to sessionmaker and carefully manage the lifetime of the Session object, what is error prone. It should nevertheless be a feasible approach.

Waiting for the next version might make sense here, although it is of note that the minimum required version of Python is bumped to 3.6 there [3] (not an issue for us as we are currently using 3.7.3, but might cause problems elsewhere). The migration document also promises various performance improvements in the next version.

The required changes to the code are non-trivial for both versions and the migration to v1.4 also seems to require some rewrites. I advise to do both at the same time and postpone this issue until after the SQLAlchemy v1.4 release.

[1] https://docs.sqlalchemy.org/en/14/orm/session_basics.html
[2] https://docs.sqlalchemy.org/en/13/orm/session_basics.html
[3] https://docs.sqlalchemy.org/en/14/changelog/migration_14.html

As for the flask-sqlalchemy, it is just a simple adaptor [1] and seems to take the sessionmaker route [2]. It also seems to manage the lifetime of the session subobject. Although it is not obvious from the example code [3] (why would an object's attribute lifetime be different from the object itself?), the documentation refers to the session as scoped and declares [4]:

You have to commit the session, but you don’t have to remove it at the end of the request, Flask-SQLAlchemy does that for you.

So the way of flask-sqlalchemy should be a viable approach to correctly access the database and if not, it is a bug somewhere in the flask-sqlalchemy\SQLAlchemy\psycopg2 stack and should be treated as such.

This is all based on code review and study of the documentation, not tested hard evidence.

[1] https://github.com/pallets/flask-sqlalchemy/blob/master/src/flask_sqlalchemy/model.py
[2] https://flask-sqlalchemy.palletsprojects.com/en/2.x/api/#flask_sqlalchemy.SQLAlchemy
[3] https://flask-sqlalchemy.palletsprojects.com/en/2.x/quickstart/#a-minimal-application
[4] https://flask-sqlalchemy.palletsprojects.com/en/2.x/quickstart/#road-to-enlightenment

Consensus on 2021-01-11 meeting was that the correct way most probably is switching to flask-sqlalchemy, which hides some sqlalchemy idiosyncracies. We would than be able to remove weird rollback introduced in #4251.

Rajmund Hruska wrote in #note-11:

From my understanding, I used the flask_sqlalchemy in the same way as in the example posted in #note-7 so it should be working.

Not quite. You need to signal an end of transaction to the ORM somehow. In #note-7, it is done by calling db.session.commit(). In whois.py, after being done with the loop of queries (somewhere about where previously the storage.session.rollback() was), the storage.session.commit() method should be called. That should close the running transaction, without it, the transaction will remain open. The symptoms are the same as for #4251, but the cause is not a deficiency in the new ORM (as was with the prior one), but in the way it is used, see #note-6 and original the source [4,#note-6] for more details.

I did not run any code now, so take all this with a grain of salt. Is my reasoning understandable?

As was discussed on the meeting, with regards also to #4569, access to SQLAlchemy's internals is needed even in the flask_sqlalchemy. Based on the documentation [1], this might work:

from flask_sqlalchemy import BaseQuery

[1] https://flask-sqlalchemy.palletsprojects.com/en/2.x/customizing/#query-class

Rajmund Hruska wrote in #note-14:

I can't fully remove sqlalchemy module because sqlalchemy.exc.OperationalError is used and exc module is not present in the flask_sqlalchemy module.

I have found:

flask_sqlalchemy.orm.exc.sa_exc.OperationalError

Not thoroughly tested.

Also, there seems to be proper support for model definition:
https://flask-sqlalchemy.palletsprojects.com/en/2.x/models/

Lastly, I would advise to redo the patches from scratch, the commit history is getting hard to understand.

Radko Krkoš wrote in #note-15:

Rajmund Hruska wrote in #note-14:

I can't fully remove sqlalchemy module because sqlalchemy.exc.OperationalError is used and exc module is not present in the flask_sqlalchemy module.

I have found:

[...]

Not thoroughly tested.

I tried running the following code:

import flask_sqlalchemy
import sqlalchemy

def func():
    raise sqlalchemy.exc.OperationalError('Test', orig=None, params=None)

def func2():
    try:
        func()
    except flask_sqlalchemy.orm.exc.sa_exc.OperationalError:
        print('caught')

func2()

Also, there seems to be proper support for model definition:
https://flask-sqlalchemy.palletsprojects.com/en/2.x/models/

I will look into that in more detail.

Lastly, I would advise to redo the patches from scratch, the commit history is getting hard to understand.

I will create a new branch.

I checked the eventstorage.py service, which uses Psycopg2. I don't have any experience with Psycopg2, I only read the official documentation. From my point of view, the Psycopg2 is used correctly and I wouldn't change anything there.

Rajmund Hruska wrote in #note-17:

I checked the eventstorage.py service, which uses Psycopg2. I don't have any experience with Psycopg2, I only read the official documentation. From my point of view, the Psycopg2 is used correctly and I wouldn't change anything there.

Yes, this is about an idiosyncrasy of the SQLAlchemy ORM. With psycopg2, the transactions are managed differently (manually), so there should be no gotchas there.

Radko Krkoš wrote in #note-18:

Rajmund Hruska wrote in #note-17:

I checked the eventstorage.py service, which uses Psycopg2. I don't have any experience with Psycopg2, I only read the official documentation. From my point of view, the Psycopg2 is used correctly and I wouldn't change anything there.

Yes, this is about an idiosyncrasy of the SQLAlchemy ORM. With psycopg2, the transactions are managed differently (manually), so there should be no gotchas there.

I thought so but I checked psycopg2 because it was mentioned in the title and description of the issue. Anyway, at least I understand it better now

Rajmund Hruska wrote in #note-21:

So, in ed1b8f23 I started using flask_sqlalchemy for sessions in lib/mentat. In lib/hawat the flask_sqlalchemy is already used. And lib/vial will be merged into lib/hawat. I guess the issue is resolved now, isn't it?

From a quick glance, not all issues from the previous review have been dealt with. At least I can see the sqlalchemy.exc.OperationalError one unaddressed.
In addition, I do not like the inconsistency of going to full path in code on lines 31 & 44 (after diff) while going in the other direction on lines 32 & 53. Why?

Radko Krkoš wrote in #note-22:

Rajmund Hruska wrote in #note-21:

So, in ed1b8f23 I started using flask_sqlalchemy for sessions in lib/mentat. In lib/hawat the flask_sqlalchemy is already used. And lib/vial will be merged into lib/hawat. I guess the issue is resolved now, isn't it?

From a quick glance, not all issues from the previous review have been dealt with. At least I can see the sqlalchemy.exc.OperationalError one unaddressed.

I think we talked about that in one of the meetings but I don't remember the reason why I didn't the OperationalError from flask_sqlalchemy. I guess I can use the other one, as it seems to be working anyway (#note-16).

In addition, I do not like the inconsistency of going to full path in code on lines 31 & 44 (after diff) while going in the other direction on lines 32 & 53. Why?

There wasn't really any particular reason at all it was just how it was convenient at the time. I only needed the OperationalError in lines 32 & 53 and not full sqlalchemy as it was before diff. And I needed multiple objects from flask_sqlalchemy (lines 76, 78).

Rajmund Hruska wrote in #note-23:

Radko Krkoš wrote in #note-22:

Rajmund Hruska wrote in #note-21:

So, in ed1b8f23 I started using flask_sqlalchemy for sessions in lib/mentat. In lib/hawat the flask_sqlalchemy is already used. And lib/vial will be merged into lib/hawat. I guess the issue is resolved now, isn't it?

From a quick glance, not all issues from the previous review have been dealt with. At least I can see the sqlalchemy.exc.OperationalError one unaddressed.

I think we talked about that in one of the meetings but I don't remember the reason why I didn't the OperationalError from flask_sqlalchemy. I guess I can use the other one, as it seems to be working anyway (#note-16).

In addition, I do not like the inconsistency of going to full path in code on lines 31 & 44 (after diff) while going in the other direction on lines 32 & 53. Why?

There wasn't really any particular reason at all it was just how it was convenient at the time. I only needed the OperationalError in lines 32 & 53 and not full sqlalchemy as it was before diff. And I needed multiple objects from flask_sqlalchemy (lines 76, 78).

OK.

As the most important aim of this task was to get rid transaction dancing kludge to sidestep bare SQLAlchemy idiosyncracies, I guess we're done here - time for polishing and merging.

There are still cases of questionable *SQLAlchemy usage, I've created another ticket for long winter evenings: #7545.