Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PsqlDosBackend: Fix changes not persisted after iterall and iterdict #6134

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 22, 2023

Fixes #6133
Fixes #6009

The iterall and iterdict generators of the QueryBuilder
implementation for the PsqlDosBackend would open a transaction in
order for the ModelWrapper to not automatically commit the session
upon any mutation as that would invalidate the cursor. However, it did
not manually commit the session at the end of the iterator, causing any
mutations to be lost when the storage backend was reloaded.

This problem was not just present in the iterall and iterdict
methods of the QueryBuilder but rather the transaction method of the
PsqlDosBackend never commits the savepoint that is returned by the
Session.begin_nested call. Now the transaction explicitly commits
the savepoint after the yield and the QueryBuilder methods are updated
to simply use the transaction method of the storage backend, rather
than going directly to the session.

This change also required a change in the SqliteZipBackend, since the
transaction is now called during archive creation and import, but the
backend raised a NotImplementedError. This is because it used to be a
read-only backend, however, this limitation was recently lifted. The
commit simply forgot to implement the transaction method.

@sphuber sphuber requested review from eimrek and unkcpz September 22, 2023 19:38
@sphuber
Copy link
Contributor Author

sphuber commented Sep 22, 2023

@eimrek This fixed the problem for my testing. Could you also give this branch a go for your environment please? Thanks!

Edit: scratch that for the time being. There are a bunch of tests failing, so I clearly need to fix those first.

@eimrek
Copy link
Member

eimrek commented Sep 24, 2023

@sphuber sure, will gladly give this a shot. feel free to ping me when ready

@sphuber sphuber force-pushed the fix/6133/querybuilder-iterall-persistence branch from 0863aab to 23a7e9b Compare September 24, 2023 19:43
@sphuber
Copy link
Contributor Author

sphuber commented Sep 24, 2023

Ok @eimrek I fixed the problem, should be good to go for a test now.

eimrek
eimrek previously approved these changes Sep 25, 2023
Copy link
Member

@eimrek eimrek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well on my end! thanks 👍

…dict`

The `iterall` and `iterdict` generators of the `QueryBuilder`
implementation for the `PsqlDosBackend` would open a transaction in
order for the `ModelWrapper` to not automatically commit the session
upon any mutation as that would invalidate the cursor. However, it did
not manually commit the session at the end of the iterator, causing any
mutations to be lost when the storage backend was reloaded.

This problem was not just present in the `iterall` and `iterdict`
methods of the `QueryBuilder` but rather the `transaction` method of the
`PsqlDosBackend` never commits the savepoint that is returned by the
`Session.begin_nested` call. Now the `transaction` explicitly commits
the savepoint after the yield and the `QueryBuilder` methods are updated
to simply use the `transaction` method of the storage backend, rather
than going directly to the session.

This change also required a change in the `SqliteZipBackend`, since the
`transaction` is now called during archive creation and import, but the
backend raised a `NotImplementedError`. This is because it used to be a
read-only backend, however, this limitation was recently lifted. The
commit simply forgot to implement the `transaction` method.
@sphuber sphuber force-pushed the fix/6133/querybuilder-iterall-persistence branch from 7a77f1f to 51c9032 Compare October 3, 2023 07:34
@sphuber sphuber merged commit 2ea5087 into aiidateam:main Oct 3, 2023
@sphuber sphuber deleted the fix/6133/querybuilder-iterall-persistence branch October 3, 2023 08:01
@edan-bainglass
Copy link
Member

edan-bainglass commented Dec 24, 2023

New test_iterall_persistence test passing on my Windows machine (WSL2) but raises the following:

--- Logging error ---
Traceback (most recent call last):
  File "/home/edanb/PSI/AiiDA/aiida-core/.tox/py39/lib/python3.9/site-packages/sqlalchemy/pool/base.py", line 763, in _finalize_fairy
    fairy._reset(pool, transaction_was_reset)
  File "/home/edanb/PSI/AiiDA/aiida-core/.tox/py39/lib/python3.9/site-packages/sqlalchemy/pool/base.py", line 1038, in _reset
    pool._dialect.do_rollback(self)
  File "/home/edanb/PSI/AiiDA/aiida-core/.tox/py39/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 683, in do_rollback
    dbapi_connection.rollback()
psycopg2.OperationalError: server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.


During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/edanb/miniconda3/envs/pr-qb/lib/python3.9/logging/__init__.py", line 1086, in emit
    stream.write(msg + self.terminator)
ValueError: I/O operation on closed file.
Call stack:
  File "/home/edanb/PSI/AiiDA/aiida-core/.tox/py39/lib/python3.9/site-packages/sqlalchemy/pool/base.py", line 509, in <lambda>
    and _finalize_fairy(
  File "/home/edanb/PSI/AiiDA/aiida-core/.tox/py39/lib/python3.9/site-packages/sqlalchemy/pool/base.py", line 791, in _finalize_fairy
    pool.logger.error(
Message: 'Exception during reset or similar'
Arguments: ()

Issue persists in v2.5.0. @sphuber comments?

@sphuber
Copy link
Contributor Author

sphuber commented Jan 3, 2024

Is this just in the teardown? I think I may have seen this at some point but only at the end of the tests. Don't think it is significant in any case for the time being as it has to do with the test shutdown

@edan-bainglass
Copy link
Member

edan-bainglass commented Jan 3, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants