perf: replace get_discussion tree walk with single recursive CTE - #383
Open
ety001 wants to merge 2 commits into
Open
perf: replace get_discussion tree walk with single recursive CTE#383ety001 wants to merge 2 commits into
ety001 wants to merge 2 commits into
Conversation
Add structured WARNING logs to diagnose connection-pool exhaustion: - [DISCUSSION_SLOW] in get_discussion(): total time + per-stage breakdown (post_id lookup, hide checks, load_discussion), logged when total > 1s - [DISCUSSION_BREAKDOWN] in _load_discussion(): tree_walk time (with query count, depth, post count) vs load_posts time, logged when either > 500ms - [POSTS_KEYED_SLOW] in load_posts_keyed(): fetch/author/comm_roles breakdown, logged when total > 500ms - [DB_SLOW] in sqltimer decorator: any DB query slower than 3s with SQL All logs use WARNING level so they appear in /var/log/messages and are collected by Scalyr. No business logic changes.
Replace the level-by-level BFS (one _child_ids query per depth level, observed 500-1300ms for depth 4-10 threads) with a single WITH RECURSIVE query that fetches the whole comment subtree in one DB round-trip. - Filter hidden authors (list_type='3') and hidden posts (list_type='1') inside the recursion so their subtrees are never traversed - Keep MAX_DEPTH / MAX_THREAD_POSTS caps (LIMIT inside the CTE) and BFS ordering so truncation semantics match the old implementation - Cache the whole tree under discussion_tree_<root_id> (120s TTL) - Fix [DB_SLOW] log: collapse SQL whitespace so multi-line queries are not truncated at the first line break by syslog (observed empty SQL body) - Update bridge_thread unit tests for single-CTE semantics
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.
Summary
Fixes two issues found while validating the
debug-get-discussion-tracingimage in production:1.
get_discussiontree walk: level-by-level BFS → single recursive CTEThe old
_load_discussionwalked the comment tree one depth level at a time, issuing one_child_idsquery per level. Production logs showed 500-1300ms for depth 4-10 threads — each level adds a pool acquire + parse + round-trip.Replaced with a single
WITH RECURSIVEquery fetching the whole subtree in one DB round-trip:list_type='3') and hidden posts (list_type='1') are filtered inside the recursion, so their subtrees are never traversedMAX_DEPTH/MAX_THREAD_POSTScaps preserved (LIMITinside the CTE), BFS ordering kept so truncation semantics match the old implementationdiscussion_tree_<root_id>(120s TTL), replacing the per-level_child_ids_*cache2.
[DB_SLOW]log: SQL body was empty in productiondb.pyloggedargs[1][:200]directly — multi-line SQL starts with\n, so syslog truncated every entry after the timestamp. Collapse whitespace before logging (same as_normalize_sqlin stats.py).Verification
tests/bridge_thread/: 9/9 passed (rewritten for single-CTE semantics)LIMIT :max_postsbinding verified with production SQLAlchemy 1.4.54tests/utils: 3 pre-existing failures unrelated to this change (confirmed via git stash baseline)Notes
DISCUSSION_SLOW/DISCUSSION_BREAKDOWN/POSTS_KEYED_SLOW/DB_SLOW) remain in this branch for continued validationinternal/api/bridge/post.go) still hasTODO: Fetch all replies recursively— this change is the reference implementation for that