Add test for verifying exception handling- cherry-pick - #603
Open
mason-sharp wants to merge 9 commits into
Open
Conversation
For in between transactions. (cherry picked from commit f787377)
Two sleep(2) calls stood in for synchronisation. The first waited for the apply worker to pick up a reloaded spock.exception_behaviour, the second for it to go idle before the cancel. Neither was guaranteed: the row arriving proves the commit is visible, but handle_commit() then calls process_syncing_tables(), which opens a transaction of its own, so the cancel could land outside the window the test is about and the test would pass without exercising the leak. Use spock.sync_event() and wait_for_sync_event() instead. A non-transactional sync event is decoded outside any remote transaction and ordered after the preceding commit, so the origin reaching its LSN means handle_commit() ran to the end -- replay queue reset, use_try_block cleared. It also orders the reload, since the worker tests ConfigReloadPending before reading from the stream. The same call replaces wait_for_row(), a 90s polling loop that burned its full timeout on failure; a plain SELECT after the sync does the job. Renumbered from 037, which PR #545 also claims, and added to tests/tap/schedule -- CI runs run_tests.sh, which is schedule-driven, so the test would never have been executed. 29 subtests to 33, passing, ~26s. (cherry picked from commit 1383001)
035, 036 and 039 each carried a copy of read_log_from() and wait_for_log(); 013_origin_change_restore, 018, 020 and 035 each hand-rolled a poll-a-query-until-it-returns-X loop under a different name. PostgreSQL::Test has wait_for_log(), slurp_file() and poll_query_until() for exactly this, but nothing in this suite uses PostgreSQL::Test, so add equivalents to SpockTest.pm instead: node_logfile, log_offset, log_since, wait_for_log, poll_query_until, sync_nodes and apply_worker_pid. They poll every 0.1s, as PostgreSQL::Test does, rather than every second. 039 uses them and loses its four local helpers, 173 lines to 128; it no longer needs to know that a node's log is named after its port. 035 and 036 keep their own copies for now -- @EXPORT_OK exports only on request, so nothing collides. (cherry picked from commit 3683135)
create_cluster() started the postgres instances and then slept a flat 17 seconds. They are normally accepting connections in about two, so most of that was dead time, paid by all 57 tap tests. Poll wait_for_pg_ready() instead, which was already in the module and already used by 013 and 016, and die naming the node if one never comes up -- previously a server that failed to start showed up later as a confusing psql error. Its polling interval goes from 1s to the 0.1s the other wait_* helpers use, so the wait does not round back up. 039 typically goes from 26s to 8s. Run-to-run variance is unchanged and still real -- an occasional run lands nearer 25s -- but the floor is no longer set by the sleep. (cherry picked from commit 800a892)
Both came out of review on 039. log_since() returned '' when the log could not be opened. Callers grep the result, so an unreadable log looked exactly like a readable one that does not contain the pattern, and the unlike() in 039 passed on a broken read. Die naming the path instead. apply_worker_pid() matched application_name LIKE 'spock apply %' across the whole instance. A node with two subscriptions returns two rows and scalar_query() strips the newline between them, so two pids came back as one run of digits that still matched /^\d+$/ and went on to pg_cancel_backend(). Restrict the match to the current database, resolve a named subscription to the subid the worker carries (spock_worker.c names workers "spock apply <dboid>:<subid>"), and die on an ambiguous match. 039 now names its subscription. (cherry picked from commit 05f15ca)
$LOG_DIR fell back to a relative "logs" when TESTLOGDIR was unset, and only run_tests.sh and the make target export it. create_postgresql_conf() feeds $LOG_DIR to log_directory, and PostgreSQL resolves a relative log_directory against the data directory while Perl resolves it against the current directory. The server logged to <datadir>/logs/005442.log while the test read ./logs/005442.log, so running a single test with plain "prove t/039_no_replay_mode_leak.pl" could not work -- it died on the first log read. 036 and 001 were unrunnable the same way. rel2abs() rather than a TESTLOGDIR default, so a relative TESTLOGDIR is also resolved. Cherry-picking c45f722 would not have covered this: it exports TESTLOGDIR from the make target, which repairs make and run_tests.sh but leaves a bare prove run still broken. (cherry picked from commit 1c6c1ad)
Both kept a local read_log_from() that returned '' when the log could not be opened, so an unreadable log looked exactly like a readable one without the pattern. Five assertions passed on a broken read: three unlike() in 035, one in 036, and the cmp_ok() in 035 that counts retries through count_in_log() -- a failed open counts zero, and zero is under the cap. Use log_offset(), log_since() and wait_for_log() from SpockTest.pm instead of patching the local copies, so the two duplicated helpers go away with the bug. count_in_log() stays in 035, now reading through log_since(). Neither test hardcodes the 00<port>.log name any more, and both can be run individually. 107 and 16 subtests as before. (cherry picked from commit 687b18f)
Up to standards ✅🟢 Issues
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR adds shared TAP polling and log helpers, updates deadlock retry tests to use them, replaces fixed cluster startup delays with readiness checks, and adds a scheduled test for replay-mode leakage under two exception behaviours. ChangesSpock test reliability
Poem
Merge Risk: ⚪ Minimal · up to This change adds test coverage and updates a configuration-reload comment. No concrete merge-blocking behavior, data integrity, security, or runtime risk is established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
danolivo
requested changes
Sep 2, 2026
| "$mode: the next transaction applies"); | ||
|
|
||
| unlike(log_since(2, $offset), | ||
| qr/applied nothing, retrying without exception handling/, |
Contributor
There was a problem hiding this comment.
Does it actually exist in Spock 6?
I see the line:
elog(LOG, "SPOCK %s: replay at LSN %X/%X applied nothing ...
in v5_STABLE only.
| my $mode = $phase->{mode}; | ||
|
|
||
| psql_or_bail(2, "ALTER SYSTEM SET spock.exception_behaviour = $mode"); | ||
| psql_or_bail(2, "SELECT pg_reload_conf()"); |
Contributor
There was a problem hiding this comment.
Being disgustingly neat, I would say this call just sends a signal to the postmaster, not actually changes the value. Hence, races are possible. But I have never seen any actual issues before so that it may be skipped.
Both from review. The unlike() matched "applied nothing, retrying without exception handling", which exists only in v5_STABLE. On main it can never match, so the assertion passed unconditionally. Main logs the same situation from handle_commit(), worded per mode, under the guard that is the leak itself: !xact_had_exception && use_try_block && (TRANSDISCARD || SUB_DISABLE). Match that instead, per phase. pg_reload_conf() is one kill(SIGHUP) at the postmaster, which reloads and only then signals its children, so the running worker could still hold the old exception_behaviour. Terminate the worker after the ALTER SYSTEM; the manager respawns it and it reads the setting at startup. 33 subtests to 35.
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
2 participants
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.
Cherry-picks from PR #585 for v5_STABLE that adds a test for exception handling