Classify SqlState Connection Exceptions as operational errors

[sjustas]

tl;dr: conform with PEP 249 better, so that libraries can treat disconnect errors caused by PgBouncer as transient.

DatabaseError is too broad and is considered not transient failure by libraries, which causes failures where an operation could simply be re-tried, like relstorage:drivers.py#L157 or pjpersist:datamanager.py#L100

A real world example: conntect to PgBouncer instead of Postgres directly. Now, if the Postgres server is disrupted, you get a ProtocolViolation (SqlState 08P01) sent by PgBouncer: pgbouncer:src/server.c#L481 which leads to pgbouncer:/src/proto.c#L128.

Traceback (most recent call last):
  File ".../ve3.7/lib/python3.7/site-packages/celery/app/trace.py", line 385, in trace_task
    R = retval = fun(*args, **kwargs)
  <...>
  File ".../src/app/taskcontext.py", line 39, in celery_request
    conn = db.open()
  File ".../ve3.7/lib/python3.7/site-packages/ZODB/DB.py", line 793, in open
    result.open(transaction_manager)
  File ".../ve3.7/lib/python3.7/site-packages/ZODB/Connection.py", line 921, in open
    self.newTransaction(None, False)
  File ".../ve3.7/lib/python3.7/site-packages/ZODB/Connection.py", line 737, in newTransaction
    invalidated = self._storage.poll_invalidations()
  File ".../ve3.7/lib/python3.7/site-packages/relstorage/storage.py", line 1421, in poll_invalidations
    changes, new_polled_tid = self._restart_load_and_poll()
  File ".../ve3.7/lib/python3.7/site-packages/relstorage/storage.py", line 1395, in _restart_load_and_poll
    self._adapter.poller.poll_invalidations, prev, ignore_tid)
  File ".../ve3.7/lib/python3.7/site-packages/relstorage/storage.py", line 365, in _restart_load_and_call
    return f(self._load_conn, self._load_cursor, *args, **kw)
  File ".../ve3.7/lib/python3.7/site-packages/relstorage/adapters/poller.py", line 95, in poll_invalidations
    cursor.execute(self.poll_query)
psycopg2.errors.ProtocolViolation: server conn crashed?
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

PEP 249 states that unexpected disconnects and similar failures not under programmers control should be considered OperationalFailure, so higher level library developers tend to treat that exception transient.

OperationalError is inherited from DatabaseError, so this PR is mostly backwards-compatible.

[sjustas]

Hey @dvarrazzo, does a change like this look reasonable to include into psycopg2?
No worries if you don't have time to look at it right now, I appreciate this is an open source effort! Just didn't want this to slip under the radar in case it looks insignificant.

[dvarrazzo]

Hello @sjustas

thank you. It is not insignificant: I think the 08 errors are misclassified, yes. Because your patch converts those errors into a subclass I think it's good to accept. Other misclassifications (PL/pgSQL errors as InternalError) are pretty bad, but cannot be changed without breaking compatibility.

Could you please add a test to verify issubclass() and an entry in the NEWS file for psycopg 2.9 (we'll probably backport it too on 2.8.next too, but now master is on 2.9).

Thank you very much.

[dvarrazzo]

Oh, I've noticed now that you opened the issue back in September. I apologise: it did slip under my radar, but it's absolutely something I'd like to address 👍

[dvarrazzo]

Merged, thank you very much!

[sjustas]

Thank you!

[sjustas]

Hey @dvarrazzo, hope you are well! Is there a chance of a new release with this small fix some time this spring? Would appreciate this a lot!

[dvarrazzo]

@sjustas thank you for the nudge. I can't immediately but might in a few weeks time.

Thank you for the patience

[sjustas]

No worries, and thanks in advance!