Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unreadテーブルにchannel_idカラムを追加 #1746

Merged
merged 22 commits into from
May 12, 2023

Conversation

logica0419
Copy link
Member

@logica0419 logica0419 commented Apr 5, 2023

resolve #1437 ?

#1737 にて、同条件でのクエリ実行時間が0.09秒→5秒となったことを受けて。
現在のテーブル構造ではどうクエリを効率化しても0.9秒程度になってしまい、前から言われていたレイテンシの問題が深刻化することが予想されるため、なんとかしようとした

  • 正規化から遠ざかってしまうが、JOINが無くなるので大幅な時間短縮が見込める
  • 不安点
    • これでクエリ実行時間がどうなるかは、デプロイしてみないとわからない
    • created_atを一意とみなしていいか大変悩みどころ

@logica0419 logica0419 added the kind/performance パフォーマンスの問題に関するもの label Apr 5, 2023
Copy link
Member

@ryoha000 ryoha000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

大体よさそう

model/messages.go Show resolved Hide resolved
@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

@logica0419

  • これ現状のデータでcreated_atが一意に定まるかどうかは確認しました?(正月とか毎朝07:00とか発生してもおかしくはない)
    • ↑複合だからそうそうないわ
    • (注)負荷がかかるのでバックアップデータから手元で行うように
  • primary keyにcreated_atを指定すると一般に得られる情報(ID)から一意に検索することが困難になりますがそこはどう考えていますか?

ちょっと今忙しいので代案についてはあとで考えます

@logica0419
Copy link
Member Author

logica0419 commented Apr 7, 2023

  • 上のに関しては、今後保証できないこともあって、昨日message_idをprimary indexの末尾につけるコミットをしました
  • 下のに関しては、現状議論の対象となるクエリとuser_id & channel_idでの検索しかunreadsテーブルを使うものが存在しない + foreign key constraintでchannel_idとmessage_idにindexが貼られるのでそこまで問題無いと考えています

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

なるほど、ありがとうございます。

僕もゆがんだデータ構造に対して忌避感を持っていて、その理由としてはメンテナンスコストがかなり増大するからです。

特に今回の場合、PRIMARY KEYが通常用いられる"一意に特定するためのキー"でなく"ソートするためのキー"(created_at)として用いられているのが僕の忌避間の原因だと考えています。

メンテナンスコストの面でいうと、PRIMARY KEYにcreated_atが存在するのが今回の場合ある種非自明で、今後(ほかの人が)この周辺の処理をいじる際に毎回このPRの経緯を見て理解する必要が出てくる(もしくはSQLを見て必要性を理解する)コストが発生するのが懸念点だと感じています。

ここに関しては速度とコストのトレードオフなのでどちらを取るかはお任せします。

僕の意見としては、数ms程度の遅延なら許容かなと考えています

補足:

PRIMARY KEYが通常用いられる"一意に特定するためのキー"でなく"ソートするためのキー"(created_at)

時系列データなど時間そのものがPRIMARYな属性を持っているような連続データな場合はその限りではないですが、今回はそうではないとの認識です

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

なんかちゃんと書いたら激怖文章になっちゃった、言葉固すぎ?

@logica0419
Copy link
Member Author

logica0419 commented Apr 7, 2023

いや、僕は慣れてるからいいよ

今までこの「気持ち悪さ」・「忌避感」が感覚でしか話されてなかったから、メンテナンスコストの話は納得感ありました、ありがとう
確かにprimary keyの意義が変わって理解しづらいのは一理ある
コメント書けば解決する部分はちょっとあるかもしれないけども根本解決にはならないし

2人から異論が唱えられたのもあって、だいぶ迷ってますねぇ

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

		SELECT 
			m.channel_id AS channel_id, 
			COUNT(m.id) AS count, 
			MAX(u.noticeable) AS noticeable, 
			MIN(m.created_at) AS since, 
			MAX(m.created_at) AS updated_at, 
			(
				SELECT message_id 
				FROM unreads u2 
				JOIN messages m2 ON u2.message_id = m2.id 
				WHERE u2.user_id = ? AND m2.channel_id = m.channel_id 
				ORDER BY m2.created_at ASC 
				LIMIT 1
			) AS oldest_message_id 
		FROM unreads u 
		JOIN messages m ON u.message_id = m.id 
		WHERE u.user_id = ? 
		GROUP BY m.channel_id;

