Repository navigation
fix: end each psycopg2 persister call's transaction - #950
Open
sangkyoonnam wants to merge 1 commit into
Open
sangkyoonnam wants to merge 1 commit into
sangkyoonnam wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PostgreSQLPersisternow commits or rolls back afteris_initialized,list_app_ids,loadandsave, so one failed insert no longer breaks the persister for the rest of the process, and reads no longer leave a transaction open.psycopg2 runs with autocommit off, so the first statement on the connection opens a transaction.
savecommits on success but doesn't roll back on failure. After a rejected insert, such as a duplicate(partition_key, app_id, sequence_id, position)or a state string with a NUL character, every laterload,list_app_idsandsaveon that persister raisesInFailedSqlTransaction. On main the repro is: save a row, save it again (UniqueViolation), thenloadthe same app.AsyncPostgreSQLPersisterisn't affected, since asyncpg doesn't open a transaction unless asked to.load,list_app_idsandis_initializednever end their transaction, so the session sits inidle in transactionuntil the nextsavecommits.created_atdefaults toCURRENT_TIMESTAMP, which is the transaction start time, so that save is stamped with the time of the earlier read. With two connections, a read on A, a save on B, then a save on A stores A's row with an earliercreated_atthan B's, andload(partition_key, None)returns B's row as the latest.Changes
is_initialized,list_app_ids,loadandsaveget their cursor from a small_transaction()context manager that commits on success and rolls back on any exception,KeyboardInterruptincluded. If the rollback itself fails, usually because the connection is already gone, the original exception is the one raised. It doesn't usewith self.connection:, because psycopg2 refuses to re-enter that from a caller's ownwith connection:block, and that pattern worked on main.How I tested this
UniqueViolationand the nextload,list_app_idsandsavesucceed; the connection reportsTRANSACTION_STATUS_IDLEright afterload(hit and miss),list_app_idsand an uncachedis_initialized; a save on the fixture connection after a read and another connection's save gets acreated_atat or after that save's; aKeyboardInterruptraised by the cursor mid-call leaves the connection idle; and the persister still works inside a caller'swith connection:block. The first four fail on main and pass here. The status checks are the deterministic ones; thecreated_attest relies on the server clock not going backwards.postgres:15container withBURR_CI_INTEGRATION_TESTS=true: 16 passed.pytest tests --ignore=tests/integrations/persisters/test_postgresql.py --ignore=tests/integrations/test_bip0042_bedrock.py: 680 passed, 4 skipped.pre-commit runon the changed files passes.Notes
loadand asavelanding in one transaction no longer get that; each call ends its own transaction, including any statements the caller had pending on that connection. Nothing in the repo does this.list_app_idsbody as fix: list each app once in SQL persister list_app_ids, newest save first #942, so whichever lands second needs a rebase.create_tableis left as is. It already commits, and a failedCREATE TABLEwould still leave the transaction aborted; that seemed out of scope for this fix.Checklist