RHINENG-27056: add feature flag to terminate all db sessions during migration#2237
Conversation
Reviewer's GuideAdds an opt-in terminate_db_sessions flag to database-admin to forcibly close app DB sessions before migrations, refactors pre-migration logic into a reusable prepareForMigration() flow, introduces helpers and tests for listing/terminating active sessions, and documents the new behavior and configuration across database and architecture docs and ClowdApp config. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="database_admin/update.go" line_range="21" />
<code_context>
+const activeAppSessionsQuery = `SELECT usename || ' ' || substring(query for 50)`
</code_context>
<issue_to_address>
**suggestion:** Consider aligning the pg_stat_activity filters between the "find" and "list" queries.
`activeAppSessionsQuery` and `activeAppSessionPIDsQuery` use slightly different filters (e.g., only the latter excludes `pg_backend_pid()`). Please either share a common base condition or make their semantics intentionally consistent, so the “active” sessions you wait on are the same ones you terminate. Adding an explicit `pg_backend_pid()` exclusion to both would also guard against future config changes (e.g., if `lockUsers` ever includes the current user).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| const activeAppSessionsQuery = `SELECT usename || ' ' || substring(query for 50) | ||
| FROM pg_stat_activity | ||
| WHERE usename = ANY($1) |
There was a problem hiding this comment.
suggestion: Consider aligning the pg_stat_activity filters between the "find" and "list" queries.
activeAppSessionsQuery and activeAppSessionPIDsQuery use slightly different filters (e.g., only the latter excludes pg_backend_pid()). Please either share a common base condition or make their semantics intentionally consistent, so the “active” sessions you wait on are the same ones you terminate. Adding an explicit pg_backend_pid() exclusion to both would also guard against future config changes (e.g., if lockUsers ever includes the current user).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2237 +/- ##
=======================================
Coverage 59.06% 59.06%
=======================================
Files 138 138
Lines 8848 8848
=======================================
Hits 5226 5226
Misses 3076 3076
Partials 546 546
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Context:
ALTER USER … NOLOGINblocks new logins but leaves existing connections open, which can block DDL. This flag is opt-in for major migrations only; normal deploys are unchanged (default false).Built on top of the waitForSessionClosed fix (PR). This PR will be rebased afterwards to avoid conflicts.
This PR:
terminate_db_sessionsPOD_CONFIG flag (default off) to kill app DB sessions before migration DDLpg_terminate_backendafterNOLOGIN, then waits until no app sessions remainprepareForMigration()fromstartMigration()(block → optional terminate → wait) so the pre-migration flow is testable without runningMigrateUplistActiveAppSessions/terminateAppSessionshelpers and unit tests for session termination and full pre-migration flowterminate_db_sessions=trueonDATABASE_ADMIN_CONFIGindeploy/clowdapp.yamlSummary by Sourcery
Add an optional configuration flag to terminate application database sessions before running migrations and document the migration/session-handling flow.
New Features:
terminate_db_sessionsPOD configuration flag to force-close app database sessions prior to migration DDL when enabled.Enhancements:
prepareForMigrationstep that blocks users, optionally terminates sessions, and waits for sessions to drain.Documentation:
terminate_db_sessionsflag, in the database and architecture docs and agent guide.terminate_db_sessionson the db-migration job.Tests: