The transaction block doesn't check the connection before attempting to roll back

[dvarrazzo]

The connection block is more well behaved:

>>> with psycopg.connect() as cnn:
...     cnn.execute("select 1")
...     cnn.close()
...     1/0
... 
Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
ZeroDivisionError: division by zero

The transaction block doesn't check the connection before trying a rollback:

>>> cnn = psycopg.connect()
>>> with cnn.transaction():
...     cnn.execute("select 1")
...     cnn.close()
...     1/0
... 
Traceback (most recent call last):
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 744, in transaction
    yield tx
  File "<stdin>", line 4, in <module>
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
  File "/usr/lib/python3.8/contextlib.py", line 131, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 744, in transaction
    yield tx
  File "/home/piro/dev/psycopg3/psycopg/psycopg/transaction.py", line 205, in __exit__
    return self._conn.wait(self._exit_gen(exc_type, exc_val, exc_tb))
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 767, in wait
    return waiting.wait(gen, self.pgconn.socket, timeout=timeout)
  File "psycopg_c/pq/pgconn.pyx", line 186, in psycopg_c.pq.PGconn.socket.__get__
  File "psycopg_c/pq/pgconn.pyx", line 632, in psycopg_c.pq._call_int
  File "psycopg_c/pq/pgconn.pyx", line 614, in psycopg_c.pq._ensure_pgconn
psycopg.OperationalError: the connection is closed

leaving the transaction block should behave like for the connection.

[asqui]

Hmmm, closing the connection while a transaction is in progress seems a bit pathological.

For the connection block, I can see how a quiet no-op makes sense... the with conn block is saying "Please close connection for me when I exit the block." When we come to exit it turns out the user already closed the connection. Fine, their post-condition is already met. We can quietly no-op.

For a transaction block it's a bit different "Please manage a transaction for me, and commit or rollback as appropriate based on how I exit the block." followed by "Oops, I closed the connection in the midst of the block." seems like a reasonable time to raise. We cannot uphold the contract of managing the transaction when exiting the block if the user closed the connection from under our feet.

Moreover, with Python 3 exception chaining both the root exception and consequential exception are clearly represented, so the latter does not hide the former.

I think the current behaviour is fine and appropriate. It is an error to close the connection in the midst of a transaction block and the transaction block should fail on exit (both for successful exit, if it doesn't already, and for exception exit, as illustrated here.)

[dvarrazzo]

Closing the connection, like in the test, is just to simulate a condition that can arise externally: the connection might have been closed by the server, or the network gone down. I think that if we find the connection closed or broken we shouldn't try to roll back: we know that it will go wrong and we will clobber the exception.

If any, because there is an exception already set, I think it's reasonable to try to roll back and, if that fails, to log the error but to reraise the original exception: see Connection.__exit__.

The situation in #158 is more tricky: if we have an error during copy we will get to __exit__ in state ACTIVE. This tells us that the rollback will fail, but, on the other hand, not doing it would be bad, because we risk to fail to maintain our promise. If the connection doesn't recover we are cool, but if it does we would have left the context without rolling back.

I think that trying to roll back is the right thing to do, just clobbering the exception is wrong. Even if the exceptions are chained in Python 3.x, reading them, it is always the bottom one that sticks out, and if someone had a cursory look at #158 they might think that rollback is broken, not kqueue.

Looking at the difference between using and not using a tx in the case of #158, I think the connection context is rather well behaved:

>>> with psycopg.connect() as cnn:
...         with cnn.cursor() as cur:
...             with cur.copy("copy (select generate_series(1,10)) to stdout") as copy:
...                 for row in copy.rows():
...                     1/0
... 
error ignored rolling back transaction on <psycopg.Connection [ACTIVE] (database=piro) at 0x7f0b53e98f10>: sending query failed: another command is already in progress
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
ZeroDivisionError: division by zero

The transaction one is rather noisy:

>>> with psycopg.connect() as cnn:
...     with cnn.transaction():
...         with cnn.cursor() as cur:
...             with cur.copy("copy (select generate_series(1,10)) to stdout") as copy:
...                 for row in copy.rows():
...                     1/0
... 
error ignored rolling back transaction on <psycopg.Connection [ACTIVE] (database=piro) at 0x7f0b53e989a0>: sending query failed: another command is already in progress
Traceback (most recent call last):
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 744, in transaction
    yield tx
  File "<stdin>", line 6, in <module>
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 6, in <module>
  File "/usr/lib/python3.8/contextlib.py", line 131, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 744, in transaction
    yield tx
  File "/home/piro/dev/psycopg3/psycopg/psycopg/transaction.py", line 205, in __exit__
    return self._conn.wait(self._exit_gen(exc_type, exc_val, exc_tb))
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 767, in wait
    return waiting.wait(gen, self.pgconn.socket, timeout=timeout)
  File "/home/piro/dev/psycopg3/psycopg/psycopg/waiting.py", line 228, in wait_epoll
    s = next(gen)
  File "/home/piro/dev/psycopg3/psycopg/psycopg/transaction.py", line 125, in _exit_gen
    return (yield from self._rollback_gen(exc_val))
  File "/home/piro/dev/psycopg3/psycopg/psycopg/transaction.py", line 172, in _rollback_gen
    yield from self._conn._exec_command(b"; ".join(commands))
  File "/home/piro/dev/psycopg3/psycopg/psycopg/connection.py", line 421, in _exec_command
    self.pgconn.send_query(command)
  File "psycopg_c/pq/pgconn.pyx", line 223, in psycopg_c.pq.PGconn.send_query
psycopg.OperationalError: sending query failed: another command is already in progress

[asqui]

What you say makes a lot of sense and you almost have me convinced. I must admit I'm not following the intimate details of #158 and more trying to think about this with fresh eyes, in isolation.

Let me confirm my understanding of the proposed change and associated reasoning:-

If an exception is raised within a transaction block:

1. We should always attempt to roll it back, per the Connection.transaction policy.
2. If that attempt fails, we log it (at what level?), and leave the original exception to propagate up the stack.

This seems reasonable. Any retry logic in the calling code could then retry the entire transaction, if desired, regardless of whether it's the original exception or the rollback failure being raised.

[dvarrazzo]

@asqui

1. We should always attempt to roll it back, per the Connection.transaction policy.
2. If that attempt fails, we log it (at what level?), and leave the original exception to propagate up the stack.

Yes, that's correct.

As for the log level, Connection.__exit__ emits a warning. I think a warning is appropriate enough because, anyway, there is an exception being thrown and the transaction has most likely been aborted anyway: the failure to ROLLBACK should be inconsequential and all we want is that an exception doesn't get swallowed into oblivion. If anyone has stronger opinion that emitting an error would be better, I'd be ok for it, as long as Connection and Transaction are consistent and emit at the same level.

Are we expecting a patch from you? :)

Thank you!