Independent DNS cache for PgSQL (fixes #5768)#5806
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds a reusable DNS-cache and async resolver worker, extracts MySQL DNS logic into the shared cache, implements a PgSQL monitor background refresh loop and lookup API, integrates cache use into PgSQL connection setup, exposes per-thread config and counters, adds lifecycle/thread wiring, and includes a comprehensive TAP test. ChangesPostgreSQL DNS Cache System
Sequence Diagram(s)sequenceDiagram
participant PgMonitor as PgSQL_Monitor
participant Config as Runtime Config / Thread-Local
participant DnsCache as DNS_Cache
participant Resolver as DNSResolverWorker
participant libpq as libpq (getaddrinfo)
participant PgConn as PgSQL_Connection
PgMonitor->>Config: read TTL / refresh_interval / queue_size
PgMonitor->>DnsCache: enqueue DNS_Resolve_Data (host, ttl, ai_family)
Resolver->>Resolver: dequeue work item
Resolver->>libpq: getaddrinfo(hostname, ai_family)
libpq-->>Resolver: resolved IP list
Resolver->>DnsCache: add/add_if_not_exist(hostname -> IPs)
Resolver-->>PgMonitor: fulfill promise (success/record)
PgConn->>DnsCache: lookup(hostname)
DnsCache-->>PgConn: selected IP (round-robin)
PgConn->>libpq: PQconnectStart with hostaddr=<ip> + host=<name>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Cppcheck (2.20.0)lib/MySQL_Monitor.cppChecking lib/MySQL_Monitor.cpp ... ... [truncated 5846 characters] ... LIENT_ZSTD_COMPRESSION_ALGORITHM... 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 shared DNS cache infrastructure to be used by both MySQL and PostgreSQL monitors, refactoring existing MySQL code and implementing a new caching mechanism for PostgreSQL. Key changes include the addition of a background resolver loop for PostgreSQL, new configuration variables for cache TTL and refresh intervals, and integration into the connection path to prevent blocking on DNS resolution. Feedback from the review identifies several critical issues, including potential stack overflows due to insufficient thread stack sizes, integer overflows in TTL calculations, and inefficiencies in thread management within the refresh loop. Additionally, improvements were suggested regarding memory management and the use of mutable members to avoid unsafe const_cast operations.
| for (unsigned int i = 0; i < num_dns_resolver_threads; i++) { | ||
| dns_resolver_threads[i] = new DNSResolverWorker(dns_resolver_queue, "pgDnsResolver"); | ||
| dns_resolver_threads[i]->start(2048, false); | ||
| } |
There was a problem hiding this comment.
The current implementation creates and destroys resolver threads and the work queue in every iteration of the refresh loop. This is inefficient and deviates from the persistent thread pool pattern used elsewhere in ProxySQL (e.g., the MySQL monitor). Consider making the resolver threads and queue persistent members of the PgSQL_Monitor class to avoid the overhead of frequent thread lifecycle management.
| std::vector<DNSResolverWorker*> dns_resolver_threads(num_dns_resolver_threads); | ||
| for (unsigned int i = 0; i < num_dns_resolver_threads; i++) { | ||
| dns_resolver_threads[i] = new DNSResolverWorker(dns_resolver_queue, "pgDnsResolver"); | ||
| dns_resolver_threads[i]->start(2048, false); |
There was a problem hiding this comment.
The stack size for the DNSResolverWorker threads is set to 2048 bytes. This is extremely small for a thread that performs DNS resolution via getaddrinfo, which can involve complex library calls and significant stack usage. This is likely to cause a stack overflow and crash the process. It should be increased to a more reasonable value, such as 2048 * 1024 (2MB), consistent with the main DNS cache thread in main.cpp.
dns_resolver_threads[i]->start(2048 * 1024, false);| private: | ||
| struct IP_ADDR { | ||
| std::vector<std::string> ips; | ||
| unsigned long counter = 0; |
There was a problem hiding this comment.
The counter member should be declared as mutable. This allows it to be modified within const methods (like get_next_ip) without resorting to const_cast, which is safer and more idiomatic when the logical state of the object remains constant despite internal bookkeeping changes.
mutable unsigned long counter = 0;| struct sockaddr* addr = (struct sockaddr*)malloc(sizeof(custom_sockaddr)); | ||
| socklen_t addrlen = sizeof(custom_sockaddr); | ||
| memset(addr, 0, sizeof(custom_sockaddr)); | ||
|
|
||
| int rc = getpeername(socket_fd, addr, &addrlen); | ||
|
|
||
| if (rc == 0) { | ||
| if (addr->sa_family == AF_INET) { | ||
| struct sockaddr_in* ipv4 = (struct sockaddr_in*)addr; | ||
| inet_ntop(addr->sa_family, &ipv4->sin_addr, ip_addr, INET_ADDRSTRLEN); | ||
| } | ||
| else if (addr->sa_family == AF_INET6) { | ||
| struct sockaddr_in6* ipv6 = (struct sockaddr_in6*)addr; | ||
| inet_ntop(addr->sa_family, &ipv6->sin6_addr, ip_addr, INET6_ADDRSTRLEN); | ||
| } | ||
|
|
||
| result = ip_addr; | ||
| } | ||
|
|
||
| free(addr); |
There was a problem hiding this comment.
The use of malloc here is unnecessary and adds overhead. Since custom_sockaddr is already allocated on the stack and provides sufficient space for both IPv4 and IPv6 structures, you can pass its address directly to getpeername and avoid the manual memory management.
| struct sockaddr* addr = (struct sockaddr*)malloc(sizeof(custom_sockaddr)); | |
| socklen_t addrlen = sizeof(custom_sockaddr); | |
| memset(addr, 0, sizeof(custom_sockaddr)); | |
| int rc = getpeername(socket_fd, addr, &addrlen); | |
| if (rc == 0) { | |
| if (addr->sa_family == AF_INET) { | |
| struct sockaddr_in* ipv4 = (struct sockaddr_in*)addr; | |
| inet_ntop(addr->sa_family, &ipv4->sin_addr, ip_addr, INET_ADDRSTRLEN); | |
| } | |
| else if (addr->sa_family == AF_INET6) { | |
| struct sockaddr_in6* ipv6 = (struct sockaddr_in6*)addr; | |
| inet_ntop(addr->sa_family, &ipv6->sin6_addr, ip_addr, INET6_ADDRSTRLEN); | |
| } | |
| result = ip_addr; | |
| } | |
| free(addr); | |
| socklen_t addrlen = sizeof(custom_sockaddr); | |
| memset(&custom_sockaddr, 0, sizeof(custom_sockaddr)); | |
| int rc = getpeername(socket_fd, (struct sockaddr*)&custom_sockaddr, &addrlen); | |
| if (rc == 0) { | |
| struct sockaddr* addr = (struct sockaddr*)&custom_sockaddr; | |
| if (addr->sa_family == AF_INET) { | |
| struct sockaddr_in* ipv4 = (struct sockaddr_in*)addr; | |
| inet_ntop(addr->sa_family, &ipv4->sin_addr, ip_addr, INET_ADDRSTRLEN); | |
| } | |
| else if (addr->sa_family == AF_INET6) { | |
| struct sockaddr_in6* ipv6 = (struct sockaddr_in6*)addr; | |
| inet_ntop(addr->sa_family, &ipv6->sin6_addr, ip_addr, INET6_ADDRSTRLEN); | |
| } | |
| result = ip_addr; | |
| } |
| dns_resolve_data->result.set_value(std::make_tuple<>(true, | ||
| DNS_Cache_Record(dns_resolve_data->hostname, | ||
| std::move(dns_resolve_data->cached_ips), | ||
| monotonic_time() + (1000 * cache_ttl)))); |
There was a problem hiding this comment.
The expression 1000 * cache_ttl uses signed 32-bit integer arithmetic. If cache_ttl is large (e.g., several weeks), this multiplication will overflow before being added to the unsigned long long returned by monotonic_time(). Use 1000ULL to ensure the calculation is performed in 64-bit unsigned space.
| monotonic_time() + (1000 * cache_ttl)))); | |
| monotonic_time() + (1000ULL * cache_ttl)))); |
| if (ip_addr.ips.empty()) | ||
| return ""; | ||
|
|
||
| const auto counter_val = __sync_fetch_and_add(const_cast<unsigned long*>(&ip_addr.counter), 1); |
There was a problem hiding this comment.
Using const_cast to modify a member of a const reference is dangerous and can lead to undefined behavior if the underlying object was originally declared const. If the counter member in IP_ADDR is declared mutable (as suggested in DNS_Cache.hpp), this const_cast can be removed.
| const auto counter_val = __sync_fetch_and_add(const_cast<unsigned long*>(&ip_addr.counter), 1); | |
| const auto counter_val = __sync_fetch_and_add(&ip_addr.counter, 1); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/DNS_Cache.cpp`:
- Around line 249-250: The disabled-cache fast path in DNS_Cache::lookup returns
early without initializing the out-parameter ip_count; set *ip_count = 0 before
returning when enabled is false (and ensure ip_count is non-null before writing)
so callers don't see a stale count on a cache miss.
- Around line 217-236: The add_if_not_exist method increments
counter_record_updated_ unconditionally even when no new record is inserted;
modify DNS_Cache::add_if_not_exist so that counter_record_updated_ (and any
related update-side effects) are only updated inside the branch where a new
record is actually created — i.e., after you create/assign records[hostname] and
set ip_addr.ips/counter, increment counter_record_updated_ (and not outside the
if (records.find(hostname) == records.end()) block). Ensure the
pthread_rwlock_wrlock/unlock usage around records remains correct and that
counter_record_updated_ is only __sync_fetch_and_add(...) when an insertion
occurred.
- Around line 34-64: The function get_connected_peer_ip_from_socket currently
returns garbage when sa_family is not AF_INET/AF_INET6 and uses malloc/free; fix
it by replacing the malloc'd sockaddr with a stack sockaddr_storage (e.g.,
declare struct sockaddr_storage custom_addr and use (struct
sockaddr*)&custom_addr), keep socklen_t addrlen = sizeof(custom_addr), call
getpeername with that, and only call inet_ntop and assign result when
addr->sa_family is AF_INET or AF_INET6; do not assign result otherwise (leave it
as the default empty string). Also use the correct buffer sizes when calling
inet_ntop (INET_ADDRSTRLEN for AF_INET, INET6_ADDRSTRLEN for AF_INET6) and
ensure inet_ntop return is checked before setting result.
In `@lib/MySQL_Monitor.cpp`:
- Line 4790: When mysql_thread___resolution_family is changed at runtime (the
places setting dns_resolve_data->ai_family), ensure you invalidate the DNS cache
so old-family entries in dns_cache/dns_records_bookkeeping are not served;
detect when the new family differs from the previous value and clear or flush
the in-memory dns_cache and dns_records_bookkeeping (or call the existing
cache-clear helper) immediately after updating dns_resolve_data->ai_family (same
for the other occurrence around line 4843) so subsequent dns_lookup() calls
honor the new family setting.
In `@lib/PgSQL_Monitor.cpp`:
- Around line 2839-2868: monitor_dns_cache() constructs a PgSQL_Thread but
doesn't refresh its per-thread vars before the loop, so initial
pgsql_thread___monitor_local_dns_* values may be zero; call
pgsql_thr->refresh_variables() immediately after creating pgsql_thr (and set
local_thread_vars_version to GloPTH->get_global_version() so the loop sees the
refreshed state) to ensure DNS cache enable/TTL/refresh-interval are initialized
correctly before the first pass.
In `@src/main.cpp`:
- Around line 1734-1742: Check and handle return values of pthread_attr_init and
pthread_attr_setstacksize before using or destroying attr: call
pthread_attr_init(&attr) and if it returns non-zero log via proxy_error and skip
pthread_create (do not call pthread_attr_setstacksize, pthread_create, or
pthread_attr_destroy); similarly check pthread_attr_setstacksize's return and on
failure log and destroy the attr only if initialized; only call pthread_create
with &attr when both init and setstacksize succeeded and set
pgsql_monitor_dns_cache_thread_started on success otherwise log via proxy_error;
ensure pthread_attr_destroy is called only if pthread_attr_init succeeded.
In `@test/tap/tests/pgsql-test_dns_cache-t.cpp`:
- Around line 329-346: The test inserts an IP literal ('0.0.0.0') which bypasses
DNS so the "cache off" assertions never exercise DNS/cache behavior; update the
INSERT in admin_exec to use a hostname (replace '0.0.0.0' with a DNS name such
as 'pg-dns-test.example' or another resolvable host) so the code path uses DNS
resolution, then run the same read_pg_counters(admin) and hammer_proxy(3) checks
(functions: admin_exec, read_pg_counters, hammer_proxy) to assert
dns_cache_queried and dns_cache_record_updated remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49c45590-46bd-4315-9534-f87fd275dc77
📒 Files selected for processing (14)
include/DNS_Cache.hppinclude/MySQL_Monitor.hppinclude/PgSQL_Connection.hinclude/PgSQL_Monitor.hppinclude/proxysql_structs.hlib/DNS_Cache.cpplib/Makefilelib/MySQL_Monitor.cpplib/PgSQL_Connection.cpplib/PgSQL_Monitor.cpplib/PgSQL_Thread.cppsrc/main.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-test_dns_cache-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables should use snake_case
Constants and macros should use UPPER_SNAKE_CASE
Use C++17 features; conditional compilation should use#ifdefPROXYSQL31,#ifdefPROXYSQL40,#ifdefPROXYSQLFFTO,#ifdefPROXYSQLTSDB,#ifdefPROXYSQLCLICKHOUSE (PROXYSQLGENAI no longer guards core code)
Use RAII for resource management and jemalloc for memory allocation
Use pthread mutexes for synchronization and std::atomic<> for counters
Files:
include/PgSQL_Connection.hinclude/proxysql_structs.hsrc/main.cpplib/PgSQL_Connection.cppinclude/PgSQL_Monitor.hpplib/PgSQL_Monitor.cpplib/MySQL_Monitor.cppinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpplib/PgSQL_Thread.cpplib/DNS_Cache.cpptest/tap/tests/pgsql-test_dns_cache-t.cpp
include/**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards should use
#ifndef_CLASS*_H format
Files:
include/PgSQL_Connection.hinclude/proxysql_structs.hinclude/PgSQL_Monitor.hppinclude/DNS_Cache.hppinclude/MySQL_Monitor.hpp
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern test_*.cpp or *-t.cpp in test/tap/tests/
Files:
test/tap/tests/pgsql-test_dns_cache-t.cpp
🧠 Learnings (1)
📚 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/pgsql-test_dns_cache-t.cpp
🪛 Cppcheck (2.20.0)
lib/PgSQL_Monitor.cpp
[error] 2958-2958: syntax error
(syntaxError)
lib/PgSQL_Thread.cpp
[warning] 4762-4762: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4764-4764: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4768-4768: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4770-4770: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4774-4774: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 4776-4776: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
lib/DNS_Cache.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: 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] 45-45: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (11)
test/tap/groups/groups.json (1)
179-179: LGTM!Also applies to: 398-398
include/PgSQL_Connection.h (1)
256-260: LGTM!lib/PgSQL_Connection.cpp (1)
11-11: LGTM!Also applies to: 377-380, 955-963, 976-985
lib/PgSQL_Thread.cpp (3)
359-361: LGTM!
4059-4061: LGTM!
4761-4778: LGTM!src/main.cpp (1)
104-105: LGTM!Also applies to: 1152-1155
lib/MySQL_Monitor.cpp (1)
1048-1048: LGTM!Also applies to: 4626-4628, 6605-6606
include/PgSQL_Monitor.hpp (1)
51-63: LGTM!Also applies to: 93-115, 127-131
include/proxysql_structs.h (1)
1222-1224: LGTM!Also applies to: 1563-1565
lib/PgSQL_Monitor.cpp (1)
121-122: LGTM!Also applies to: 2740-2825, 3063-3067
| dns_resolve_data->ttl = mysql_thread___monitor_local_dns_cache_ttl; | ||
| dns_resolve_data->refresh_intv = mysql_thread___monitor_local_dns_cache_refresh_interval; | ||
| dns_resolve_data->dns_cache = dns_cache; | ||
| dns_resolve_data->ai_family = mysql_resolution_family_to_ai_family(mysql_thread___resolution_family); |
There was a problem hiding this comment.
Clear cached records when mysql_thread___resolution_family changes.
These assignments only affect newly queued resolutions. If the resolution family is changed at runtime while the cache stays enabled, dns_lookup() can keep serving old-family entries from dns_cache/dns_records_bookkeeping until TTL expiry, so the new setting does not take effect promptly.
💡 Suggested fix
bool dns_cache_enable = true;
+ int last_ai_family = AF_UNSPEC;
...
if (MySQL_Monitor__thread_MySQL_Thread_Variables_version < glover) {
MySQL_Monitor__thread_MySQL_Thread_Variables_version = glover;
mysql_thr->refresh_variables();
next_loop_at = 0;
+
+ const int new_ai_family =
+ mysql_resolution_family_to_ai_family(mysql_thread___resolution_family);
+ if (last_ai_family != AF_UNSPEC && last_ai_family != new_ai_family) {
+ dns_cache->clear();
+ dns_records_bookkeeping.clear();
+ force_dns_cache_update = true;
+ }
+ last_ai_family = new_ai_family;
// dns cache is disabled
if (mysql_thread___monitor_local_dns_cache_ttl == 0 ||
mysql_thread___monitor_local_dns_cache_refresh_interval == 0) {Also applies to: 4843-4843
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/MySQL_Monitor.cpp` at line 4790, When mysql_thread___resolution_family is
changed at runtime (the places setting dns_resolve_data->ai_family), ensure you
invalidate the DNS cache so old-family entries in
dns_cache/dns_records_bookkeeping are not served; detect when the new family
differs from the previous value and clear or flush the in-memory dns_cache and
dns_records_bookkeeping (or call the existing cache-clear helper) immediately
after updating dns_resolve_data->ai_family (same for the other occurrence around
line 4843) so subsequent dns_lookup() calls honor the new family setting.
| pthread_attr_t attr; | ||
| pthread_attr_init(&attr); | ||
| pthread_attr_setstacksize(&attr, 2048 * 1024); | ||
| if (pthread_create(&pgsql_monitor_dns_cache_thread, &attr, &PgSQL_monitor_dns_cache_pthread, NULL) == 0) { | ||
| pgsql_monitor_dns_cache_thread_started = true; | ||
| } else { | ||
| proxy_error("Thread creation: PgSQL DNS cache\n"); | ||
| } | ||
| pthread_attr_destroy(&attr); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant region from src/main.cpp (around the referenced lines)
sed -n '1700,1770p' src/main.cpp | nl -ba | sed -n '1,120p'
# Also locate the exact symbol usage in case the line numbers drift
rg -n "pgsql_monitor_dns_cache_thread|PgSQL_monitor_dns_cache_pthread|pthread_attr_init|pthread_attr_setstacksize|pthread_attr_destroy" src/main.cppRepository: sysown/proxysql
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/main.cpp excerpt 1690-1785 ==="
sed -n '1690,1785p' src/main.cpp | cat -n
echo
echo "=== rg occurrences in src/main.cpp ==="
rg -n "pgsql_monitor_dns_cache_thread|PgSQL_monitor_dns_cache_pthread|pthread_attr_init|pthread_attr_setstacksize|pthread_attr_destroy" src/main.cpp
echo
echo "=== show full block around pgsql_monitor_dns_cache_thread_create ==="
rg -n "pthread_create\\(&pgsql_monitor_dns_cache_thread" -n src/main.cpp -C 3 || trueRepository: sysown/proxysql
Length of output: 4828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/main.cpp excerpt 1690-1785 ==="
sed -n '1690,1785p' src/main.cpp | cat -n
echo
echo "=== rg occurrences in src/main.cpp ==="
rg -n "pgsql_monitor_dns_cache_thread|PgSQL_monitor_dns_cache_pthread|pthread_attr_init|pthread_attr_setstacksize|pthread_attr_destroy" src/main.cpp
echo
echo "=== show block around pthread_create(&pgsql_monitor_dns_cache_thread) ==="
rg -n "pthread_create\\(&pgsql_monitor_dns_cache_thread" src/main.cpp -C 6 || trueRepository: sysown/proxysql
Length of output: 5127
Handle pthread attribute return codes before using/destroying pthread_attr_t for the PgSQL DNS cache thread
pthread_attr_init() and pthread_attr_setstacksize() return values are ignored, but pthread_create() and pthread_attr_destroy() use/destroy attr unconditionally—if pthread_attr_init() fails, subsequent use/destroy is unsafe.
Suggested fix
pthread_attr_t attr;
- pthread_attr_init(&attr);
- pthread_attr_setstacksize(&attr, 2048 * 1024);
- if (pthread_create(&pgsql_monitor_dns_cache_thread, &attr, &PgSQL_monitor_dns_cache_pthread, NULL) == 0) {
+ bool attr_initialized = (pthread_attr_init(&attr) == 0);
+ pthread_attr_t* attr_ptr = nullptr;
+ if (attr_initialized) {
+ if (pthread_attr_setstacksize(&attr, 2048 * 1024) != 0) {
+ proxy_warning("PgSQL DNS cache thread: unable to set custom stack size, using default\n");
+ }
+ attr_ptr = &attr;
+ }
+ if (pthread_create(&pgsql_monitor_dns_cache_thread, attr_ptr, &PgSQL_monitor_dns_cache_pthread, NULL) == 0) {
pgsql_monitor_dns_cache_thread_started = true;
} else {
proxy_error("Thread creation: PgSQL DNS cache\n");
}
- pthread_attr_destroy(&attr);
+ if (attr_initialized) {
+ pthread_attr_destroy(&attr);
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.cpp` around lines 1734 - 1742, Check and handle return values of
pthread_attr_init and pthread_attr_setstacksize before using or destroying attr:
call pthread_attr_init(&attr) and if it returns non-zero log via proxy_error and
skip pthread_create (do not call pthread_attr_setstacksize, pthread_create, or
pthread_attr_destroy); similarly check pthread_attr_setstacksize's return and on
failure log and destroy the attr only if initialized; only call pthread_create
with &attr when both init and setstacksize succeeded and set
pgsql_monitor_dns_cache_thread_started on success otherwise log via proxy_error;
ensure pthread_attr_destroy is called only if pthread_attr_init succeeded.
| // Use an IP literal so any client connect attempt does not invoke | ||
| // DNS code at all — keeps the assertion clean. | ||
| admin_exec(admin, "DELETE FROM pgsql_servers WHERE hostgroup_id=999"); | ||
| admin_exec(admin, | ||
| "INSERT INTO pgsql_servers (hostgroup_id,hostname,port,max_connections,comment) " | ||
| "VALUES (999,'0.0.0.0',7861,10,'pgsql-dns-test cache-off')"); | ||
| admin_exec(admin, "LOAD PGSQL SERVERS TO RUNTIME"); | ||
| sleep_seconds(2); | ||
| before = read_pg_counters(admin); | ||
| hammer_proxy(3); | ||
| sleep_seconds(1); | ||
| after = read_pg_counters(admin); | ||
| ok(after.queried == before.queried, | ||
| "cache off: dns_cache_queried unchanged (%ld -> %ld)", | ||
| before.queried, after.queried); | ||
| ok(after.record_updated == before.record_updated, | ||
| "cache off: dns_cache_record_updated unchanged (%ld -> %ld)", | ||
| before.record_updated, after.record_updated); |
There was a problem hiding this comment.
Step 7 does not actually validate the cache-disabled path.
Line 329 uses an IP literal (0.0.0.0), which bypasses DNS/cache regardless of refresh_interval. So Lines 341-346 can pass even if disabling the cache regresses.
Suggested minimal fix
- // Use an IP literal so any client connect attempt does not invoke
- // DNS code at all — keeps the assertion clean.
+ // Use a resolvable hostname so a regression in cache-disable logic
+ // would move counters and fail this assertion.
admin_exec(admin, "DELETE FROM pgsql_servers WHERE hostgroup_id=999");
admin_exec(admin,
"INSERT INTO pgsql_servers (hostgroup_id,hostname,port,max_connections,comment) "
- "VALUES (999,'0.0.0.0',7861,10,'pgsql-dns-test cache-off')");
+ "VALUES (999,'example.com',7861,10,'pgsql-dns-test cache-off')");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/tap/tests/pgsql-test_dns_cache-t.cpp` around lines 329 - 346, The test
inserts an IP literal ('0.0.0.0') which bypasses DNS so the "cache off"
assertions never exercise DNS/cache behavior; update the INSERT in admin_exec to
use a hostname (replace '0.0.0.0' with a DNS name such as 'pg-dns-test.example'
or another resolvable host) so the code path uses DNS resolution, then run the
same read_pg_counters(admin) and hammer_proxy(3) checks (functions: admin_exec,
read_pg_counters, hammer_proxy) to assert dns_cache_queried and
dns_cache_record_updated remain unchanged.
DNS_Cache, DNS_Cache_Record, DNS_Resolve_Data, the resolver worker, and
the validate_ip / get_connected_peer_ip_from_socket / debug_iplisttostring
helpers used to live inside MySQL_Monitor.{hpp,cpp}. Move them to a new
DNS_Cache.{hpp,cpp} so the same plumbing can back an independent
PgSQL_Monitor cache.
DNS_Cache no longer references GloMyMon directly; counters are wired in
per-instance via set_counters(). monitor_dns_resolver_thread no longer
reads mysql_thread___resolution_family; the desired ai_family is now a
field on DNS_Resolve_Data set by the caller. MySQL_Monitor wires both up
in its constructor and resolver loop respectively; behavior is identical.
No change to the MySQL DNS cache semantics.
Fixes the watchdog asserts under DNS degradation reported in #5768. Symptom: with a PgSQL backend addressed by hostname, an unhealthy resolver made every PQconnectStart() block ~5s on getaddrinfo inside libpq. Under load, all PgSQL_Thread workers spent the bulk of each loop iteration in that synchronous DNS call, atomic_curtime fell behind the watchdog threshold (poll_timeout + 1s), and after restart_on_missing_heartbeats misses the watchdog asserts. MySQL avoids this because mysql-monitor's DNS cache lets MySQL_Connection::connect_start pass an IP directly to libmariadb; PgSQL had no equivalent. This change adds the equivalent for PgSQL, with state fully independent of the MySQL cache: * PgSQL_Monitor gains its own DNS_Cache, force_dns_cache_update flag, and dns_cache_{queried,lookup_success,record_updated} counters. PgSQL_Monitor::dns_lookup / update_dns_cache_from_pgsql_conn / trigger_dns_cache_update mirror the MySQL_Monitor surface. * A new background thread (PgSQL_monitor_dns_cache_pthread) drives PgSQL_Monitor::monitor_dns_cache(), which walks PgHGM->pgsql_servers_to_monitor and feeds the shared DNSResolverWorker pool. Launched alongside PgSQL_monitor_scheduler and lives independently of pgsql-monitor_enabled, matching MySQL. * PgSQL_Connection::connect_start now consults the cache. On a hit it appends `hostaddr=<ip>` to the libpq conninfo while keeping `host=<hostname>`; libpq documents this combo specifically to skip name resolution while preserving TLS hostname verification. On a miss / IP literal / disabled cache we fall back to the previous behavior (libpq does getaddrinfo). * Successful PgSQL connects seed the cache via update_dns_cache_from_pgsql_conn, so the second connect avoids getaddrinfo even before the resolver loop has visited the host. * The pgsql-monitor_local_dns_cache_ttl / _refresh_interval / _resolver_queue_maxsize variables (already accepted by Admin via PgSQL_Thread's variable registry) now actually propagate to pgsql_thread___monitor_local_dns_* globals. The propagation block in PgSQL_Thread::refresh_variables was previously commented out, so the values never reached anywhere. * The runtime stats output (SQL3_GlobalStatus) now exposes the PgSQL_Monitor_dns_cache_* counters alongside the existing PgSQL_Monitor_* counters. DNSResolverWorker is shared infrastructure added to DNS_Cache.hpp/cpp, used here by PgSQL_Monitor. MySQL_Monitor still uses its existing ConsumerThread-based pool; nothing about the MySQL cache changes.
The names were defined in PgSQL_Thread::variables but sat inside a commented-out block in pgsql_thread_variables_names, so Admin's has_variable() returned false and SET pgsql-monitor_local_dns_* was rejected with "Unknown global variable". Move them out of the /* ... */ so the variables actually show up in global_variables and runtime_global_variables and can be tuned at runtime. Verified end-to-end on a smoke run: SET ... LOAD PGSQL VARIABLES TO RUNTIME succeeds, the resolver loop populates the cache, and a client connect through the proxy increments dns_cache_queried / dns_cache_lookup_success (cache hit, libpq skipped getaddrinfo).
Adds pgsql-test_dns_cache-t mirroring the existing MySQL test_dns_cache
test, exercising 16 scenarios:
* pgsql-monitor_local_dns_cache_* variables propagate to runtime
* IP literal does not touch the cache
* Resolvable hostname populates dns_cache_record_updated
* Client connect through the proxy increments dns_cache_queried and
dns_cache_lookup_success on a cache hit
* Hostname with leading/trailing whitespace is trimmed before
resolution
* Unresolvable hostname is queried but produces no record (cache miss
bumps dns_cache_queried only)
* Removing pgsql_servers rows drops the orphaned cache record
* Cache disabled via refresh_interval=0 flatlines all counters
* MySQL DNS cache counters do not move in response to pgsql-side
activity (independence between the two caches)
While writing the test, step 4 (whitespace trim) caught a real bug:
PgSQL_Monitor::monitor_dns_cache was inserting hostnames verbatim from
the SQLite3_result into the resolver queue, so ' example.org ' was
handed to getaddrinfo() unchanged and failed. MySQL_Monitor sidesteps
this by reading via SELECT trim(hostname) FROM monitor_internal.*; the
pgsql variant reads directly from pgsql_servers_to_monitor so it needs
to call trim() in C++. Fix: trim before validate_ip / insert into
the hostnames set. The lookup path already trimmed, so the bookkeeper
key now matches.
Registered the test in groups.json under legacy-g4 and the mysql-*
variant g4 groups, matching how the other pgsql-* tests are grouped.
Fixes for actionable findings on PR #5806: DNS_Cache.cpp / hpp: - lookup(): initialize *ip_count = 0 on the disabled-cache fast path so callers don't observe a stale count from a previous lookup (CodeRabbit). - add_if_not_exist(): only bump counter_record_updated_ when an insert actually happened, and return that signal to callers. Previously a no-op call inflated the cache-update stats (CodeRabbit). - get_connected_peer_ip_from_socket(): drop the malloc, switch to a stack sockaddr_storage, and only assign result on AF_INET / AF_INET6 with a successful inet_ntop. Previously, on an unknown address family, we'd copy the uninitialized ip_addr buffer into result (Gemini + CodeRabbit). - monitor_dns_resolver_thread(): cache_ttl is signed int and at the max configured TTL (7*24*3600*1000 ms) `1000 * cache_ttl` overflows before being added to monotonic_time(). Force the multiply into unsigned long long space with 1000ULL (Gemini). - DNS_Cache::IP_ADDR::counter is now mutable so get_next_ip() (const) can __sync_fetch_and_add it without a const_cast (Gemini). PgSQL_Monitor.cpp: - monitor_dns_cache(): refresh pgsql_thread___monitor_local_dns_* before the loop, not just on the first version-bump. Otherwise the resolver runs its first pass against zero-initialized TTL / refresh interval and starts in the wrong enabled/disabled state until a later config bump (CodeRabbit). test/tap/tests/pgsql-test_dns_cache-t.cpp: - Step 7 ("Cache disabled by refresh_interval=0") was inserting an IP literal (0.0.0.0), which bypasses DNS regardless of the cache state. Switched to a resolvable hostname (example.com) so a regression in the cache-disable path would actually move counters and fail the assertion. Added a third assertion on dns_cache_lookup_success for symmetry. Plan adjusted to 17. Deliberately skipped: - CR comment about mysql_thread___resolution_family change requiring a cache flush: pre-existing MySQL behavior, out of scope for this PR. - CR comment about pthread_attr_init / setstacksize return checks in main.cpp: matches the existing un-checked pattern in MySQL_Monitor::run(). No need to diverge here. - Gemini comment about persistent thread pool vs. per-iteration spawn: MySQL_Monitor::monitor_dns_cache() also recreates the resolver threads each loop iteration; the PgSQL variant follows the same pattern. - Gemini comment about 2048-byte stack size: Thread::start takes ss in KB (lib/thread.cpp:46 `pthread_attr_setstacksize(&attr, ss*1024)`), so 2048 here means 2 MB, matching the MySQL resolver pool.
A subagent code review on top of the CodeRabbit + Gemini bot reviews found five additional issues that the bots missed. All five are fixed here. 1. LOAD PGSQL SERVERS TO RUNTIME was waking MySQL's resolver, not PgSQL's. PgSQL_HostGroups_Manager::commit() inherited a MySQL_Monitor::trigger_dns_cache_update() call from a copy of the MySQL HGM, so after a LOAD pgsql hostnames had to wait for the refresh interval (default 60 s) before becoming resolvable — defeating the warm-cache-before-traffic intent that motivated issue #5768. Route to PgSQL_Monitor::trigger_dns_cache_update(). Verified directly: with refresh_interval=60000 ms, inserting a pgsql_servers row + LOAD now produces record_updated=1 within ~4 s instead of waiting a full minute. 2. DNSResolverWorker could deadlock shutdown. monitor_dns_resolver_thread isn't noexcept (vector growth, set_thread_name, etc. can throw); if it threw, the std::promise on DNS_Resolve_Data was never satisfied, the producer's future::get() blocked forever, the resolver loop never reached the shutdown check, and pthread_join in main hung. Wrap the call in try/catch and force-satisfy the promise with a failure result on any uncaught exit. 3. Test step 3 wasn't actually exercising the connect-path lookup it claimed. hammer_proxy() routes through the proxy front-end, which in standard test infra resolves through the default hostgroup — typically configured with an IP literal (see e.g. test/infra/docker-pgsql16-single/conf/proxysql/config.sql). IP literals bypass dns_lookup via validate_ip(), so the dns_cache_queried bumps observed came from the resolver loop alone, not from the connect path. Step 3 would have passed even if PgSQL_Connection::connect_start_DNS_lookup() was wired up wrong. Install a pgsql_query_rules row from inside the test that routes testuser to hostgroup 999, so the proxy actually calls into the cache on client connects. Clean up the rule in the test exit path. 4. Use-after-move in the _dns_cache_update debug log. Both PgSQL_Monitor::_dns_cache_update and MySQL_Monitor::_dns_cache_update logged debug_iplisttostring(ip_address) after std::move'ing ip_address into add_if_not_exist(). The moved-from vector is in a "valid but unspecified" state — typically empty — so the log printed "IP:[]" instead of the IPs. Capture the string before the move. Same bug also fixed in the bookkeeper expired-record requeue path inside PgSQL_Monitor::monitor_dns_cache. 5. monitor_dns_cache had inconsistent indentation inside the `if (hostnames.empty()) { ... } else { ... }` block introduced when I wrapped the original goto-based control flow. The braces were correct but the body sat at the wrong tab depth, making the scope visually misleading and prone to mis-edits. Re-indented. Findings deliberately skipped from the review (rationale in commit body): - "thread_local shared_ptr<DNS_Cache> keeps stale cache alive across GloPgMon recreate" — pre-existing MySQL pattern; GloPgMon is never destroyed mid-process today. - "PgSQL_monitor_dns_cache_pthread can deref GloPgMon during a tight shutdown" — shutdown ordering in main.cpp sets GloPgMon->shutdown before joining the thread and deletes GloPgMon only afterwards, matching the MySQL pattern. - "cache only seeded on ASYNC_CONNECT_SUCCESSFUL" — matches MySQL; pre-existing scope choice. - "validate_ip rejects bracketed IPv6" — pre-existing in MySQL; no current configuration path produces bracketed IPv6 in pgsql_servers. - "Test relies on outbound DNS for example.com" — matches the upstream MySQL test_dns_cache test which uses google.com / yahoo.com / amazon.com. Established convention. - "ai_family hard-coded to AF_UNSPEC for PgSQL" — already commented in code; follow-up if/when pgsql-resolution_family is added.
5a947c4 to
f2b53ea
Compare
|
Rebased on CodeRabbit + Gemini findings — actioned in
|
| finding | resolution | |
|---|---|---|
| CR | DNS_Cache::lookup() disabled fast-path leaves *ip_count stale |
initialize *ip_count = 0 when enabled == false |
| CR | DNS_Cache::add_if_not_exist() bumps record_updated on no-op |
only bump when an insert actually happens; return false otherwise |
| CR + Gemini | get_connected_peer_ip_from_socket() may copy uninitialized ip_addr on unknown sa_family; uses malloc instead of stack |
drop the malloc, switch to stack sockaddr_storage, assign result only on AF_INET / AF_INET6 with successful inet_ntop |
| CR | PgSQL_Monitor::monitor_dns_cache() doesn't refresh thread vars before the first iteration |
pgsql_thr->refresh_variables() up-front and seed local_thread_vars_version from GloPTH |
| CR | Test step 7 used IP literal so cache-disabled assertion couldn't catch regressions | switch to example.com; added a third assertion on dns_cache_lookup_success |
| Gemini | 1000 * cache_ttl overflows int at max TTL |
force 1000ULL and explicit cast to unsigned long long |
| Gemini | const_cast<unsigned long*>(&counter) to bump round-robin counter |
make IP_ADDR::counter mutable; drop the const_cast |
Deliberately not actioned
- CR — flush cache when
mysql_thread___resolution_familychanges: pre-existing MySQL behavior, out of scope. - CR —
pthread_attr_init/setstacksizereturn-value checks inmain.cpp: matches the un-checked pattern inMySQL_Monitor::run(); no reason to diverge. - Gemini — persistent thread pool:
MySQL_Monitor::monitor_dns_cache()also recreates the resolver threads each loop iteration; the PgSQL variant follows the same pattern. - Gemini — "2048-byte stack size will overflow": this was a misread.
Thread::start()takesssin KB (lib/thread.cpp:46→pthread_attr_setstacksize(&attr, ss*1024)), sostart(2048, ...)gives a 2 MB stack, matching the existing MySQL resolver pool.
Independent subagent review — actioned in f2b53ea6e
After processing the bots' feedback, I had an independent subagent code-review the full diff. It found five issues that the bots missed:
-
LOAD PGSQL SERVERS TO RUNTIMEwas waking the MySQL resolver, not PgSQL's.PgSQL_HostGroups_Manager::commit()inherited aMySQL_Monitor::trigger_dns_cache_update()call from a copy of the MySQL HGM, so freshly added pgsql hostnames had to wait for the nextrefresh_interval(default 60 s) before being cached — defeating the warm-cache-before-traffic intent of ProxySQL crashes after DNS name resolution error - (PostgreSQL) #5768. Verified directly: withrefresh_interval=60000 ms, INSERT + LOAD now producesrecord_updated=1within ~4 s instead of waiting a full minute. -
DNSResolverWorkercould deadlock shutdown ifmonitor_dns_resolver_thread()ever threw (vector growth,set_thread_name, etc.) — thestd::promisewould never be satisfied, the producer'sfuture::get()would block forever, andpthread_joininmainwould hang. Wrap the call intry/catchand force-satisfy the promise with a failure result on any uncaught exit. -
Test step 3 wasn't actually exercising the connect-path lookup it claimed.
hammer_proxy()routes through the proxy front-end, which in standard test infra resolves through the default hostgroup — typically an IP literal in test fixtures likedocker-pgsql16-single/conf/proxysql/config.sql. IP literals bypassdns_lookup()viavalidate_ip(), sodns_cache_queriedbumps came from the resolver loop alone, not from the connect path. Step 3 would have passed even ifPgSQL_Connection::connect_start_DNS_lookup()was wired up wrong. The test now installs its ownpgsql_query_rulesrow that routestestuserto hostgroup 999, and cleans it up on exit. -
Use-after-move in
_dns_cache_updatedebug log. BothPgSQL_Monitor::_dns_cache_updateandMySQL_Monitor::_dns_cache_update(pre-existing) printeddebug_iplisttostring(ip_address)afterstd::move(ip_address). The moved-from vector is in a "valid but unspecified" state — typically empty — so the debug log silently printedIP:[]. Capture the string before the move. Same bug also fixed in the bookkeeper expired-record requeue path. -
Inconsistent indentation in
monitor_dns_cacheinside theif (hostnames.empty()) { ... } else { ... }block I introduced when removing agoto. Braces were correct but the body sat at the wrong tab depth — visually misleading and prone to mis-edits. Re-indented.
Local verification
- Full release build clean.
pgsql-test_dns_cache-t: 17/17 ok in ~1m50s (test now installs its own routing rule so it actually exercises the connect-path lookup it asserts on).LOAD PGSQL SERVERS TO RUNTIMEtriggers immediaterecord_updatedbump (manual check, refresh_interval=60s).
Status
- Rebase clean, force-pushed
f2b53ea6e. - SonarCloud Code Analysis is failing on the previous commit — I'll check whether that's noise from the cppcheck nullPointerOutOfMemory hits CodeRabbit also flagged in the bot's "Additional context" section, but those are spurious on container-managed allocators.
|
validate_ip() treated inet_pton(...) != 0 as success, but inet_pton returns 1 / 0 / -1; a -1 (e.g. unsupported family) would be reported as a valid IP. Compare against == 1 instead. The DNS cache counters (dns_cache_queried / dns_cache_lookup_success / dns_cache_record_updated) were declared as plain unsigned long long on MySQL_Monitor. Resolver workers update them atomically via __sync_fetch_and_add, but p_update_metrics() reads them from a different thread without a barrier, racing the writers. Promote to std::atomic<unsigned long long> and switch DNS_Cache to fetch_add / the atomic pointer type so both ends are consistent. Addresses CodeRabbit review feedback on PR #5806.
GloPgMon->shutdown was a plain bool, written by main.cpp's teardown and read by the monitor / DNS resolver loops on worker threads — a data race that can let the loops spin past shutdown or observe a stale value indefinitely. Promote to std::atomic<bool> and switch the readers/writer to acquire/release ordering. PgSQL_Monitor::dns_lookup() was also returning an empty string when GloPgMon / dns_cache_thread weren't initialized yet, ignoring the return_hostname_if_lookup_fails contract. Move the fallback out of the dns_cache_thread branch so startup / shutdown behave like a normal cache miss for callers that asked for the hostname fallback. Addresses CodeRabbit review feedback on PR #5806.
Two regressions surfaced by CodeRabbit on PR #5806: * admin_exec() return values for the DNS-cache runtime config ('SET pgsql-monitor_local_dns_cache_ttl' / 'LOAD PGSQL VARIABLES TO RUNTIME') were silently dropped, so a failed setup cascaded into misleading downstream assertions. BAIL_OUT immediately. * The pgsql_query_rules INSERT hardcoded username='testuser', but hammer_proxy() connects as cl.pgsql_username. When the infra's test user is anything else the rule never matches, hostgroup 999 is bypassed, and the DNS-cache assertions exercise the wrong path. Build the INSERT with cl.pgsql_username instead.
1 participant
Summary
DNS_Cacheinstance, counters, and resolver loop — admin sets on one side never affect the other.PgSQL_Connection::connect_startnow consults the cache and passeshostaddr=<ip>to libpq on a hit.PQconnectStartno longer blocks ongetaddrinfowhen DNS is degraded, which fixes the watchdog asserts reported in ProxySQL crashes after DNS name resolution error - (PostgreSQL) #5768.DNS_Cache/DNS_Cache_Record/DNS_Resolve_Data/resolver-worker machinery moves into a newDNS_Cache.{hpp,cpp}. MySQL behavior is unchanged (still uses its existingConsumerThread-based pool); only the code is shared.update_dns_cache_from_pgsql_conn. The previously-commented-out propagation ofpgsql-monitor_local_dns_cache_*to globals is uncommented, and the variable names are registered with Admin (they were inside a/* ... */and rejected bySET).stats_pgsql_globalexposes three new counters:PgSQL_Monitor_dns_cache_queried,_lookup_success,_record_updated.See #5768 (comment) for the full analysis: the heartbeat asserts in the original report are a downstream symptom — both protocols already bump
atomic_curtimeafter every connect attempt, but PgSQL had no DNS cache so everyPQconnectStart()blocked synchronously inside libpq on a broken resolver, dominating each worker-loop iteration.Commits
Extract DNS cache into shared DNS_Cache module— refactor only; MySQL behavior unchanged.Add independent DNS cache for PgSQL— the new cache, resolver loop, connection lookup, and admin globals.Register pgsql-monitor_local_dns_* names with Admin— small follow-up; the variable names were commented out inpgsql_thread_variables_namesso admin rejectedSET.Test: cover pgsql DNS cache + fix whitespace handling— newpgsql-test_dns_cache-t.cpp(16 assertions) and a trim() fix the test caught.Test plan
make -jbuilds clean.pgsql-test_dns_cache-tpasses locally (16/16, ~93s):record_updatedqueried+lookup_success(cache hit)queriedbut stays out ofrecord_updatedandlookup_successpgsql_serversrow drops the orphaned cache entryrefresh_interval=0disables the cache and all counters flatlineMySQL_Monitor_dns_cache_*counters (independence)groups.jsonunderlegacy-g4and the fourmysql-*-g4variant groups, matching the other pgsql-* tests. CI picks it up via the%-t: %-t.cpppattern rule.Summary by CodeRabbit
New Features
Metrics
Tests