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.

Sign up for GitHub

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

Jump to bottom

perf: decouple user used storage sql function #2069

Merged
merged 1 commit into from
Nov 1, 2022
Merged

perf: decouple user used storage sql function #2069

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 packages/api/test/user.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('GET /user/account', () => {
assert(res.ok)
const data = await res.json()
assert.strictEqual(data.usedStorage.uploaded, 32000)
assert.strictEqual(data.usedStorage.psaPinned, 10000)
assert.strictEqual(data.usedStorage.psaPinned, 710000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed updated as now we are counting fixtures that were not pinned status from https://github.com/web3-storage/web3.storage/blob/main/packages/api/test/fixtures/init-data.sql#L156-L167

})
})

Expand Down
30 changes: 20 additions & 10 deletions packages/db/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {
normalizePins,
normalizeDeals,
normalizePsaPinRequest,
parseTextToNumber
parseTextToNumber,
safeNumber
} from './utils.js'
import { ConstraintError, DBError, RangeNotSatisfiableDBError } from './errors.js'

Expand Down Expand Up @@ -316,19 +317,28 @@ export class DBClient {
* @returns {Promise<import('./db-client-types').StorageUsedOutput>}
*/
async getStorageUsed (userId) {
/** @type {{ data: { uploaded: string, psa_pinned: string, total: string }, error: PostgrestError }} */
const { data, error } = await this._client
.rpc('user_used_storage', { query_user_id: userId })
.single()
const [userUploadedResponse, psaPinnedResponse] = await Promise.all([
this._client
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale behind splitting the 2 up?
Is it to better understand where the bottleneck is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Goal is to decrease the request time to be less likely of getting to the 30s timeout of heroku dyno. With Alan's account that has a lot of data this was taking so much.

Also, we were talking about having a view per product usage, and we will be able to perform queries specific for each thing.

.rpc('user_uploaded_storage', { query_user_id: userId })
.single(),
this._client
.rpc('user_psa_storage', { query_user_id: userId })
.single()
])

if (error) {
throw new DBError(error)
if (userUploadedResponse.error) {
throw new DBError(userUploadedResponse.error)
} else if (psaPinnedResponse.error) {
throw new DBError(psaPinnedResponse.error)
}

const uploaded = parseTextToNumber(userUploadedResponse.data)
const psaPinned = parseTextToNumber(psaPinnedResponse.data)

return {
uploaded: parseTextToNumber(data.uploaded),
psaPinned: parseTextToNumber(data.psa_pinned),
total: parseTextToNumber(data.total)
uploaded,
psaPinned,
total: safeNumber(uploaded + psaPinned)
}
}

Expand Down
46 changes: 33 additions & 13 deletions packages/db/postgres/functions.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ DROP FUNCTION IF EXISTS create_key;
DROP FUNCTION IF EXISTS create_upload;
DROP FUNCTION IF EXISTS upsert_pin;
DROP FUNCTION IF EXISTS upsert_pins;
DROP FUNCTION IF EXISTS user_uploaded_storage;
DROP FUNCTION IF EXISTS user_psa_storage;
DROP FUNCTION IF EXISTS user_used_storage;
DROP FUNCTION IF EXISTS user_auth_keys_list;
DROP FUNCTION IF EXISTS find_deals_by_content_cids;
Expand Down Expand Up @@ -225,20 +227,15 @@ BEGIN
END
$$;

CREATE TYPE stored_bytes AS (uploaded TEXT, psa_pinned TEXT, total TEXT);

CREATE OR REPLACE FUNCTION user_used_storage(query_user_id BIGINT)
RETURNS stored_bytes
CREATE OR REPLACE FUNCTION user_uploaded_storage(query_user_id BIGINT)
RETURNS text
LANGUAGE plpgsql
AS
$$
DECLARE
used_storage stored_bytes;
uploaded BIGINT;
psa_pinned BIGINT;
total BIGINT;
BEGIN
uploaded :=
total :=
(
SELECT COALESCE((
SELECT SUM(dag_size)
Expand All @@ -247,16 +244,25 @@ BEGIN
c.dag_size
FROM upload u
JOIN content c ON c.cid = u.content_cid
JOIN pin p ON p.content_cid = u.content_cid
WHERE u.user_id = query_user_id::BIGINT
AND u.deleted_at is null
AND p.status = 'Pinned'
GROUP BY c.cid,
c.dag_size
) AS uploaded_content), 0)
);
return (total)::TEXT;
END
$$;

psa_pinned :=
CREATE OR REPLACE FUNCTION user_psa_storage(query_user_id BIGINT)
RETURNS text
LANGUAGE plpgsql
AS
$$
DECLARE
total BIGINT;
BEGIN
total :=
(
SELECT COALESCE((
SELECT SUM(dag_size)
Expand All @@ -265,16 +271,30 @@ BEGIN
c.dag_size
FROM psa_pin_request psa_pr
JOIN content c ON c.cid = psa_pr.content_cid
JOIN pin p ON p.content_cid = psa_pr.content_cid
JOIN auth_key a ON a.id = psa_pr.auth_key_id
WHERE a.user_id = query_user_id::BIGINT
AND psa_pr.deleted_at is null
AND p.status = 'Pinned'
GROUP BY psa_pr.content_cid,
c.dag_size
) AS pinned_content), 0)
);
return (total)::TEXT;
END
$$;

