Nothing Special   »   [go: up one dir, main page]
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

Upsert in SQLiteStore #3867

Open
mcollina opened this issue Nov 22, 2024 · 5 comments
Open

Upsert in SQLiteStore #3867

mcollina opened this issue Nov 22, 2024 · 5 comments

Comments

@mcollina
Copy link
Member
mcollina commented Nov 22, 2024 •
edited
Loading

As pointed out by @ronag in #3657, we can improve our SQLite store design by using a composite cache key as the ID of the table instead of an auto increment field.

One specific problem of our SQLiteStore design is that on reading the cache, we read all the values for a given route, independently of the vary headers:

undici/lib/cache/sqlite-cache-store.js

Lines 375 to 421 in e49b575

#findValue (key, canBeExpired = false) {
const url = this.#makeValueUrl(key)
/**
* @type {SqliteStoreValue[]}
*/
const values = this.#getValuesQuery.all(url, key.method)
if (values.length === 0) {
// No responses, let's just return early
return undefined
}
const now = Date.now()
for (const value of values) {
if (now >= value.deleteAt && !canBeExpired) {
this.#deleteExpiredValuesQuery.run(now)
return undefined
}
let matches = true
if (value.vary) {
if (!key.headers) {
// Request doesn't have headers so it can't fulfill the vary
// requirements no matter what, let's return early
return undefined
}
value.vary = JSON.parse(value.vary)
for (const header in value.vary) {
if (key.headers[header] !== value.vary[header]) {
matches = false
break
}
}
}
if (matches) {
return value
}
}
return undefined
}
}
.

For similar reasons in writing, we need to first read the cache, find if we have a matching row, and the go update that.

undici/lib/cache/sqlite-cache-store.js

Lines 276 to 315 in e49b575

final (callback) {
if (body === null) {
return callback()
}
/**
* @type {SqliteStoreValue | undefined}
*/
const existingValue = findValue(key, true)
if (existingValue) {
// Updating an existing response, let's delete it
updateValueQuery.run(
JSON.stringify(stringifyBufferArray(body)),
value.deleteAt,
value.statusCode,
value.statusMessage,
value.rawHeaders ? JSON.stringify(stringifyBufferArray(value.rawHeaders)) : null,
value.etag,
value.cachedAt,
value.staleAt,
value.deleteAt,
existingValue.id
)
} else {
// New response, let's insert it
insertValueQuery.run(
url,
key.method,
JSON.stringify(stringifyBufferArray(body)),
value.deleteAt,
value.statusCode,
value.statusMessage,
value.rawHeaders ? JSON.stringify(stringifyBufferArray(value.rawHeaders)) : null,
value.etag ? value.etag : null,
value.vary ? JSON.stringify(value.vary) : null,
value.cachedAt,
value.staleAt,
value.deleteAt
)
}

We should be able to fix both problems at the same time by using a primary key that includes the vary headers, so we can avoid loading everything at once during getting.

For the algorithm to compute the key, check #3657 (comment).
@ronag
Copy link
Member
ronag commented Nov 22, 2024

I'm not sure we can solve the first problem (since we don't know the vary headers in advance). But certainly the second.

@metcoder95
Copy link
Member
metcoder95 commented Nov 22, 2024 •
edited
Loading

Agree with @ronag, the vary header will help for validation/invalidation; unless we do a HEAD request first to get the header (assuming servers comply), we cannot use it for a preemptive cache lookup as we don't know them beforehand.

@mcollina
Copy link
Member Author

I'm not sure we can solve the first problem (since we don't know the vary headers in advance).

I'm probably missing something then. Can you clarify why you wouldn't have the vary headers?

@ronag
Copy link
Member
ronag commented Nov 22, 2024 •
edited
Loading

Can you clarify why you wouldn't have the vary headers?

The vary header is part of the response, not the request.

@ronag
Copy link
Member
ronag commented Nov 22, 2024

The current implementation is actually a bit broken since a more specific response (more vary dependencies) could overwrite a non stale and less specific response. But I don't think that's relevant in practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcollina @ronag @metcoder95