Query Cache unit tests (Phase 2.3)#5486
Conversation
Comprehensive unit tests for MySQL_Query_Cache and PgSQL_Query_Cache covering 26 test cases across 11 test functions. Runs in <0.01s with no infrastructure dependencies. PgSQL_Query_Cache is used for most tests because its set()/get() API is simpler (no MySQL protocol parsing required). The underlying Query_Cache<T> template logic is identical for both protocols. Test coverage: - Basic set/get cycle with value verification - Cache miss on nonexistent key - User hash isolation (same key, different user_hash = miss) - Cache replacement (same key overwrites previous value) - Hard TTL expiration (expire_ms in the past) - Soft TTL expiration (cache_ttl parameter to get()) - flush() clears all entries and returns correct count - Set/flush cycle (entry retrievable then unretrievable) - Stats counters (SET and GET via SQL3_getStats()) - SQL3_getStats() result format (2 columns, has rows) - MySQL_Query_Cache construction, flush, and SQL3_getStats() - purgeHash() eviction of expired entries (live entries survive) - Bulk insert (100 entries) across 32 internal hash tables Key implementation note: Query_Cache constructor requires a valid Prometheus registry (GloVars.prometheus_registry). The test_init_ query_cache() helper creates one automatically.
- tests/unit/Makefile: Register query_cache_unit-t in the build system - test_init.cpp: Create a real Prometheus registry in test_init_query_ cache() since the Query_Cache constructor registers metrics via GloVars.prometheus_registry (nullptr would crash)
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the unit test coverage for the query cache subsystem, a critical component for performance optimization. By introducing 26 new, isolated test cases for both PostgreSQL and MySQL query caches, it validates core functionalities such as data storage, retrieval, expiration, and statistical tracking. This effort enhances the robustness and reliability of the query cache, ensuring its correct behavior under various conditions and preventing regressions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a new TAP unit test executable for the query cache and ensures test initialization sets Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of unit tests for the MySQL_Query_Cache and PgSQL_Query_Cache components, which is a great step towards improving test coverage. The tests are well-structured and cover a wide range of scenarios, including basic operations, TTL expiration, and cache flushing.
I've made a few suggestions to improve the precision and determinism of some tests. Specifically, I've pointed out a redundant check, a way to make a test more deterministic by clearing state beforehand, and an opportunity to add an assertion to more fully verify the behavior of purgeHash().
| static void test_pgsql_flush() { | ||
| // Add several entries | ||
| uint64_t t = now_ms(); | ||
| for (int i = 0; i < 10; i++) { | ||
| char keybuf[32]; | ||
| snprintf(keybuf, sizeof(keybuf), "SELECT flush_%d", i); | ||
| unsigned char *val = (unsigned char *)malloc(4); | ||
| memcpy(val, "data", 4); | ||
| GloPgQC->set(40000 + i, | ||
| (const unsigned char *)keybuf, strlen(keybuf), | ||
| val, 4, t, t, t + 60000); | ||
| } | ||
|
|
||
| uint64_t flushed = GloPgQC->flush(); | ||
| // flushed should include our entries plus any from prior tests | ||
| ok(flushed >= 10, | ||
| "PgSQL QC: flush() returns count >= 10"); | ||
|
|
||
| // Verify entries are gone | ||
| auto entry = GloPgQC->get(40000, | ||
| (const unsigned char *)"SELECT flush_0", 14, now_ms(), 60000); | ||
| ok(entry == nullptr, | ||
| "PgSQL QC: entries gone after flush()"); | ||
| } |
There was a problem hiding this comment.
This test is not fully deterministic because it depends on the state left by previous tests. The check ok(flushed >= 10, ...) is correct given this, but you could make the test more robust by calling GloPgQC->flush() at the beginning to ensure a clean state. Then you could assert that exactly 10 entries were flushed.
static void test_pgsql_flush() {
GloPgQC->flush(); // Ensure a clean state before the test
// Add several entries
uint64_t t = now_ms();
for (int i = 0; i < 10; i++) {
char keybuf[32];
snprintf(keybuf, sizeof(keybuf), "SELECT flush_%d", i);
unsigned char *val = (unsigned char *)malloc(4);
memcpy(val, "data", 4);
GloPgQC->set(40000 + i,
(const unsigned char *)keybuf, strlen(keybuf),
val, 4, t, t, t + 60000);
}
uint64_t flushed = GloPgQC->flush();
ok(flushed == 10,
"PgSQL QC: flush() returns correct count of 10");
// Verify entries are gone
auto entry = GloPgQC->get(40000,
(const unsigned char *)"SELECT flush_0", 14, now_ms(), 60000);
ok(entry == nullptr,
"PgSQL QC: entries gone after flush()");
}| ok(flushed == 0 || flushed >= 0, | ||
| "MySQL QC: flush() on empty cache succeeds"); |
There was a problem hiding this comment.
The check flushed >= 0 is always true for an unsigned type like uint64_t, making it redundant. Since GloMyQC is newly initialized and should be empty, a more precise check would be to assert that flushed is exactly 0.
| ok(flushed == 0 || flushed >= 0, | |
| "MySQL QC: flush() on empty cache succeeds"); | |
| ok(flushed == 0, | |
| "MySQL QC: flush() on empty cache returns 0"); |
| auto entry = GloPgQC->get(70099, | ||
| (const unsigned char *)"SELECT live", 11, now_ms(), 60000); | ||
| ok(entry != nullptr, | ||
| "PgSQL QC: live entry survives purgeHash()"); |
There was a problem hiding this comment.
This check correctly verifies that the live entry survives. To make the test more robust, you could also verify that the expired entries were removed. For example, by checking that the Query_Cache_Entries stat is 1 after the purge.
// Add this after the existing check
uint64_t entries_after_purge = get_qc_stat(GloPgQC, "Query_Cache_Entries");
ok(entries_after_purge == 1, "PgSQL QC: expired entries are removed by purgeHash()");Remember to update the test plan count in main() if you add this check.
There was a problem hiding this comment.
Pull request overview
Adds a new unit test binary for the Query_Cache<T> subsystem (primarily via PgSQL_Query_Cache, plus minimal MySQL_Query_Cache coverage) and updates the unit-test harness so query cache objects can be constructed safely in isolation.
Changes:
- Introduces
query_cache_unit-t.cppwith 26 TAP assertions covering basic set/get, TTL behavior, flush, stats, purge, and bulk insert sampling. - Extends the unit test
Makefileto build and run the newquery_cache_unit-ttarget. - Updates
test_init_query_cache()to ensureGloVars.prometheus_registryis initialized before constructing query caches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/tap/tests/unit/query_cache_unit-t.cpp | New TAP unit tests for PgSQL/MySQL query cache behaviors and stats. |
| test/tap/tests/unit/Makefile | Adds query_cache_unit-t to unit test build targets. |
| test/tap/test_helpers/test_init.cpp | Initializes a Prometheus registry to prevent query cache constructor crashes in unit tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @brief Test storing and retrieving many entries to exercise | ||
| * distribution across the 32 internal hash tables. | ||
| */ |
There was a problem hiding this comment.
This test (and its docstring) claims it exercises/validates distribution across the 32 internal hash tables, but it only checks that a sample of inserted keys can be retrieved. Either adjust the comment/PR description to match what’s actually verified, or add an explicit check of the bucket mapping (e.g., compute the same SpookyHash/% SHARED_QUERY_CACHE_HASH_TABLES index for the inserted keys and assert that multiple buckets are used).
| uint64_t flushed = GloMyQC->flush(); | ||
| // Flush on empty cache should return 0 | ||
| ok(flushed == 0 || flushed >= 0, | ||
| "MySQL QC: flush() on empty cache succeeds"); |
There was a problem hiding this comment.
ok(flushed == 0 || flushed >= 0, ...) is a tautology because flushed is uint64_t, so this test will always pass even if the cache is unexpectedly non-empty. Make the assertion meaningful (e.g., expect flushed == 0 right after initialization, or explicitly flush() first and then assert the second flush() returns 0).
| uint64_t flushed = GloMyQC->flush(); | |
| // Flush on empty cache should return 0 | |
| ok(flushed == 0 || flushed >= 0, | |
| "MySQL QC: flush() on empty cache succeeds"); | |
| // First flush clears any existing entries (if any). | |
| uint64_t flushed_first = GloMyQC->flush(); | |
| // Second flush should see an empty cache and return 0. | |
| uint64_t flushed_second = GloMyQC->flush(); | |
| ok(flushed_second == 0, | |
| "MySQL QC: flush() on empty cache returns 0"); |
| // purgeHash with large max_memory so it only evicts by TTL | ||
| GloPgQC->purgeHash(1024ULL * 1024 * 1024); | ||
|
|
||
| // Live entry should still be accessible | ||
| auto entry = GloPgQC->get(70099, | ||
| (const unsigned char *)"SELECT live", 11, now_ms(), 60000); | ||
| ok(entry != nullptr, | ||
| "PgSQL QC: live entry survives purgeHash()"); |
There was a problem hiding this comment.
purgeHash(1024ULL * 1024 * 1024) will likely be a no-op in this test because Query_Cache::purgeHash(max_memory_size) returns early when current_used_memory_pct(max_memory_size) < DEFAULT_purge_threshold_pct_min (3%). With a 1GB limit and only a few small entries, the threshold isn't reached so expired entries won't be purged. Use a much smaller max_memory_size to force the purge path to run, and add an assertion that expired entries were actually evicted (e.g., flush() count drops to 1, or Query_Cache_Entries / memory-bytes stats decrease while the live entry remains accessible).
![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
🧹 Nitpick comments (1)
test/tap/tests/unit/query_cache_unit-t.cpp (1)
186-209: Test name may be misleading — testscache_ttlcutoff, notsoft_ttl_pct.This test validates that
get()returnsnullptrwhencreate_ms + cache_ttl < curtime_ms. However, the "soft TTL" feature in ProxySQL refers to thequery_cache_soft_ttl_pctthread variable which triggers background refresh while still returning cached data.The current test correctly exercises the
cache_ttlparameter behavior, but the name "soft TTL" may cause confusion with the percentage-based soft TTL refresh mechanism.Consider renaming to clarify:
-static void test_pgsql_soft_ttl() { +static void test_pgsql_cache_ttl_cutoff() {And update the comment to distinguish from
query_cache_soft_ttl_pct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/query_cache_unit-t.cpp` around lines 186 - 209, Rename the test function test_pgsql_soft_ttl to a name that reflects it verifies the cache_ttl cutoff (e.g., test_pgsql_cache_ttl_cutoff) and update the block comment above it to state explicitly that the test asserts get(user_hash, key, kl, now_ms(), cache_ttl) returns nullptr when create_ms + cache_ttl < curtime_ms; also add a note distinguishing this behavior from the query_cache_soft_ttl_pct background-refresh feature so readers won't confuse the cache_ttl cutoff with percentage-based soft TTL refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/unit/query_cache_unit-t.cpp`:
- Around line 362-366: The test's assertion is a tautology because it checks
"flushed == 0 || flushed >= 0" for an unsigned uint64_t; change the assertion to
validate the intended behavior by asserting the exact expected value from
GloMyQC->flush(), e.g., replace the condition with "flushed == 0" (and update
the test message to something like "MySQL QC: flush() on empty cache returns 0")
so the test actually verifies that GloMyQC->flush() returns zero for an empty
cache.
---
Nitpick comments:
In `@test/tap/tests/unit/query_cache_unit-t.cpp`:
- Around line 186-209: Rename the test function test_pgsql_soft_ttl to a name
that reflects it verifies the cache_ttl cutoff (e.g.,
test_pgsql_cache_ttl_cutoff) and update the block comment above it to state
explicitly that the test asserts get(user_hash, key, kl, now_ms(), cache_ttl)
returns nullptr when create_ms + cache_ttl < curtime_ms; also add a note
distinguishing this behavior from the query_cache_soft_ttl_pct
background-refresh feature so readers won't confuse the cache_ttl cutoff with
percentage-based soft TTL refresh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a36c2677-0559-434a-b76f-b402a20fddb9
📒 Files selected for processing (3)
test/tap/test_helpers/test_init.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/query_cache_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: Agent
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/test_helpers/test_init.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/query_cache_unit-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/query_cache_unit-t.cpp
[error] 28-28: 'tap.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/test_helpers/test_init.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 84-84: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 85-85: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/query_cache_unit-t.cpp
[error] 149-149: Common realloc mistake
(memleakOnRealloc)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 66-66: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 112-112: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 140-140: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 144-144: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 175-175: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 197-197: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 225-225: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 274-274: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 307-307: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 394-394: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 405-405: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (3)
test/tap/test_helpers/test_init.cpp (1)
84-89: LGTM! Necessary null-guard for Query_Cache construction.This correctly initializes
GloVars.prometheus_registrybefore constructingMySQL_Query_CacheandPgSQL_Query_Cache. The constructor callsinit_prometheus_counter_array()which dereferences the registry, so the guard prevents crashes in unit tests that bypass the full daemon initialization.Note: The static analysis warnings about null pointer dereference on memory allocation failure (lines 84-86) are false positives—those lines are comments. On line 88,
std::make_sharedthrowsstd::bad_allocon failure rather than returning nullptr, so no additional check is needed.test/tap/tests/unit/query_cache_unit-t.cpp (1)
468-495: Well-structured test suite with comprehensive coverage.The test organization is clean with 26 TAP assertions covering core query cache behaviors. The test count in
plan(26)matches the actual assertions (4+1+2+2+1+1+2+2+2+3+3+1+2 = 26). Good use of helper functions and consistent null-checking patterns.test/tap/tests/unit/Makefile (1)
234-256: LGTM! Build target follows established pattern.The new
query_cache_unit-ttarget correctly mirrors thesmoke_test-tbuild rule with the same dependencies, include paths, library links, and platform-specific flags.
- test_pgsql_flush: flush() at start for clean state, assert == 10 - test_mysql_construction_and_flush: fix tautology (flushed == 0 || flushed >= 0 is always true for uint64_t), double-flush to ensure empty cache, then assert == 0 - test_pgsql_purge_expired: use max_memory_size=1 instead of 1GB to force the purge path to actually run (3% threshold wasn't reached) - test_pgsql_many_entries: fix misleading docstring claiming hash table distribution verification
|
Summary
Implements Phase 2.3 (#5475) of the Unit Testing Framework: unit tests for
MySQL_Query_CacheandPgSQL_Query_Cache.PgSQL_Query_Cachefor most tests (simpler API, sameQuery_Cache<T>template logic)Test Coverage
Implementation Notes
Query_Cacheconstructor registers metrics viaGloVars.prometheus_registry. Thetest_init_query_cache()helper now creates a realprometheus::Registryto prevent nullptr crashes.MySQL_Query_Cache::set()parses MySQL protocol result sets (packet boundaries, EOF/OK offsets). Testing this requires crafting valid MySQL protocol data, which is deferred to Phase 2.5 (Protocol tests).Glo_cntSet,Glo_cntGetetc. are shared across allQuery_Cache<T>instantiations. Tests verify relative changes, not absolute values.Depends On
Test plan
Summary by CodeRabbit