fix(#589,#597,#593): rollback_ret lock retention, drop_db tx orphan, per-connection sweep registry - #601
Merged
Merged
Conversation
…sweep registry #589: isc_rollback_retaining retains attachment-level relation locks exactly like isc_commit_retaining (#586) - probe: after a retaining rollback of DDL, a fresh SERIALIZABLE NOWAIT attachment blocked over metadata. Zero-open-cursor rollback_ret now hard-rolls-back + restarts with the stored TPB (_php_fbird_trans_rollback_restart), releasing the locks; cursor-bearing transactions keep true retaining semantics. #597: fbird_drop_db() frees live transaction handles BEFORE fbc_drop_database() while the attachment is still valid (rollbackNoThrow+delete via fbt_free) instead of orphaning the fb::Transaction wrappers afterwards (LSAN 40B each). issue591/issue582 run with full LSAN again (scoped detect_leaks=0 removed); only fbird_drop_db_003 keeps it for an unrelated residual init-path leak. #593: statement sweep registry moved from thread_local onto fb::Connection (stmt_head_) - whoever closes the connection sweeps its wrappers regardless of preparing thread (ZTS-safe by construction). fbs_prepare gains a connection_ptr parameter (service-API callers pass NULL - synchronous lifetimes). Service wrappers remain unregistered no-ops by design. New test: tests/issue589_rollback_ret_locks.phpt
…tatements PR #601 session-4 changed fbs_prepare's signature (connection_ptr for the #593 per-connection sweep registry) but pdo_fbird carries its OWN copies of firebird_utils.h/php_fbird_includes.h. PDO compiled against the stale 8-param declaration while the main ext exported 9 params -> garbage args (sql=0x1f) -> SIGSEGV in pdo_fbird_handle_preparer on every pdo test, across all CI matrix cells. Fix: sync the header and pass H->fbc_conn at all three call sites - which also enrolls PDO statements in the #593 sweep registry (bonus: PDO prepared statements now get connection-death invalidation too). Found via full-CI-matrix failure; reproduced locally on php83-fb3-dev; FB3 full suite 358/0 after fix.
This was referenced Aug 24, 2026
Closed
Review findings applied: - UPGRADING #589: BLOB-handle invalidation warning (same hazard as #586; rollback_ret blob-loops must keep a cursor open) - fbird_inspection.c x3: comment rationale corrected - NULL is safe because the statement is freed synchronously in-function, NOT because it is a 'service attachment' (these use the user's attachment; the wrong rationale invited unsafe copies of the pattern) - fbird_transaction.c: stale comment claiming fbt_* 'ALWAYS delete the wrapper' contradicted by implementations and load-bearing-adjacent for rollback_restart's failure path #602 acceptance criteria (review finding 3 - landed here per reviewer): - scripts/check-header-parity.sh: fails when firebird_utils.h (ABI- load-bearing prototypes) or php_fbird_includes.h (struct layouts) drift between main ext and pdo_fbird copies - pdo_fbird/php_fbird_includes.h reconciled to a byte-identical copy (was generations stale; safe because pdo touches only opaque handles - verified by FB3 full suite 358/0 after sync) - code-quality.yml runs the gate alongside check-stubs-sync Ponytail -30 twin-merge skipped deliberately: repo convention is documented per-issue twins (commit_restart/rollback_restart).
Author
Review findings implemented (human-level ponytail review, all four)
Ponytail -30 twin-merge skipped deliberately (repo convention: documented per-issue twins). CI on head commit: CI/Sanitizers/Coverage/Code Quality (incl. new parity gate)/Windows/CodeQL all green; doctrine 4.4.x green; v3.18 = documented pre-existing #578. Awaiting human merge approval. |
4 tasks
- fbird_transaction.c: the #599 batch-walk order bug (batch_head cleared before the walk reads it - #600 init-regex collateral) is documented with a jane: comment and DEFERRED to #603: activating the walk crashes fbird_batch_multitype_001 at process exit (simple case passes; the complex lifecycle needs its own root-cause). Walk left in the known-good dead state. - firebird_utils.cpp + fb_connection.hpp: unified #591/#593 banner - stale per-thread description removed; thread-model claim corrected to 'safe because PHP never manipulates one connection's statements from multiple threads concurrently' (was: ZTS-safe-by-construction, which overstated) - UPGRADING.md: lockstep-upgrade note for the firebird.so/pdo_fbird.so ABI boundary (old pdo .so + new firebird .so = PDO::prepare SIGSEGV, #602 class) Full suite php83-dev: 312/0.
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
1 participant
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.
Verification