-
Notifications
You must be signed in to change notification settings - Fork 2.2k
postgres & redshift ddl and e2e fixes #26367
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
|
|
||
| from sqlalchemy import text | ||
| from sqlalchemy.engine import Engine, reflection | ||
| from sqlalchemy.exc import ProgrammingError | ||
| from sqlalchemy.schema import CreateTable, MetaData | ||
|
|
||
| from metadata.utils.logger import ingestion_logger | ||
|
|
@@ -158,6 +159,14 @@ def get_all_table_ddls( | |
| except Exception as exc: | ||
| logger.debug(traceback.format_exc()) | ||
| logger.debug(f"Failed to get table ddls for {schema_name}: {exc}") | ||
| # Roll back the aborted transaction so the connection remains usable | ||
| # for subsequent queries (e.g. get_table_comment). Without this, | ||
| # psycopg2 raises InFailedSqlTransaction on every query that follows. | ||
| if isinstance(exc, ProgrammingError): | ||
| try: | ||
| connection.rollback() | ||
| except Exception: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: Silent pass on rollback failure hides connection issuesThe bare Suggested fix: Was this helpful? React with 👍 / 👎 | Reply |
||
| pass | ||
|
|
||
|
|
||
| def get_table_ddl_wrapper( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rollback guard at line 165 only triggers for
ProgrammingError, but PostgreSQL/Redshift can abort a transaction on other exception types too (e.g.,OperationalError,DatabaseError). Any unhandled aborted transaction will cause the sameInFailedSqlTransactionproblem for subsequent queries. Since this is inside a broadexcept Exceptionblock, it would be safer to attempt rollback for anyDBAPIError(the base class for all DBAPI-related SQLAlchemy exceptions).Suggested fix:
Was this helpful? React with 👍 / 👎 | Reply
gitar fixto apply this suggestion