Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

Conversation

mfbx9da4
Copy link
Contributor

@mfbx9da4 mfbx9da4 commented Oct 8, 2021

Now you can do

const result = db.all(sql`SELECT * FROM data WHERE stock IN ${['a', 'b', 'c']} LIMIT 1`)

Also added a prettier config matching current formatting.

@leafac
Copy link
Owner

leafac commented Oct 8, 2021

Thanks for the pull request!

I’ve been thinking about this issue for a while, and I still don’t have a good answer. I like the approach you propose because it’s transparent and natural, but it may lead the prepared statement cache to blow up. Maybe we should just add a warning explaining the situation in the documentation?

Also, I use Prettier without tweaking any settings—why do you think the .prettierrc.json is necessary?

@mfbx9da4
Copy link
Contributor Author

mfbx9da4 commented Oct 8, 2021

RE prettier
I have a default prettier config in my dotfiles for my machine which is different to default prettier so it's necessary to have one for this project for me to be able to contribute effectively.

@mfbx9da4
Copy link
Contributor Author

mfbx9da4 commented Oct 8, 2021

The cache size growing out of control is a gotcha of leafac/sqlite in general and most of the time is not an issue. I would treat it as a separate problem to array parameters and suggest a few things

  1. Add a default statement cache size of 2000? Convention over configuration. 2000 seems big enough to cover a lot of applications and unlikely to ever be too big to have an effect on the process. Even if there is a constant cache miss due to this estimate cache size, creating a statement each time is probably not a huge performance hit when compared to query time. Afterall, auto caching the statements feels like a nice-to-have optimization. Better to have a slight performance degradation than a process crash due to a memory leak.
  2. Allow the user to configure statementCacheSize: number might be nice to be able to tweak dynamically.
  3. Allow the user to configure a handler in the case that the statementCache.size has reached the max size. onStatementCacheSizeFull: (cache: StatementCache) => void. The user can then look at the offending statements and dig into the documentation around statement caching and proposed solutions for managing it.
  4. There should be an escape hatch to avoid caching the statement e.g. db.all(sql...., { cacheStatement: false })
  5. The cache should be an LRU cache to avoid the round robin constant cache miss scenario as much as possible. (ES6 Map is already an LRU cache out of the box since it preserves insertion order, so all you need to do is statements.set on every statements.get and then prune in iteration order when max size reached). You could have a frequency+recency cache but meh over engineered?
  6. Add more documentation.

In the situation where the user wants to cache the statements but wants control over purging the cache you could do

db.all(sql`...`)
db.statementCache.clear() // clears the entire statement cache

db.all(sql`...`, { statementCacheNamespace: 'foo' })
db.statementCache.clear('foo') // clears the cache for only statements tagged with namespace foo 

This last one seems a bit overkill but just an idea. As it might be useful to cache statements on a per request basis, 'foo' might be the requestId for example.

Also good to note that the suggested approach of caching based on size of the array parameter length will work out of the box with this implementation.

@mfbx9da4
Copy link
Contributor Author

mfbx9da4 commented Oct 8, 2021

Also I realize you didn't ask for this PR so no worries if you're not interested in it's contents.

@leafac
Copy link
Owner

leafac commented Oct 9, 2021

You make excellent points. I’ll work on this at some point in the near future. Thank you very much!

Also I realize you didn't ask for this PR so no worries if you're not interested in it's contents.

Oh, I’m merging this, for sure!

@leafac leafac merged commit b80a82f into leafac:main Nov 10, 2021
@leafac
Copy link
Owner

leafac commented Nov 10, 2021

I merged this and published as part of version 2.0.0 (major bump because of move to ESM). Thank you very much for the pull request!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants