Log, but allow, failures during cleanup rollbacks. #1647
Conversation
In Postgres, rollbacks can fail if the transaction was killed by the database. One common scenario is that the `idle_in_transaction_session_timeout` is enabled. If the transaction was cancelled, the connection is left open in `dbt`. `dbt` attempts to close that connection after issuing a `ROLLBACK`. But it fails since the transaction was severed. Since the cleanup is carried out in a `finally` statement, the `psycopg2.InternalDatabaseError` is thrown and prevents the test case results from ever being shown. Changes here wrap the `ROLLBACK` in a try-catch such that if there is an exception thrown, it is logged appropriately, but ultimately proceeds.
There was a problem hiding this comment.
Thanks for the PR @levimalott! This looks great!
One comment about the log output, let me know if you'd like to discuss! I think we should be able to get this merged for the 0.14.1 release. Thanks again!
| try: | ||
| connection.handle.rollback() | ||
| except Exception: | ||
| logger.exception('Failed to rollback {}'.format(connection.name)) |
There was a problem hiding this comment.
Can we use logger.error instead of logger.exception here? We try not to log stack traces to stdout. In this case, it might be appropriate to log the exception to the debug logs, eg. with logger.debug('', exc_info=True).
There was a problem hiding this comment.
Can we use
logger.errorinstead oflogger.exceptionhere? We try not to log stack traces to stdout. In this case, it might be appropriate to log the exception to the debug logs, eg. withlogger.debug('', exc_info=True).
Makes sense! I'll have an update shortly.
|
Thanks! Just kicked off the CI tests :) |
|
Ah! Looks like there's just a pep8 (formatting) issue: |
Should be good now 😎 |
|
Thanks @levimalott! kicking off the tests now :) |
|
Merging this! Thanks @levimalott - this is going out in dbt v0.14.1 |
In Postgres, rollbacks can fail if the transaction was killed
by the database. One common scenario is that the
idle_in_transaction_session_timeoutis enabled.If the
transaction was cancelled, the connection is left open
in
dbt.dbtattempts to close that connection after issuinga
ROLLBACK. But it fails since the transaction was severed.Since the cleanup is carried out in a
finallystatement, thepsycopg2.InternalDatabaseErroris thrown and prevents thetest case results from ever being shown.
Changes here wrap the
ROLLBACKin a try-catch such thatif there is an exception thrown, it is logged appropriately,
but ultimately proceeds.
Before
Prior to this change, a failed rollback during cleanup would prevent the test results from every being shown as the exception is thrown during the
finallyclause.After
Here the exception is logged, but caught. Test case results are shown afterwards rather than
dbtcrashing.