CREATE TYPE stored_bytes AS (uploaded TEXT, psa_pinned TEXT, total TEXT);

CREATE OR REPLACE FUNCTION user_used_storage(query_user_id BIGINT)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping this function because users_by_storage_used uses it. So, just refactored to re-use partial functions created

RETURNS stored_bytes
LANGUAGE plpgsql
AS
$$
DECLARE
used_storage stored_bytes;
uploaded BIGINT := (user_uploaded_storage(query_user_id))::BIGINT;
psa_pinned BIGINT := (user_psa_storage(query_user_id))::BIGINT;
total BIGINT;
BEGIN
total := uploaded + psa_pinned;

SELECT uploaded::TEXT,
Expand Down
81 changes: 81 additions & 0 deletions packages/db/postgres/migrations/030-decouple-user-used-storage.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
CREATE INDEX CONCURRENTLY IF NOT EXISTS psa_pin_request_search_idx ON psa_pin_request (auth_key_id) INCLUDE (content_cid, deleted_at);

DROP INDEX IF EXISTS psa_pin_request_content_cid_idx;
DROP INDEX IF EXISTS psa_pin_request_deleted_at_idx;

CREATE OR REPLACE FUNCTION user_uploaded_storage(query_user_id BIGINT)
RETURNS text
LANGUAGE plpgsql
AS
$$
DECLARE
total BIGINT;
BEGIN
total :=
(
SELECT COALESCE((
SELECT SUM(dag_size)
FROM (
SELECT c.cid,
c.dag_size
FROM upload u
JOIN content c ON c.cid = u.content_cid
WHERE u.user_id = query_user_id::BIGINT
AND u.deleted_at is null
GROUP BY c.cid,
c.dag_size
) AS uploaded_content), 0)
);
return (total)::TEXT;
END
$$;

CREATE OR REPLACE FUNCTION user_psa_storage(query_user_id BIGINT)
RETURNS text
LANGUAGE plpgsql
AS
$$
DECLARE
total BIGINT;
BEGIN
total :=
(
SELECT COALESCE((
SELECT SUM(dag_size)
FROM (
SELECT psa_pr.content_cid,
c.dag_size
FROM psa_pin_request psa_pr
JOIN content c ON c.cid = psa_pr.content_cid
JOIN auth_key a ON a.id = psa_pr.auth_key_id
WHERE a.user_id = query_user_id::BIGINT
AND psa_pr.deleted_at is null
GROUP BY psa_pr.content_cid,
c.dag_size
) AS pinned_content), 0)
);
return (total)::TEXT;
END
$$;

CREATE OR REPLACE FUNCTION user_used_storage(query_user_id BIGINT)
RETURNS stored_bytes
LANGUAGE plpgsql
AS
$$
DECLARE
used_storage stored_bytes;
uploaded BIGINT := (user_uploaded_storage(query_user_id))::BIGINT;
psa_pinned BIGINT := (user_psa_storage(query_user_id))::BIGINT;
total BIGINT;
BEGIN
total := uploaded + psa_pinned;

SELECT uploaded::TEXT,
psa_pinned::TEXT,
total::TEXT
INTO used_storage;

return used_storage;
END
$$;
3 changes: 1 addition & 2 deletions packages/db/postgres/tables.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,7 @@ CREATE TABLE IF NOT EXISTS psa_pin_request
updated_at TIMESTAMP WITH TIME ZONE DEFAULT timezone('utc'::text, now()) NOT NULL
);

