feat: blog post about caching count(*) #2674
Conversation
chronark
requested review from
perkinsjr,
MichaelUnkey and
mcstepp
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughA new blog post titled "Approximating Row Counts" has been added to the project, specifically in the file Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Database
User->>API: Request key count
API->>Database: Check last updated timestamp
alt If stale data
Database->>Database: Execute query to count keys
Database-->>API: Return updated count
else
Database-->>API: Return cached count
end
API-->>User: Return key count
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/www/content/blog/approximating-row-counts.mdx (4)
37-37: Fix typo in textChange "out query" to "our query"
-As you can see, many `keys` belong to a single `key_space` and out query in question is: +As you can see, many `keys` belong to a single `key_space` and our query in question is:🧰 Tools
🪛 LanguageTool
[uncategorized] ~37-~37: “out” (outside) seems less likely than “our” (belonging to us).
Context: ...eysbelong to a singlekey_space` and out query in question is: ```sql SELECT cou...(AI_HYDRA_LEO_CP_OUT_OUR)
39-40: Consider adding index recommendation for performanceThe
count(*)query would benefit from an index onkey_space_id. Consider adding a note about the importance of proper indexing for performance optimization.CREATE INDEX idx_keys_key_space_id ON keys(key_space_id);
79-88: Consider adding optimistic lockingThe asynchronous update could lead to race conditions if multiple requests try to update the count simultaneously.
Consider adding optimistic locking:
c.executionCtx.waitUntil( db.primary .update(schema.keySpace) .set({ sizeApprox: keySpace.sizeApprox, sizeLastUpdatedAt: keySpace.sizeLastUpdatedAt, }) - .where(eq(schema.keySpace.id, keySpace.id)), + .where( + and( + eq(schema.keySpace.id, keySpace.id), + eq(schema.keySpace.sizeLastUpdatedAt, keySpace.sizeLastUpdatedAt - 60_000) + ) + ), );
94-94: Consider implementing the background refresh nowWhile the current solution works, implementing a background refresh job would provide more consistent performance and reduce the likelihood of multiple concurrent count updates.
Would you like me to help design a background job implementation using a cron-based approach? This could include:
- Efficient batch processing of stale counts
- Error handling and retry mechanisms
- Monitoring and alerting setup
🧰 Tools
🪛 LanguageTool
[typographical] ~94-~94: After an expression of time at the start of a sentence, a comma is usually used.
Context: ...where we always count all rows. In the future we might want to run a cron job to refr...(IN_THE_PAST_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
apps/www/public/images/blog-images/covers/approximating-row-counts.pngis excluded by!**/*.png
📒 Files selected for processing (1)
apps/www/content/blog/approximating-row-counts.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/www/content/blog/approximating-row-counts.mdx
[uncategorized] ~37-~37: “out” (outside) seems less likely than “our” (belonging to us).
Context: ...eysbelong to a singlekey_space` and out query in question is: ```sql SELECT cou...
(AI_HYDRA_LEO_CP_OUT_OUR)
[uncategorized] ~44-~44: Possible missing preposition found.
Context: ...tions We were looking at a few options how to fix this: 1. Caching the count as p...
(AI_EN_LECTOR_MISSING_PREPOSITION)
[uncategorized] ~53-~53: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...r dashboard just as easily as in our API and it behaves the same. Adding these two ...
(COMMA_COMPOUND_SENTENCE)
[grammar] ~62-~62: Did you mean “because”?
Context: ...space` table, we get the count for free cause we're not doing an extra query. To keep...
(CAUSE_BECAUSE)
[uncategorized] ~65-~65: You might be missing the article “the” here.
Context: ...asynchronously. Here's how we do it in drizzle: ```ts const keySpace = await db.quer...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~92-~92: The phrase ‘a lot of’ might be wordy and overused. Consider using an alternative.
Context: ...ght kick off many queries to refresh if a lot of requests come in at the same time, but ...
(A_LOT_OF)
[typographical] ~94-~94: After an expression of time at the start of a sentence, a comma is usually used.
Context: ...where we always count all rows. In the future we might want to run a cron job to refr...
(IN_THE_PAST_COMMA)
| if (keySpace.sizeLastUpdatedAt < Date.now() - 60_000) { | ||
| const count = await db | ||
| .select({ count: sql<string>`count(*)` }) | ||
| .from(schema.keys) | ||
| .where(and(eq(schema.keys.keySpaceId, keySpace.id), isNull(schema.keys.deletedAt))); | ||
|
|
||
| keySpace.sizeApprox = Number.parseInt(count?.at(0)?.count ?? "0"); | ||
| keySpace.sizeLastUpdatedAt = Date.now(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve count parsing
The current implementation has several areas that could be improved:
- Missing error handling for the count query
- The count parsing could fail if the result is undefined
- Potential race condition if multiple requests update the count simultaneously
Consider this more robust implementation:
if (keySpace.sizeLastUpdatedAt < Date.now() - 60_000) {
- const count = await db
- .select({ count: sql<string>`count(*)` })
- .from(schema.keys)
- .where(and(eq(schema.keys.keySpaceId, keySpace.id), isNull(schema.keys.deletedAt)));
-
- keySpace.sizeApprox = Number.parseInt(count?.at(0)?.count ?? "0");
- keySpace.sizeLastUpdatedAt = Date.now();
+ try {
+ const count = await db
+ .select({ count: sql<string>`count(*)` })
+ .from(schema.keys)
+ .where(and(eq(schema.keys.keySpaceId, keySpace.id), isNull(schema.keys.deletedAt)));
+
+ const parsedCount = Number.parseInt(count?.[0]?.count);
+ if (Number.isNaN(parsedCount)) {
+ console.error('Failed to parse count result:', count);
+ return;
+ }
+
+ keySpace.sizeApprox = parsedCount;
+ keySpace.sizeLastUpdatedAt = Date.now();
+ } catch (error) {
+ console.error('Failed to update key count:', error);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (keySpace.sizeLastUpdatedAt < Date.now() - 60_000) { | |
| const count = await db | |
| .select({ count: sql<string>`count(*)` }) | |
| .from(schema.keys) | |
| .where(and(eq(schema.keys.keySpaceId, keySpace.id), isNull(schema.keys.deletedAt))); | |
| keySpace.sizeApprox = Number.parseInt(count?.at(0)?.count ?? "0"); | |
| keySpace.sizeLastUpdatedAt = Date.now(); | |
| if (keySpace.sizeLastUpdatedAt < Date.now() - 60_000) { | |
| try { | |
| const count = await db | |
| .select({ count: sql<string>`count(*)` }) | |
| .from(schema.keys) | |
| .where(and(eq(schema.keys.keySpaceId, keySpace.id), isNull(schema.keys.deletedAt))); | |
| const parsedCount = Number.parseInt(count?.[0]?.count); | |
| if (Number.isNaN(parsedCount)) { | |
| console.error('Failed to parse count result:', count); | |
| return; | |
| } | |
| keySpace.sizeApprox = parsedCount; | |
| keySpace.sizeLastUpdatedAt = Date.now(); | |
| } catch (error) { | |
| console.error('Failed to update key count:', error); | |
| } |
Summary by CodeRabbit