これ内側から実行されるからGROUP_BYの前にサブクエリが実行されて、結果として全行に対してクエリが発行されるから遅くなってるのであって、分割したら早くなりそう

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

いやSelectのほうが後だから、GROUP_BYはかかってるのか?

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

サブクエリのm2.channel_id = m.channel_idがどうなるかで結構結果が変わる気がする

SQL筋不足です

@logica0419
Copy link
Member Author

logica0419 commented Apr 7, 2023

		SELECT 
			m.channel_id AS channel_id, 
			COUNT(m.id) AS count, 
			MAX(u.noticeable) AS noticeable, 
			MIN(m.created_at) AS since, 
			MAX(m.created_at) AS updated_at, 
			(
				SELECT message_id 
				FROM unreads u2
				WHERE user_id = ? AND created_at = MIN(u.created_at)
			) AS oldest_message_id 
		FROM unreads u 
		JOIN messages m ON u.message_id = m.id 
		WHERE u.user_id = ? 
		GROUP BY m.channel_id;

一応これで、今のmasterのコードの1/5の実行時間にはなりました
(このPRを出す前にうにゃうにゃした結果)

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

結構気になってるのはそのサブクエリ、GROUP_BYされた各行に対して実行されてる気がするから(最適化かかったらどうなるか知らないから曖昧なことしか言えない)
サブクエリじゃなくて分割してWINDOW関数にしたほうがいい気がする

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

早いのか動くのかもわからんけどとりあえず

  • JOINする行数を減らす(message tableがでかいので)
  • クエリ発行回数を減らす
SELECT 
    m.channel_id AS channel_id, 
    COUNT(m.id) AS count, 
    MAX(u.noticeable) AS noticeable, 
    MIN(m.created_at) AS since, 
    MAX(m.created_at) AS updated_at 
FROM (
    SELECT * FROM unreads WHERE unreads.user_id = ? 
) AS u
JOIN messages m ON u.message_id = m.id 
GROUP BY m.channel_id;
SELECT t.message_id, t.channel_id, t.created_at, ROW_NUMBER() OVER (PARTITION BY t.channel_id ORDER BY t.created_at ASC) AS row_num
FROM (
    SELECT u.channel_id AS channel_id, u.message_id AS message_id, m.created_at AS created_at FROM (
        SELECT * FROM unreads WHERE u.user_id = ? 
    ) AS u
    JOIN messages m ON u.message_id = m.id 
    WHERE m.channel_id IN (?)
) AS t
WHERE row_num=1;

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

WINDOW関数8.0空だったわ

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

でも普通に分割したほうがいいと思います

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

あとこれchannel_id単体にindexついてないけど大丈夫だっけ

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

pikachuくんのでもサブクエリ別にすれば普通に早くなると思います

@hijiki51
Copy link
Member

hijiki51 commented Apr 7, 2023

一クエリならサブクエリとメインクエリ入れ替えるとサブクエリ発行回数が一回になって早くなると思うけど普通に読みにくいし別呼び出しがいいと思う

@logica0419
Copy link
Member Author

メモ: 現時点でマイグレーションは手元45分

@logica0419
Copy link
Member Author

メモ: 現時点でマイグレーションは手元15分弱

Copy link
Member

@motoki317 motoki317 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

いくつか

repository/gorm/channel.go Outdated Show resolved Hide resolved
repository/gorm/message.go Outdated Show resolved Hide resolved
migration/v33.go Outdated Show resolved Hide resolved
docs/dbSchema/messages_stamps.md Show resolved Hide resolved
@logica0419 logica0419 requested a review from motoki317 May 12, 2023 01:55
migration/v33.go Outdated Show resolved Hide resolved
motoki317
motoki317 previously approved these changes May 12, 2023
Copy link
Member

@motoki317 motoki317 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrationの動作確認が取れていればよさそう

@logica0419 logica0419 merged commit dda8a0b into master May 12, 2023
5 checks passed
@logica0419 logica0419 deleted the add-chan-id-to-unread branch May 12, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/performance パフォーマンスの問題に関するもの
Projects
None yet
Development

Successfully merging this pull request may close these issues.

未読が多い場合に/api/v3/users/me/unreadが遅い
4 participants