CREATE INDEX IF NOT EXISTS psa_pin_request_content_cid_idx ON psa_pin_request (content_cid);
CREATE INDEX IF NOT EXISTS psa_pin_request_deleted_at_idx ON psa_pin_request (deleted_at) INCLUDE (content_cid, auth_key_id);
CREATE INDEX IF NOT EXISTS psa_pin_request_search_idx ON psa_pin_request (auth_key_id) INCLUDE (content_cid, deleted_at);

CREATE TABLE IF NOT EXISTS agreement (
id BIGSERIAL PRIMARY KEY,
Expand Down
19 changes: 1 addition & 18 deletions packages/db/test/user.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-env mocha, browser */
import assert from 'assert'
import { DBClient } from '../index.js'
import { createUpload, initialPinsNotPinned, pinsError, randomCid, token } from './utils.js'
import { createUpload, token } from './utils.js'

describe('user operations', () => {
const name = 'test-name'
Expand Down Expand Up @@ -183,23 +183,6 @@ describe('user operations', () => {
dagSize: dagSize2
})

// Create "Failed" Upload. It should not be counted.
const cid3 = await randomCid()
const dagSize3 = 100000
await createUpload(client, user._id, authKey._id, cid3, {
dagSize: dagSize3,
pins: pinsError
})

// Create Upload not pinned yet. It should not be counted yet.
await createUpload(client, user._id, authKey._id, await randomCid(), {
dagSize: 1000000,
pins: initialPinsNotPinned
})

const usedStorageWithFailed = await client.getStorageUsed(user._id)
assert.strictEqual(usedStorageWithFailed.uploaded, dagSize1 + dagSize2, 'used storage should not count unpinned')

Comment on lines -186 to -202
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These would be accounted now, so really no need to add them given there is no exception anymore

const secondUsedStorage = await client.getStorageUsed(user._id)
assert.strictEqual(secondUsedStorage.uploaded, dagSize1 + dagSize2, 'used storage with second upload')

Expand Down
24 changes: 0 additions & 24 deletions packages/db/test/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,37 +127,13 @@ export async function createUserWithFiles (dbClient, options = {}) {
const pinRequests = 3
const dagSize = Math.ceil(((percentStorageUsed / 100) * storageQuota) / (uploads + pinRequests))

// Create a failed upload.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failed + Not pinned are now counted too

await createUpload(dbClient, Number(user._id), Number(authKey), await randomCid(), {
dagSize,
pins: pinsError
})

// Create a yet to be pinned upload.
await createUpload(dbClient, Number(user._id), Number(authKey), await randomCid(), {
dagSize,
pins: initialPinsNotPinned
})

for (let i = 0; i < uploads; i++) {
const cid = await randomCid()
await createUpload(dbClient, Number(user._id), Number(authKey), cid, {
dagSize
})
}

// Create a failed PinRequest.
await createPsaPinRequest(dbClient, authKey, await randomCid(), {
dagSize,
pins: pinsError
})

// Create a yet to be pinned PinRequest.
await createPsaPinRequest(dbClient, authKey, await randomCid(), {
dagSize,
pins: initialPinsNotPinned
})

for (let i = 0; i < pinRequests; i++) {
const cid = await randomCid()
await createPsaPinRequest(dbClient, authKey, cid, {
Expand Down
10 changes: 10 additions & 0 deletions packages/db/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,13 @@ export function parseTextToNumber (numberText) {
}
return num
}

/**
* @param {Number} num
*/
export function safeNumber (num) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit.

We could call this function from parseTextToNumber.

Copy link
Member

Choose a reason for hiding this comment

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

This is not parsing anything - just ensuring it's within the safe integer range.

Maybe validateSafeInteger?

Copy link
Contributor

@flea89 flea89 Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

what I meant is

export function parseTextToNumber (numberText) {
  const num = Number(numberText)
  validateSafeInteger(num)
  return num
}

/**
 * @param {Number} num
 */
export function validateSafeInteger (num) {
  if (!Number.isSafeInteger(num)) {
    throw new Error('Invalid integer number.')
  }
  return num
}

to avoid duplicating the validation in 2 functions and making sure for instance we're raising a consistent Error.

Really minor ;)

if (!Number.isSafeInteger(num)) {
throw new Error('Invalid integer number.')
}
return num
}