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

Add an index and revise a select related to deleting all shots #3629

Merged
merged 7 commits into from
Oct 18, 2017
Merged

Add an index and revise a select related to deleting all shots #3629

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6dd5ea9
Remove two seq scans and a sort from a select query. (#3568)
chenba Oct 12, 2017
e793a04
Add index to column that's used in a WHERE. (#3568)
chenba Oct 12, 2017
00958db
Update db schema. (#3568)
chenba Oct 12, 2017
7bc28d3
Up the DB level. (#3568)
chenba Oct 12, 2017
2b35570
Merge branch 'master' into 3568-faster-deviceid-select
chenba Oct 13, 2017
ba5ec9d
Use acct id to get device ids; del img data for all device ids. (#3568)
chenba Oct 13, 2017
53129f7
Use device id when there's no account id to delete shots. (#3568)
chenba Oct 16, 2017
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
1 change: 1 addition & 0 deletions server/db-patches/patch-20-21.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CREATE INDEX images_shotid_idx ON images (shotid);
1 change: 1 addition & 0 deletions server/db-patches/patch-21-20.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP INDEX images_shotid_idx;
1 change: 1 addition & 0 deletions server/schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ ALTER TABLE ONLY states
ADD CONSTRAINT states_pkey PRIMARY KEY (state);
CREATE INDEX data_deviceid_idx ON data USING btree (deviceid);
CREATE INDEX devices_accountid_idx ON devices USING btree (accountid);
CREATE INDEX images_shotid_idx ON images USING btree (shotid);
CREATE INDEX searchable_text_idx ON data USING gin (searchable_text);
CREATE INDEX states_deviceid_idx ON states USING btree (deviceid);
ALTER TABLE ONLY data
Expand Down
2 changes: 1 addition & 1 deletion server/src/dbschema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const mozlog = require("./logging").mozlog("dbschema");

// When updating the database, please also run ./bin/dumpschema --record
// This updates schema.sql with the latest full database schema
const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 20;
const MAX_DB_LEVEL = exports.MAX_DB_LEVEL = 21;

exports.forceDbVersion = function(version) {
mozlog.info("forcing-db-version", {db: db.constr, version});
Expand Down
2 changes: 1 addition & 1 deletion server/src/pages/leave-screenshots/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ app.post("/leave", function(req, res) {
if (!req.deviceId) {
res.status(403).send(req.getText("leavePageErrorAddonRequired"));
}
Shot.deleteEverythingForDevice(req.backend, req.deviceId).then(() => {
Shot.deleteEverythingForDevice(req.backend, req.deviceId, req.accountId).then(() => {
res.redirect("/leave-screenshots/?complete");
}).catch((e) => {
mozlog.error("delete-account-error", {msg: "An error occurred trying to delete account", error: e});
Expand Down
74 changes: 46 additions & 28 deletions server/src/servershot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,42 +620,60 @@ Shot.deleteShot = function(backend, shotId, deviceId, accountId) {
})
};

Shot.deleteEverythingForDevice = function(backend, deviceId) {
return db.select(
`SELECT images.id
FROM images JOIN data
ON images.shotid = data.id
WHERE data.deviceid = $1`,
[deviceId]
).then((rows) => {
rows.forEach((row) => del(row.id))
}
).then(() => {
return db.select(
`SELECT DISTINCT devices.id
FROM devices, devices AS devices2
Shot.deleteEverythingForDevice = function(backend, deviceId, accountId) {
let deviceIdsSelect, deviceIds = [];

if (accountId) {
deviceIdsSelect = db.select(
`SELECT devices.id
FROM devices
WHERE devices.accountid = $1`,
[accountId]);
} else {
deviceIdsSelect = db.select(
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case we know exactly what the deviceIds will be: [deviceId]. So probably what it should be is something like:

if (accountId) {
  deviceIdsSelect = db.select(...).then((rows) => {
    return rows.map((row) => row.deviceId);
  });
} else {
  deviceIdsSelect = Promise.resolve([deviceId]);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to clarify/confirm: this changes the previous behavior when shots of one device is deleted, if the device is associated to an account (but not authenticated thus no accountId), then the shots of the other devices that are associated to that account will also be deleted. After this patch, only the shots of the given device will be deleted.

Copy link
Contributor

@ianb ianb Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Really? That doesn't seem like what I'm reading here. This query seems to get images from all devices:

return db.select(
     `SELECT images.id
      FROM images JOIN data
      ON images.shotid = data.id
      WHERE data.deviceid IN (${db.markersForArgs(1, deviceIds.length)})`,
     deviceIds);

Or at least all devices listed in deviceIds...?

Note the change I'm proposing is that when accountId is null (and so there are no other devices), then we just use deviceIds = [deviceId] (more or less)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you are reading that query correctly. But if we use deviceIds = [deviceId], then there's only one device id in IN. Currently, the device ids come from https://github.com/mozilla-services/screenshots/blob/master/server/src/servershot.js#L634 regardless of the user's authentication state, and that could returns multiple device ids.

Copy link
Contributor

Choose a reason for hiding this comment

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

If accountId is null then devices.accountid is also null, and there aren't any associated devices. At login or account connect time we set accountid, so we can trust that if it's null then there aren't any other associated deviceIds.

`SELECT devices.id
FROM devices
WHERE devices.id = $1
OR (devices.accountid = devices2.accountid
AND devices2.id = $1)
`,

UNION ALL

SELECT devices.id
FROM devices, devices AS devices2
WHERE devices.accountid = devices2.accountid
AND devices2.id = $1`,
[deviceId]);
}

const imageIdsSelect = (deviceIdRows) => {
deviceIdRows.forEach(row => deviceIds.push(row.id));
if (!deviceIds.length) {
deviceIds = [deviceId];
}
).then((rows) => {
let ids = [];
for (let i = 0; i < rows.length; i++) {
ids.push(rows[i].id);
}
if (!ids.length) {
ids = [deviceId];
}
return db.select(
`SELECT images.id
FROM images JOIN data
ON images.shotid = data.id
WHERE data.deviceid IN (${db.markersForArgs(1, deviceIds.length)})`,
deviceIds);
};

const deleteImageData = (imageIdRows) => {
imageIdRows.forEach(row => del(row.id));
};

const deleteShotRecords = () => {
let deleteSql = `DELETE FROM data WHERE
deviceid IN (${db.markersForArgs(1, ids.length)})`;
deviceid IN (${db.markersForArgs(1, deviceIds.length)})`;
return db.update(
deleteSql,
ids
deviceIds
);
}

});
return deviceIdsSelect
.then(imageIdsSelect)
.then(deleteImageData)
.then(deleteShotRecords);
};

ClipRewrites = class ClipRewrites {
Expand Down