Use more efficient query #45
Conversation
| @@ -258,8 +258,11 @@ func listPhotos(tx *sql.Tx, userID int, photoIDs *[][]byte) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| const selectSQL = ` | |||
| SELECT id, caption, commentCount, latitude, longitude, timestamp FROM photos WHERE userID = $1 ORDER BY timestamp DESC LIMIT 100` | |||
| const selectSQL = `SELECT commentID, userID, message, timestamp FROM comments ` + | |||
There was a problem hiding this comment.
This seems to be in the wrong spot. You're selecting comments here not photos. Did you mean this to be in the listComments ,method?
There was a problem hiding this comment.
Did you change the correct SELECT? The original was FROM photos while this one is FROM comments.
There was a problem hiding this comment.
ah, weak search-fu. Let me put this in the right spot.
There was a problem hiding this comment.
See my new comment on cockroachdb/cockroach#4925. I think we could change the schema instead of the query.
There was a problem hiding this comment.
I like Peter's suggestion, that would be even better. If the index stores all the needed columns (commentID, userID, message, timestamp), it should work.
The change LGTM though if you want to try this first.
There was a problem hiding this comment.
I'm happy if someone wants to take it over - I've got some loose ends to tie up. @RaduBerinde, would you mind?
There was a problem hiding this comment.
BTW Specifically Peter's suggestion is to change commentsByPhotoID to be:
CREATE UNIQUE INDEX IF NOT EXISTS commentsByPhotoID ON comments (photoID, timestamp) STORING (commentID, userID, message);
Verified locally this would do the right thing:
+-------+---------+------------------------------------+
| Level | Type | Description |
+-------+---------+------------------------------------+
| 0 | revscan | comments@commentsByPhotoID /10-/11 |
+-------+---------+------------------------------------+
But this means we will be storing each message twice. So I'm no longer sure which one is better :)
There was a problem hiding this comment.
I'm about to head out and don't know much about testing/running this (I don't even have this repo cloned). I think it's fine if you merge this as-is for now.
There was a problem hiding this comment.
We could also dump the commentsByPhotoID index entirely and change the primary key of the comments table to PRIMARY KEY (photoID, timestamp, commentID).
|
@petermattis If you'd like to take this over, please go ahead - otherwise, I'm ready for an LGTM :-) |
See cockroachdb/cockroach#4925 (comment)
This change is