perf(#124): クエリ最適化 (joinedload + 認可統合 + pool 設定見直し) - #143
Conversation
Issue #124 のレイテンシーをさらに削るため、1 リクエスト中の SQL ラウンドトリップ数を 減らす 3 点の最適化をまとめる。Singapore 移行で短縮した RTT (≈ 160ms × 3-5 クエリ) をクエリ数を 1-2 に絞ることでさらに削減し、ゴール p95 < 500ms を狙う。 - selectinload → joinedload 化 (cruds/trips, pages, blocks) - Trip → Pages → Blocks → Locations の 4 段連鎖クエリを 1 つの LEFT JOIN に集約 - 1-to-many の重複行除去のため呼び出し側で `result.unique()` を挟む - Block の location / destination_location は 1-to-1 なので重複は増えない - GET 系の認可と本体取得を 1 クエリに統合 (routers/blocks, pages) - 従来の require_block_access / require_page_access は 認可用 SELECT を 1 本発行 していたが、本体取得の joinedload に乗せて trip_id を取り出す形に変更 - GET /blocks/{block_id}: blocks_cruds.get_block_with_page で Block + Page + locations を 1 クエリ取得 → page.trip_id を Cookie で検証 - GET /pages/{page_id}: pages_cruds.get_page で Page + blocks + locations を 1 クエリ取得 → page.trip_id を Cookie で検証 - GET /pages/{page_id}/blocks: 上と同じ get_page を再利用し page.blocks を返す (旧 find_blocks 経由は廃止) - PUT / DELETE 系は副作用ありで境界が複雑なので require_*_access のまま据え置き - pool_pre_ping=False (db_connection.py) - チェックアウト毎の SELECT 1 (1 RTT) を省略 - 代わりに pool_recycle=600 を追加し、10 分以上経った接続は次回 checkout 時に 破棄して再確立。Neon Pooler 側の切断やネットワーク中断を握り潰さない保険 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a series of performance optimizations aimed at reducing database latency and SQL roundtrips to meet the p95 < 500ms target. By consolidating related data fetching into single queries and streamlining authorization logic, the changes significantly reduce the overhead per request. Additionally, database connection pool settings were adjusted to minimize handshake latency. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Visit the preview URL for this PR (updated for commit 07e329d): https://tabi-share-8ef6b--pr143-perf-issue-124-query-tg1716in.web.app (expires Thu, 04 Jun 2026 15:05:35 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9f2a87ede127df7673322845e34cf22c1372d720 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes database queries by switching from selectinload to joinedload to fetch relations in a single query, and updates routers to perform authorization checks in-memory. However, the review highlights several performance and stability concerns: disabling pool_pre_ping could lead to unexpected 500 errors due to dropped connections, and using heavy joined loads in list and update operations (find_trips, update_trip, and update_page) introduces unnecessary query overhead. It is recommended to revert pool_pre_ping to True and separate the queries for update and list operations to avoid loading unused relations.
| echo=False, | ||
| # チェックアウト時の SELECT 1 を省略 (1 RTT 削減)。代わりに pool_recycle で | ||
| # 古い接続を破棄し、まれな切断は SQLAlchemy の自動 reconnect に任せる。 | ||
| pool_pre_ping=False, |
There was a problem hiding this comment.
概要
pool_pre_ping=False に設定すると、データベース接続が切断された際、最初のリクエストで OperationalError などの例外が発生し、ユーザーに 500 エラーが返る原因になります。
詳細
SQLAlchemy は pool_pre_ping=True（Pessimistic Disconnect 方式）が指定されていない場合、プール内の古い接続が有効かどうかを事前に検証しません。特に Neon のようなサーバーレスデータベースでは、一定時間無風状態が続くとコンピュートノードがサスペンド（一時停止）したり、コネクションプール側で接続が強制切断されたりすることが頻繁に発生します。
pool_recycle=600 を設定していても、10分以内に切断された場合や、サスペンドからの復帰時には、プール内のデッドコネクションを掴んでしまい、リクエストが失敗します。自動再接続（reconnect）は、そのエラーが発生した後の次のリクエストから有効になるため、最初のリクエストを救うことはできません。
RTT 削減のメリットに対して、本番環境での突発的な 500 エラー（可用性の低下）というデメリットが大きすぎるため、pool_pre_ping=True を維持することを強く推奨します。
| pool_pre_ping=False, | |
| pool_pre_ping=True, |
References
- リポジトリスタイルガイドの「パフォーマンス」および「保守性」原則に基づき、本番環境での可用性と安定性を確保するために接続の健全性チェックを維持します。 (link)
| """ | ||
| result = await db.execute(_trip_with_relations()) | ||
| return list(result.scalars().all()) | ||
| return list(result.unique().scalars().all()) |
There was a problem hiding this comment.
概要
find_trips において、すべての旅行プラン（Trip）を一覧取得する際にも _trip_with_relations() を使用して Pages -> Blocks -> Locations までを joinedload で一括取得していますが、これはパフォーマンス上のボトルネックになる可能性があります。
詳細
- レスポンススキーマの確認: もし旅行プラン一覧取得 API（例:
GET /trips）のレスポンスにpagesやblocksの詳細が含まれていない場合、これらの大量の関連データを JOIN して取得することは無駄なクエリコストとなり、メモリやネットワーク帯域を圧迫します。 - データ量増加時の懸念: 全ての Trip に対して全 Page、全 Block を 1 クエリの LEFT JOIN で取得すると、レコード数が掛け算で増加し（巨大なデカルト積のような状態）、
result.unique()による Python 側での重複排除処理の負荷も非常に高くなります。
対策案
一覧取得用の find_trips では、リレーションを含めないシンプルなクエリ（または必要な最小限のリレーションのみ）を使用するようにクエリを分離することを推奨します。
# 例: リレーションを含めないシンプルな一覧取得
async def find_trips(db: AsyncSession) -> list[Trip]:
result = await db.execute(select(Trip))
return list(result.scalars().all())References
- リポジトリスタイルガイドの「パフォーマンス」原則（コードは効率的であるべき）に基づき、不要な関連データのロードを避ける設計に改善します。 (link)
| """ | ||
| result = await db.execute(_trip_with_relations().where(Trip.id == trip_id)) | ||
| return result.scalar_one_or_none() | ||
| return result.unique().scalar_one_or_none() |
There was a problem hiding this comment.
概要
get_trip が _trip_with_relations() を使用して Pages -> Blocks -> Locations をすべて joinedload するようになったため、update_trip（旅行プランの更新処理）でもこの重いクエリが実行されてしまいます。
詳細
旅行プランの更新（update_trip）では Trip の基本情報（title や detail など）のみを更新するため、すべての関連データを JOIN して取得するのは非効率です。
対策案
update_trip 内では、関連データを含まないシンプルなクエリで Trip を取得するように変更することを推奨します。
async def update_trip(db: AsyncSession, trip_id: int, trip: TripUpdate) -> Trip | None:
# 関連データを含まないシンプルな取得
stmt = select(Trip).where(Trip.id == trip_id)
result = await db.execute(stmt)
db_trip = result.scalar_one_or_none()
if db_trip:
for key, value in trip.model_dump().items():
setattr(db_trip, key, value)
await db.commit()
return db_tripReferences
- リポジトリスタイルガイドの「パフォーマンス」原則（コードは効率的であるべき）に基づき、更新処理における不要な関連データのロードを避ける設計に改善します。 (link)
| """ | ||
| result = await db.execute(_page_with_relations().where(Page.id == page_id)) | ||
| return result.scalar_one_or_none() | ||
| return result.unique().scalar_one_or_none() |
There was a problem hiding this comment.
概要
get_page は _page_with_relations() を使用して常に blocks や locations を joinedload していますが、これが update_page（ページの更新処理）内でも呼び出されているため、更新時にも不要な関連データの取得（LEFT JOIN）が走ってしまいます。
詳細
ページの更新処理（update_page）では、通常 Page のスカラー値（title や date）のみを更新するため、関連データをロードする必要はありません。
対策案
update_page 内では、関連データを含まないシンプルなクエリで Page を取得するように変更することを推奨します。
async def update_page(db: AsyncSession, page_id: int, page: PageUpdate) -> Page | None:
# 関連データを含まないシンプルな取得
stmt = select(Page).where(Page.id == page_id)
result = await db.execute(stmt)
db_page = result.scalar_one_or_none()
if db_page:
for key, value in page.model_dump().items():
setattr(db_page, key, value)
await db.commit()
return db_pageReferences
- リポジトリスタイルガイドの「パフォーマンス」原則（コードは効率的であるべき）に基づき、更新処理における不要な関連データのロードを避ける設計に改善します。 (link)
選択句に明示的な ORDER BY が無い状態で `result.unique().scalars()` を回すと PostgreSQL の返却順がオプティマイザ依存になり、joinedload では LEFT JOIN の ハッシュ結果順序が selectinload 時代の id 昇順と一致しなくなった。 test_find_trips が trips[0] = 'trip 2' になって失敗していたのを修正する。 - models.py: Trip.pages と Page.blocks に order_by="..." を追加し、relationship 経由のアクセスでも子要素を id 昇順で揃える - cruds/trips.py: find_trips() の select に order_by(Trip.id) を追加 - cruds/pages.py: find_pages() の select に order_by(Page.id) を追加 - cruds/blocks.py: find_blocks() の select に order_by(Block.id) を追加 ローカル (Docker Postgres 16) で `pytest tests/` 97 件すべて PASS を確認。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gemini code assist から指摘あり: Neon Free は 5 分アイドルで compute がサスペンス されるため、`pool_pre_ping=False` + `pool_recycle=600` ではプール内に残った dead connection を最初のリクエストが掴んで 500 になる可能性が高い。 - pool_pre_ping を True に戻して checkout 時の SELECT 1 で死活確認する - pool_recycle は 1800 (30 分) に延長。pool_pre_ping との二重防御として残す - 1 RTT のオーバーヘッドが戻るが、joinedload 化 (3 RTT 削減) と認可統合 (1 RTT 削減) の効果は維持される ローカル (Docker Postgres 16) で `pytest tests/` 97 件すべて PASS を確認。 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@gemini-code-assist レビューありがとうございます。各指摘への対応を以下にまとめます。 ✅ 指摘 1 (
|
|
ご丁寧な回答と修正内容の共有、ありがとうございます。 ご指摘いただいた各点について確認いたしました。
ローカル環境でのテスト通過も確認済みとのことですので、このまま進めて問題ないと考えます。引き続き、本番環境での計測とパフォーマンス改善の成功を応援しております。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/app/routers/blocks.py (1)
57-61: ⚡ Quick winCentralize the inline trip-access check.
The
NotFound/Forbiddenpolicy is now duplicated here and again inserver/app/routers/pages.py, so future auth changes can drift across handlers. A tiny helper that only validatestrip_idfrom an already-fetched entity would keep the 1-query optimization while restoring a single place for this contract.Also applies to: 83-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/app/routers/blocks.py` around lines 57 - 61, Duplicate inline trip-access checks (raising NotFound/Forbidden) should be replaced by a single helper to centralize the contract: add a function like validate_trip_access_for_entity(entity, request) (or validate_entity_trip_access) that accepts an already-fetched model (e.g., the db_page returned by pages_cruds.get_page) and calls get_allowed_trip_ids(request) then raises NotFound or Forbidden as appropriate; update the call sites in blocks.py (the checks after pages_cruds.get_page) and the similar block in pages.py (lines referenced in the comment) to call this helper instead of duplicating the logic so future auth changes live in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/app/routers/blocks.py`:
- Around line 57-61: Duplicate inline trip-access checks (raising
NotFound/Forbidden) should be replaced by a single helper to centralize the
contract: add a function like validate_trip_access_for_entity(entity, request)
(or validate_entity_trip_access) that accepts an already-fetched model (e.g.,
the db_page returned by pages_cruds.get_page) and calls
get_allowed_trip_ids(request) then raises NotFound or Forbidden as appropriate;
update the call sites in blocks.py (the checks after pages_cruds.get_page) and
the similar block in pages.py (lines referenced in the comment) to call this
helper instead of duplicating the logic so future auth changes live in one
place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16e28e20-963d-4635-a1bc-275d52272508
📒 Files selected for processing (8)
.vscode/settings.jsonserver/app/cruds/blocks.pyserver/app/cruds/pages.pyserver/app/cruds/trips.pyserver/app/db_connection.pyserver/app/models.pyserver/app/routers/blocks.pyserver/app/routers/pages.py
1 participant
概要
Issue #124 のレイテンシーをさらに削るため、1 リクエスト中の SQL ラウンドトリップ数を減らす 3 点の最適化をまとめる。Singapore 移行で短縮した RTT (≈ 160ms × 3-5 クエリ) を、クエリ数を 1-2 に絞ることでさらに削減し、ゴール p95 < 500ms を狙う。
詳細
A.
selectinload→joinedload化cruds/trips.py/cruds/pages.py/cruds/blocks.pyTrip → Pages → Blocks → Locationsの 4 段連鎖クエリを 1 つの LEFT JOIN に集約。Trip → Pages → Blocksは 1-to-many なので重複行が発生するためresult.unique()を挟む。Block → Locationは 1-to-1 で重複なし。B. GET 系の認可と本体取得を 1 クエリに統合
routers/blocks.py/routers/pages.pyrequire_block_access/require_page_accessは 認可用 SELECT を 1 本発行 していたが、本体取得の joinedload に乗せてtrip_idを取り出す形に変更:GET /blocks/{block_id}blocks_cruds.get_block_with_pageで Block + Page + locations を 1 クエリ取得 →page.trip_idを Cookie 検証GET /pages/{page_id}pages_cruds.get_pageで 1 クエリ取得 →page.trip_idを Cookie 検証GET /pages/{page_id}/blocksget_pageを再利用しpage.blocksを返す (find_blocks 経由は廃止)PUT / DELETE 系は副作用ありで境界が複雑なので
require_*_accessのまま 据え置き。C.
pool_pre_ping=False+pool_recycle=600db_connection.pypool_pre_ping=Trueだと checkout 毎にSELECT 1を発行して 1 RTT 消費していた。これを False に切替え、代わりにpool_recycle=600で 10 分超過した接続を自動破棄。Neon Pooler 側の切断やネットワーク中断は SQLAlchemy の自動 reconnect に任せる。期待効果
Singapore 環境 (1 RTT ≈ 160ms) で:
/blocks/{block_id}/pages/{page_id}(相当)/pages/{page_id}/blocks/trips/url/{url_id}/trips/{trip_id}pool_pre_ping=Falseでさらに ~160ms 分 (1 RTT) 削減見込み。ゴール p95 < 500ms に届く想定。動作確認
uv run ruff check/ruff format通過from app import mainで 20 routes が認識されて import 成功トレードオフ
pool_pre_ping=Falseで接続切断の検出が遅れる可能性あり。pool_recycle=600で 10 分超過は自動破棄するので軽減されるが、Neon Pooler 側の挙動次第。問題が出たらpool_pre_ping=Trueに戻す orpool_recycle短縮で対処require_block_access/require_page_access) は GET から呼ばれなくなり、PUT / DELETE 専用に。今後の追加エンドポイントは取得関数側で認可ロジックを持つ設計に揃える必要あり確認項目
Summary by CodeRabbit
Release Notes
Chores
Refactor