proxysql

Commit Graph

Author	SHA1	Message	Date
Rene Cannao	de37c64f5e	chore(sonar): tighten NOSONAR placement for remaining hotspots Two SonarCloud hotspots from the previous round didn't actually pick up the inline annotations: - lib/DNS_Cache.cpp: the NOSONAR comment was on the line above the `std::mt19937 gen(...)` construction. Sonar attributes the hotspot to the construction line itself, so the comment needs to be inline on that line. Moved it. Kept the explanatory comment block above. - test/tap/tests/setparser_parsersql_test.cpp: new hotspot on the strlen() call in the inline_validate_search_path helper added in commit `950f6415b`. Test code, caller-supplied null-terminated C string, and the SIZE_MAX guard on the very next line bounds the result. Annotated inline. No behaviour change; only Sonar annotation cleanups. Should clear the last two TO_REVIEW hotspots on PR #5809.	3 weeks ago
Rene Cannao	6f63499731	chore(sonar): silence cpp:S2245 PRNG false-positives in DNS jitter sites Two SonarCloud "weak-cryptography" hotspots flagged uses of mt19937 (DNS_Cache.cpp:117) and rand() (PgSQL_Monitor.cpp:2956). Both are non-cryptographic jitter sources -- one to spread the DNS cache TTL refresh, the other to spread the DNS bookkeeping refresh interval -- with no security boundary touching them. Adding inline NOSONAR annotations with the reasoning so future reviewers (human or bot) don't have to re-derive it.	3 weeks ago
Rene Cannao	039d538c2a	fix(dns): correct inet_pton check; make DNS cache counters atomic 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.	3 weeks ago
Rene Cannao	f2b53ea6e3	Address findings from independent diff review 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.	3 weeks ago
Rene Cannao	a62d1b4e99	Address CodeRabbit + Gemini review feedback 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 (72436001000 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.	3 weeks ago
Rene Cannao	a19894427e	Add independent DNS cache for PgSQL 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.	3 weeks ago
Rene Cannao	a2fff3725f	Extract DNS cache into shared DNS_Cache module 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.	3 weeks ago

7 Commits (97eab1bbf0ce3c7c453e8a8bb96b2bd564e4758f)