Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Simplify retrieval of My Shots data, hopefully speeding things up #3374

Merged
merged 1 commit into from
Aug 18, 2017
Merged

Simplify retrieval of My Shots data, hopefully speeding things up #3374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/pages/shotindex/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ app.get("/", csrf({cookie: true}), function(req, res) {
let query = req.query.q || null;
let getShots = Promise.resolve(null);
if (req.deviceId && req.query.withdata) {
getShots = Shot.getShotsForDevice(req.backend, req.deviceId, query);
getShots = Shot.getShotsForDevice(req.backend, req.deviceId, req.accountId, query);
}
getShots.then(_render)
.catch((err) => {
Expand Down
11 changes: 5 additions & 6 deletions server/src/servershot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,18 +459,17 @@ Shot.checkOwnership = function(shotId, deviceId, accountId) {
})
};

Shot.getShotsForDevice = function(backend, deviceId, searchQuery) {
Shot.getShotsForDevice = function(backend, deviceId, accountId, searchQuery) {
if (!deviceId) {
throw new Error("Empty deviceId: " + deviceId);
}
// accountId is null if not set, treated as NULL in the SQL query
return db.select(
`SELECT DISTINCT devices.id
FROM devices, devices AS devices2
WHERE devices.id = $1
OR (devices.accountid = devices2.accountid
AND devices2.id = $1)
FROM devices
WHERE devices.id = $1 OR devices.accountid = $2
`,
[deviceId]
[deviceId, accountId]
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess technically if accountId is null (as it is when you aren't logged in), then devices.accountid = NULL will always be false (I guess that's just SQL doing what it's supposed to do), but that always seemed like an unusual corner of SQL. At least worth a comment so a later reader who isn't aware of how NULL is handled will realize the code works as it should.

We could probably go further than this, and do a join later on and do the whole thing in one query... but that might actually reintroduce the performance issue.

Should we also add CREATE INDEX devices_accountid_idx ON devices (accountid) ? Though we could batch up a bunch of these indexes later tool.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess technically if accountId is null (as it is when you aren't logged in), then devices.accountid = NULL will always be false (I guess that's just SQL doing what it's supposed to do), but that always seemed like an unusual corner of SQL. At least worth a comment so a later reader who isn't aware of how NULL is handled will realize the code works as it should.

Sure, I'll add a note here.

We could probably go further than this, and do a join later on and do the whole thing in one query... but that might actually reintroduce the performance issue.

Yeah, good point. Maybe once we're sure there aren't other sequential scans lurking elsewhere, we can focus on reducing the number of round trips to the database.

Should we also add CREATE INDEX devices_accountid_idx ON devices (accountid) ? Though we could batch up a bunch of these indexes later tool.

The index already exists, but wasn't getting used. I was thinking of adding lookup tables (device to account, and account to device), to provide a hint to the query planner, but this SO answer suggests using compound indexes to do the same job (account + device and device + account indexes).

That same SO answer points out that varchar is not the most efficient data type for indexes or sorting, which is something else to maybe consider in the future.

).then((rows) => {
searchQuery = searchQuery || null;
let ids = [];
Expand Down