proxysql

Commit Graph

Author	SHA1	Message	Date
Rene Cannao	638d33bf96	test: fix two false-positive failures unmasked by recent CI Two distinct test bugs, same kind of mistake: assuming the absence of one specific external signal in a fixed time window. Both are purely test-side -- the proxy code under test is correct. 1. reg_test_oversize_first_pkt-t (legacy-g7, mysql84-g7) ----------------------------------------------------- `peer_closed_within()` polled once, recv'd once, and concluded "peer is still talking" if any bytes came back. That works for PgSQL (server stays silent until the client sends StartupMessage) but fails for MySQL: ProxySQL emits its ~80-byte Initial Handshake Packet immediately on accept, so by the time the helper polls the client socket those bytes are already in the recv buffer. recv() returned them with n>0, the helper returned 0 (not closed), and all four MySQL probes failed with closed=0 -- even though the proxysql.log clearly shows the security-fix bound check ("Oversized first packet... Closing fd=...") firing for every probe and shut_soft() being called. Replace the single-shot poll/recv with a drain-and-re-poll loop bounded by a steady_clock deadline. The helper now returns 1 only when it observes EOF (recv == 0) or a non-EAGAIN error (RST). Bytes that arrive without an accompanying close are drained and we keep polling until either close or the deadline. This keeps the test's contract intact (3-second timeout for the peer to close after we send an oversized header) while correctly handling server-initiated greeting traffic that exists in some protocols but not others. 2. pgsql-test_dns_cache-t (legacy-g4) ---------------------------------- Step 8's assertion `MySQL_Monitor_dns_cache_queried` stays unchanged during a 4-5 s PgSQL-only window was ~40% flaky. Root cause: that counter is bumped by the MySQL Monitor's own DNS resolver loop -- mysql-monitor_local_dns_cache_refresh_interval defaults to 60 s and on each tick the loop iterates every hostname in mysql_servers UNION proxysql_servers. A 4-5 s window sometimes catches one tick (counter goes up by N hostnames) and sometimes catches zero (counter stays flat). The failing run showed 407 -> 411 (+4 = one tick covering four hostnames). The substantive isolation guarantee is "PgSQL activity does not mutate MySQL DNS cache state", which is already covered by the sibling assertion on MySQL_Monitor_dns_cache_record_updated (records are not added or refreshed by the background loop unless hostnames actually change). Drop the noisy queried-counter assertion and adjust the plan from 17 to 16. Alternatives considered: (a) disable mysql-monitor_local_dns_cache for the duration of the test -- doesn't reliably remove in-flight resolver-queue items so the race still exists, (b) widen the tolerance -- still timing-dependent. Removing the assertion is the cleanest, and the long comment in the test explains the why so the next reader does not re-add it. Neither fix touches the security fix (GHSA-58ww-865x-grpr) nor any proxy code; the bound checks in lib/mysql_data_stream.cpp:678 and lib/PgSQL_Data_Stream.cpp:547 fire correctly and identically on both protocols. Verified by reading proxysql.log from a failing run -- all 8 "Oversized first packet... Closing fd=" lines are present.	1 month ago
Rene Cannao	3734690a52	test(pgsql-dns_cache): bail on setup failure; use runtime username 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 month 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.	1 month 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.	1 month ago
Rene Cannao	c10b30523c	Test: cover pgsql DNS cache + fix whitespace handling 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.	1 month ago

5 Commits (fix/pgsql-unix-socket)