[#31805] DocDB: Fix 60s libpq connect block in CheckYsqlLaggingCatalogVersions - #33006
Open
bmcnama-shopify wants to merge 1 commit into
Open
[#31805] DocDB: Fix 60s libpq connect block in CheckYsqlLaggingCatalogVersions#33006bmcnama-shopify wants to merge 1 commit into
bmcnama-shopify wants to merge 1 commit into
Conversation
…gVersions The periodic lagging-catalog-versions check (94e258a, #26940) opened its internal libpq connection with the general 60s client timeout and scheduled itself on the messenger scheduler with no tracking. When the local postmaster is unreachable (stopped, restarting, or its unix socket gone), `PGConn::Connect` retries until the deadline, so every invocation pinned a messenger io_thread for the full 60s, logged "Could not get the set of lagging catalog versions" only after a minute, and immediately re-scheduled itself - including during process shutdown, where the blocked io_thread stalled the `Messenger::Shutdown` reactor join (~61s SIGTERM-to-exit with a thread-stack dump instead of ~1s). The `should_stop` predicate from 2ef5075 (#32293) only aborts the connect once `TabletServer::Shutdown()` has set `shutting_down_`, so the runtime manifestation and the window before `Shutdown()` remained. Fix, per the two options in the issue: - Use a short connect budget (2s x kTimeMultiplier) for this heartbeat-style check instead of `default_client_timeout()`. A missed cycle only delays invalidation-message garbage collection to the next interval. - Track the task with an `rpc::ScheduledTaskTracker`: bail out of the callback once `shutting_down_` is set, stop re-scheduling after `StartShutdown()`, and `CompleteShutdown()` before the base-class shutdowns join the messenger io_threads that run the callback. The regression test removes the postmaster socket directory (the post-postmaster-shutdown state) under a 1s check interval and asserts the check fails fast (pre-fix: no failure logged within 20s because the in-flight connect was still inside its 60s budget) and that a subsequent graceful tserver shutdown is quick (0.134s observed). Jira: DB-21507 --- _automated · Claude Fable 5 (opencode)_
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.
Summary
Fixes #31805 (Jira: DB-21507).
TabletServer::ScheduleCheckLaggingCatalogVersions()scheduled the periodic lagging-catalog-versions check directly on the messenger scheduler with noScheduledTaskTracker, andCheckYsqlLaggingCatalogVersions()opened its internal libpq connection with the general 60s client timeout (tserver_yb_client_default_timeout_ms).PGConn::Connectretries a failed connect until the deadline, so whenever the local postmaster is unreachable (stopped, restarting, or its unix socket removed) each invocation pinned a messenger io_thread for the full 60s, loggedCould not get the set of lagging catalog versionsonly after a minute, and re-scheduled itself unconditionally - including during process shutdown, where the blocked io_thread stalls theMessenger::Shutdownreactor join: SIGTERM-to-exit took ~61s with a "dumping all thread stacks" report instead of ~1s. Theshould_stoppredicate added by 2ef5075 (#32293) only covers the window afterTabletServer::Shutdown()has setshutting_down_; the runtime manifestation and the gap beforeShutdown()remained.This applies both fixes suggested in the issue:
2s * kTimeMultiplier) for this heartbeat-style check instead ofdefault_client_timeout(). A missed cycle only delays invalidation-message garbage collection until the next interval. This also covers the non-shutdown manifestation, which theshould_stoppredicate cannot.rpc::ScheduledTaskTracker(check_lagging_catalog_versions_task_, thePgClientServiceImpl/YsqlManagerpattern): the callback bails out onceshutting_down_is set,StartShutdown()at the top ofTabletServer::Shutdown()stops the re-schedule loop, andCompleteShutdown()runs before the base-class shutdowns join the messenger io_threads that execute the callback. The runtime re-read ofcheck_lagging_catalog_versions_interval_secson every cycle is preserved.The new regression test drives the check at a 1s interval, removes the postmaster's unix socket directory (the exact post-postmaster-shutdown state), and asserts the check fails fast and that a subsequent graceful tserver shutdown is quick. On the unfixed tree the test fails: no check failure is logged within 20s of socket removal because the in-flight connect is still inside its 60s budget (
connect_timeout=59visible in the conn string). With the fix, the failure is logged ~2.9s after socket removal and graceful tserver shutdown takes ~0.13s.Upgrade/Rollback safety
No wire-format, persisted-state, RPC-versioning, or flag-default changes. The change is node-local (an internal connect deadline for a periodic best-effort check, plus shutdown-path task tracking), so mixed-version clusters and rollback are unaffected.
Test plan
./yb_build.sh debug --cxx-test pg_catalog_version-test --gtest_filter PgCatalogVersionTest.CheckLaggingCatalogVersionsPostmasterDown- new regression test; fails on the unfixed tree (log-waiter timeout after 20005ms), passes with the fix (failure logged 2.9s after socket removal; tserver shutdown 0.134s)../yb_build.sh debug --cxx-test pg_catalog_version-test --gtest_filter PgCatalogVersionTest.InvalMessageMinimalRetention- the test from the issue report; passes with the fix../build-support/lint.sh --rev origin/master- 0 errors, 0 warnings.