feat(pgsql): advanced query logging parity with mysql eventslog#5391
Merged
renecannao merged 15 commits intov3.0from Feb 18, 2026
Merged
feat(pgsql): advanced query logging parity with mysql eventslog#5391renecannao merged 15 commits intov3.0from
renecannao merged 15 commits intov3.0from
Conversation
Context: - Bison/Flex-generated parser files in `lib/` can be created locally during parser work. - These files are generated artifacts and should not be tracked. Changes: - Add generated parser artifacts to `.gitignore`: - `lib/MySQL_Lexer.yy.c` - `lib/MySQL_Parser.output` - `lib/MySQL_Parser.tab.c` - `lib/MySQL_Parser.tab.h` - Extend `lib/Makefile` `clean` target to remove the same generated files. Why: - Prevent accidental inclusion of regenerated parser outputs in feature commits. - Keep worktrees clean and make `make clean` deterministic for parser-related changes.
…k sync
Context:
- PostgreSQL advanced query logging already had end-to-end coverage for manual dump flows.
- Scheduler-driven automatic dump path also needs explicit TAP coverage.
Test added:
- `test/tap/tests/pgsql_query_logging_autodump-t.cpp`
- Connects to PGSQL admin and frontend interfaces.
- Enables query logging and admin scheduler variable:
`admin-stats_pgsql_eventslog_sync_buffer_to_disk=1`
- Generates frontend workload (`SELECT 1` loop).
- Polls `history_pgsql_query_events` to assert rows are flushed to disk by scheduler.
- Validates successful rows are present (`sqlstate IS NULL`).
- Restores scheduler variable to `0` during cleanup.
Harness wiring:
- Register `pgsql_query_logging_autodump-t` in `test/tap/groups/groups.json` so it runs with default PGSQL test group settings.
Why:
- Close e2e coverage gap between manual dump and auto-dump behavior for PGSQL events logging.
- Catch regressions in scheduler-based buffer synchronization path.
…anced logging components Context: - The PostgreSQL advanced query logging implementation added new event, buffer, and metrics APIs. - Several newly introduced functions lacked Doxygen coverage. Changes: - Add Doxygen documentation blocks for newly introduced logging types and APIs in `include/PgSQL_Logger.hpp`: - metric enums and metric-map indexes - `PGSQL_LOG_EVENT_TYPE` - `PgSQL_Event` class and its new/updated lifecycle + serialization methods - `PgSQL_Logger_CircularBuffer` constructor/destructor and buffer operations - `PgSQL_Logger` constructor/destructor Why: - Improve maintainability and code navigation for the new PostgreSQL advanced logging feature set. - Align implementation with project expectation that introduced features include inline Doxygen documentation.
… section with auto-dump E2E coverage Context: - The architecture document was rewritten as an as-built reference, but latest test coverage additions were not fully reflected. Changes: - Update validation mapping in `doc/internal/pgsql_advanced_query_logging_architecture.md` to include: - `pgsql_query_logging_autodump-t` TAP test - scheduler-driven auto-dump acceptance coverage - both relevant TAP group registrations Why: - Keep internal architecture/validation documentation aligned with implemented and wired test coverage. - Avoid stale validation references when reviewing branch readiness.
…ross admin ports Context: - Admin commands are exposed by one shared Admin module over two protocols: - MySQL admin endpoint (`6032`) - PostgreSQL admin endpoint (`6132`) - `DUMP PGSQL EVENTSLOG ...` must behave identically regardless of which admin protocol executes the command. Issue fixed: - PGSQL events dump path used thread-local PGSQL sizing variables when draining to memory (`stats_pgsql_query_events`). - When command execution happened from a non-PGSQL protocol admin worker, TLS values could be out of sync for PGSQL sizing decisions. Changes: - In `lib/PgSQL_Logger.cpp`, add runtime-size helper functions that read canonical values from `GloPTH->variables` with TLS fallback: - `eventslog_buffer_max_query_length` - `eventslog_table_memory_size` - Use these helpers in: - `PgSQL_Event` copy constructor (buffered query truncation) - `PgSQL_Logger::processEvents()` (in-memory retention for `stats_pgsql_query_events`) - Update internal architecture doc to explicitly state command availability from both admin protocol endpoints. Why: - Ensure `DUMP PGSQL EVENTSLOG ...` semantics remain consistent across `6032` and `6132`, matching the shared-admin-module design.
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughImplements PostgreSQL advanced query logging: in-memory circular buffer, batched persistence to memory/disk SQLite tables, Prometheus-style metrics, scheduler-driven auto-dumps, admin command DUMP PGSQL EVENTSLOG, runtime/thread variables, TAP tests, docs, and build/ignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Connection
participant PgLogger as PgSQL_Logger
participant CircBuf as PgSQL_Logger_CircularBuffer
participant MemDB as Stats Memory DB
participant DiskDB as Stats Disk DB
participant Scheduler as Scheduler/Timer
Client->>PgLogger: log_request(event)
activate PgLogger
PgLogger->>CircBuf: insert(PgSQL_Event)
activate CircBuf
CircBuf-->>CircBuf: bounded enqueue / drop oldest
CircBuf-->>PgLogger: buffered
deactivate CircBuf
PgLogger-->>Client: return
Scheduler->>PgLogger: processEvents(statsdb, statsdb_disk)
activate PgLogger
PgLogger->>CircBuf: get_all_events()
activate CircBuf
CircBuf-->>PgLogger: vector<PgSQL_Event*>
deactivate CircBuf
PgLogger->>MemDB: insertPgSQLEventsIntoDb(stats_pgsql_query_events)
PgLogger->>DiskDB: insertPgSQLEventsIntoDb(history_pgsql_query_events)
PgLogger->>PgLogger: p_update_metrics()
PgLogger-->>Scheduler: done
deactivate PgLogger
sequenceDiagram
participant Admin as Admin Client
participant Handler as Admin_Handler
participant PgLogger as PgSQL_Logger
participant MemDB as Stats Memory DB
participant DiskDB as Stats Disk DB
Admin->>Handler: DUMP PGSQL EVENTSLOG FROM BUFFER TO BOTH
activate Handler
Handler->>PgLogger: processEvents(statsdb, statsdb_disk)
activate PgLogger
PgLogger->>PgLogger: get_all_events()
PgLogger->>MemDB: INSERT stats_pgsql_query_events
PgLogger->>DiskDB: INSERT history_pgsql_query_events
PgLogger->>PgLogger: update metrics
PgLogger-->>Handler: row_count
Handler-->>Admin: OK row_count
deactivate PgLogger
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 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 |
Summary of ChangesHello @renecannao, 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 enhances PostgreSQL query observability by introducing an advanced logging system that achieves feature parity with the existing MySQL eventslog. It provides mechanisms for capturing, buffering, and persisting PostgreSQL query events, including detailed error information, to both in-memory and on-disk SQLite tables. The changes enable both manual and scheduled automatic dumping of these events, along with new configuration options and Prometheus metrics for monitoring. This upgrade allows users to gain deeper insights into PostgreSQL query performance and failures within the ProxySQL environment. 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. Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 advanced query logging for PostgreSQL, bringing it to parity with the existing MySQL eventslog functionality. Key changes include capturing query events, in-memory circular buffering, manual and scheduled auto-dumping to SQLite tables, and Prometheus metrics exposure. The changes also include necessary updates to the .gitignore file, internal documentation, and TAP tests to validate the new features. The implementation appears robust and well-documented, aligning PostgreSQL logging capabilities with those already present for MySQL.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
lib/ProxySQL_Admin_Stats.cpp (1)
787-798: Prefer prepared statement for PgSQL logger metrics inserts.Avoid per-metric
malloc/snprintfand keep the stats insert path consistent with the prepared-statement helpers used elsewhere.♻️ Suggested refactor
if (GloPgSQL_Logger != nullptr) { const string prefix = "PgSQL_Logger_"; - std::unordered_map<std::string, unsigned long long> metrics = GloPgSQL_Logger->getAllMetrics(); - for (std::unordered_map<std::string, unsigned long long>::iterator it = metrics.begin(); it != metrics.end(); it++) { - string var_name = prefix + it->first; - query = (char*)malloc(strlen(a) + var_name.length() + 32 + 16); - snprintf(query, strlen(a) + var_name.length() + 32 + 16, a, var_name.c_str(), std::to_string(it->second).c_str()); - statsdb->execute(query); - free(query); - } + int rc = 0; + stmt_unique_ptr u_stmt { nullptr }; + std::tie(rc, u_stmt) = statsdb->prepare_v2("INSERT INTO stats_pgsql_global VALUES (?1, ?2)"); + ASSERT_SQLITE_OK(rc, statsdb); + sqlite3_stmt* const stmt { u_stmt.get() }; + + std::unordered_map<std::string, unsigned long long> metrics = GloPgSQL_Logger->getAllMetrics(); + for (std::unordered_map<std::string, unsigned long long>::iterator it = metrics.begin(); it != metrics.end(); it++) { + const string var_name = prefix + it->first; + const string val = std::to_string(it->second); + rc = (*proxy_sqlite3_bind_text)(stmt, 1, var_name.c_str(), -1, SQLITE_TRANSIENT); ASSERT_SQLITE_OK(rc, statsdb); + rc = (*proxy_sqlite3_bind_text)(stmt, 2, val.c_str(), -1, SQLITE_TRANSIENT); ASSERT_SQLITE_OK(rc, statsdb); + SAFE_SQLITE3_STEP2(stmt); + rc = (*proxy_sqlite3_clear_bindings)(stmt); ASSERT_SQLITE_OK(rc, statsdb); + rc = (*proxy_sqlite3_reset)(stmt); ASSERT_SQLITE_OK(rc, statsdb); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ProxySQL_Admin_Stats.cpp` around lines 787 - 798, The loop that iterates GloPgSQL_Logger->getAllMetrics() currently builds SQL per-metric using malloc/snprintf and calls statsdb->execute(query); replace this with a single prepared-statement path: prepare the INSERT/UPDATE statement once (reuse the same parameterized statement used by other metrics helpers), then for each metric call the prepared-statement execution API with bound parameters (use the metric name = prefix + it->first and metric value = it->second converted to string/number) instead of allocating and formatting strings; update code around GloPgSQL_Logger, getAllMetrics, and statsdb->execute usage to use the prepared statement helpers and release any resources accordingly.test/tap/tests/pgsql_query_logging_autodump-t.cpp (1)
169-175: Consider adding a diagnostic for the success-row count assertion.If
success_rows < num_queries, theok()at line 175 fails with no context about the actual row count, which can make CI failures harder to triage — especially if the auto-dump fired but logged fewer rows than expected.💡 Suggested improvement
ok(success_rows_ok && success_rows >= num_queries, "History table includes expected successful rows"); + if (success_rows_ok && success_rows < num_queries) { + diag("Expected >= %d successful rows, got %lld", num_queries, success_rows); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql_query_logging_autodump-t.cpp` around lines 169 - 175, The test currently calls ok(success_rows_ok && success_rows >= num_queries, ...) without emitting the actual values on failure; update the assertion to emit a diagnostic when the count check fails by checking success_rows_ok and comparing success_rows to num_queries and calling the test framework's diagnostic (e.g., diag or a message) with success_rows, num_queries and success_rows_ok before/when calling ok; locate the variables success_rows, success_rows_ok and the call to query_one_int and modify the logic so a clear diagnostic containing success_rows and num_queries is printed when the condition is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/Admin_Handler.cpp`:
- Around line 3193-3218: The current branch handling "DUMP PGSQL EVENTSLOG"
treats a null GloPgSQL_Logger as success returning 0 rows; change that to report
an explicit error: in the block where you check if (it != commandMap.end()) add
a guard for GloPgSQL_Logger == nullptr and call
SPA->send_error_msg_to_client(sess, ...) (with a clear message like "PgSQL
logger not initialized") and avoid sending SPA->send_ok_msg_to_client;
otherwise, when GloPgSQL_Logger is non-null call
GloPgSQL_Logger->processEvents(it->second.first, it->second.second) and then
send_ok_msg_to_client as before so operators get a real error when the logger is
unavailable (refer to GloPgSQL_Logger, processEvents,
SPA->send_error_msg_to_client, SPA->send_ok_msg_to_client, commandMap,
query_no_space).
In `@lib/PgSQL_Logger.cpp`:
- Around line 1455-1472: The current logic deletes the table when events.size()
>= maxInMemorySize but then calls insertPgSQLEventsIntoDb(statsdb,
"stats_pgsql_query_events", numEventsToInsert, events.begin()), which reinserts
the oldest events from the batch; change the insert to use the tail of the
vector so the newest numEventsToInsert events are inserted (use events.end() -
numEventsToInsert as the iterator start) while keeping the same deletion logic
and using the existing helpers get_pgsql_eventslog_table_memory_size_runtime(),
statsdb, insertPgSQLEventsIntoDb, numEventsToInsert and events.
- Around line 319-322: PgSQL_Event::set_error currently stores raw pointers
(sqlstate, errmsg) to c_str() data which can dangle; instead copy the strings
into owned storage: modify set_error to take or construct std::string copies
(e.g., assign to member std::string fields or allocate std::string members)
rather than const char*; update PgSQL_Event to have std::string members (or
ensure assignments like sqlstate_str = _sqlstate; errmsg_str = _errmsg and set
sqlstate = sqlstate_str.c_str(); errmsg = errmsg_str.c_str()) so the error text
lifetime is owned by the object.
In `@test/tap/tests/pgsql_query_logging_autodump-t.cpp`:
- Around line 177-180: The cleanup currently only disables auto-dump scheduler
but leaves pgsql-eventslog_default_log and pgsql-eventslog_buffer_history_size
changed; update the cleanup sequence in the test (around the cleanup_ok /
exec_ok calls using admin_conn and exec_ok) to restore both settings (set
pgsql-eventslog_default_log back to 0 and pgsql-eventslog_buffer_history_size
back to its original value) before calling "LOAD ADMIN VARIABLES TO RUNTIME",
and include those exec_ok results in the cleanup_ok boolean so failures are
detected.
---
Nitpick comments:
In `@lib/ProxySQL_Admin_Stats.cpp`:
- Around line 787-798: The loop that iterates GloPgSQL_Logger->getAllMetrics()
currently builds SQL per-metric using malloc/snprintf and calls
statsdb->execute(query); replace this with a single prepared-statement path:
prepare the INSERT/UPDATE statement once (reuse the same parameterized statement
used by other metrics helpers), then for each metric call the prepared-statement
execution API with bound parameters (use the metric name = prefix + it->first
and metric value = it->second converted to string/number) instead of allocating
and formatting strings; update code around GloPgSQL_Logger, getAllMetrics, and
statsdb->execute usage to use the prepared statement helpers and release any
resources accordingly.
In `@test/tap/tests/pgsql_query_logging_autodump-t.cpp`:
- Around line 169-175: The test currently calls ok(success_rows_ok &&
success_rows >= num_queries, ...) without emitting the actual values on failure;
update the assertion to emit a diagnostic when the count check fails by checking
success_rows_ok and comparing success_rows to num_queries and calling the test
framework's diagnostic (e.g., diag or a message) with success_rows, num_queries
and success_rows_ok before/when calling ok; locate the variables success_rows,
success_rows_ok and the call to query_one_int and modify the logic so a clear
diagnostic containing success_rows and num_queries is printed when the condition
is false.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements advanced query logging for PostgreSQL with operational parity to the existing MySQL eventslog infrastructure. It introduces a complete event capture, buffering, persistence, and management pipeline for PostgreSQL queries, including in-memory circular buffering, manual dump commands, scheduler-driven auto-dump, comprehensive metrics, and end-to-end test coverage.
Changes:
- Adds PostgreSQL query event logging pipeline with
PgSQL_Eventdeep-copy semantics,PgSQL_Logger_CircularBufferfor thread-safe event buffering, and dual-sink persistence to in-memory (stats_pgsql_query_events) and on-disk (history_pgsql_query_events) SQLite tables - Introduces
DUMP PGSQL EVENTSLOG FROM BUFFER TO {MEMORY|DISK|BOTH}admin commands accessible from both MySQL (port 6032) and PostgreSQL (port 6132) admin interfaces, with symmetric scheduler auto-dump controlled byadmin-stats_pgsql_eventslog_sync_buffer_to_diskvariable - Adds TAP test coverage for manual dump flow and scheduler-driven auto-dump with validation of table schemas, SQLSTATE capture, error message persistence, and row accounting
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
lib/PgSQL_Logger.cpp |
Core implementation of event capture, circular buffer, copy constructor, persistence to SQLite, and Prometheus metrics |
include/PgSQL_Logger.hpp |
Class definitions for PgSQL_Event, PgSQL_Logger_CircularBuffer, metric enums, and API documentation |
lib/PgSQL_Thread.cpp |
Integration of eventslog variables and runtime buffer size updates |
lib/ProxySQL_Admin.cpp |
Admin variable registration, scheduler loop integration for auto-dump, and metrics update hooks |
lib/Admin_Handler.cpp |
Command parser and dispatcher for DUMP PGSQL EVENTSLOG commands |
lib/ProxySQL_Admin_Stats.cpp |
Stats table population with PgSQL_Logger metrics |
lib/ProxySQL_Statistics.cpp |
Timer logic for periodic PgSQL eventslog dumps and table/index definitions |
lib/Admin_Bootstrap.cpp |
In-memory stats table registration for stats_pgsql_query_events |
include/ProxySQL_Admin_Tables_Definitions.h |
Schema definition for in-memory PostgreSQL query events table |
include/ProxySQL_Statistics.hpp |
Schema definition for on-disk history table and timer state |
include/proxysql_admin.h |
Admin variable declaration for stats_pgsql_eventslog_sync_buffer_to_disk |
include/PgSQL_Thread.h |
Thread variable declarations for buffer configuration |
include/proxysql_structs.h |
Thread-local variable extern declarations for eventslog config |
test/tap/tests/pgsql_query_logging_memory-t.cpp |
TAP test validating manual dump, column schema, SQLSTATE capture, and error message persistence |
test/tap/tests/pgsql_query_logging_autodump-t.cpp |
TAP test validating scheduler-driven auto-dump behavior |
test/tap/groups/groups.json |
Test group registration for new PostgreSQL logging tests |
doc/internal/pgsql_advanced_query_logging_architecture.md |
Comprehensive as-built architecture documentation |
lib/Makefile |
Enhanced clean target to remove generated parser artifacts |
.gitignore |
Added MySQL parser artifacts to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/PgSQL_Logger.cpp
Outdated
Comment on lines
+320
to
+321
| sqlstate = const_cast<char*>(_sqlstate); | ||
| errmsg = const_cast<char*>(_errmsg); |
There was a problem hiding this comment.
The set_error method uses const_cast to store pointers without copying the data. This creates a lifetime/ownership bug. The stored pointers (sqlstate and errmsg) point to memory owned by the caller (myds->myconn), but when the event is deep-copied via the copy constructor (lines 242-247), strdup is used to duplicate these pointers, setting free_on_delete=true. This means:
- The original event holds borrowed pointers (not owned)
- The copied event holds owned pointers (duplicated via strdup)
- When both are destroyed, the copy correctly frees its memory but the original event has free_on_delete=false so it won't free
However, if the original connection object is destroyed before the event copy, the original event's pointers become dangling.
MySQL's equivalent method set_errmsg (lib/MySQL_Logger.cpp:485-489) correctly uses strdup immediately to take ownership. This PgSQL implementation should match that pattern and strdup the strings in set_error rather than storing borrowed pointers with const_cast.
Suggested change
| sqlstate = const_cast<char*>(_sqlstate); | |
| errmsg = const_cast<char*>(_errmsg); | |
| // If we previously owned these strings, free them before overwriting. | |
| if (free_on_delete) { | |
| if (sqlstate != nullptr) { | |
| free(sqlstate); | |
| } | |
| if (errmsg != nullptr) { | |
| free(errmsg); | |
| } | |
| } | |
| // Take ownership of new copies of the provided strings. | |
| sqlstate = (_sqlstate != nullptr) ? strdup(_sqlstate) : nullptr; | |
| errmsg = (_errmsg != nullptr) ? strdup(_errmsg) : nullptr; | |
| free_on_delete = true; |
| } | ||
|
|
||
| PgSQL_Event::PgSQL_Event(const PgSQL_Event& other) { | ||
| memcpy(this, &other, sizeof(PgSQL_Event)); |
There was a problem hiding this comment.
The copy constructor uses memcpy(this, &other, sizeof(PgSQL_Event)) before selectively deep-copying pointer fields. This shallow copy includes copying all pointer values, which are then overwritten. While this works, it's fragile and error-prone:
- The
bufarray is copied but immediately zeroed in the original constructor (line 206), creating inconsistency - If new pointer fields are added to PgSQL_Event but not handled in the copy constructor, they'll be shallow-copied leading to double-free bugs
- The pattern mixes shallow and deep copying in a non-obvious way
A safer approach would be to explicitly copy each field, or use a member-wise copy followed by selective deep-copies with clear documentation that all pointer fields must be handled explicitly.
lib/PgSQL_Logger.cpp
Outdated
| } | ||
| wrunlock(); | ||
| } | ||
| if (PgLogCB->buffer_size != 0) { |
There was a problem hiding this comment.
The buffer_size field is declared as std::atomic<size_t> (line 187 in include/PgSQL_Logger.hpp) but is accessed and modified while holding the mutex lock in setBufferSize. This creates a race condition:
- Line 1039 reads
PgLogCB->buffer_size != 0without holding the mutex - Line 1353 writes
buffer_size = newSizewhile holding the mutex - Lines 1322 and 1327 read buffer_size while holding the mutex
This mixing of atomic and mutex-protected access is inconsistent. Either:
- Make buffer_size a plain size_t and always access it under the mutex lock, OR
- Use only atomic operations (no mutex) for buffer_size access, OR
- Document that unlocked atomic reads are intentional for performance but ensure the logic handles potential races correctly
The current implementation at line 1039 could read a stale/intermediate buffer_size value during a concurrent setBufferSize call, though this particular race may be benign.
|
|
||
| free_on_delete = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing assignment operator (operator=) for PgSQL_Event class. The class has:
- A custom copy constructor that performs deep copying
- A custom destructor that conditionally frees memory
- Pointer members that need special handling
Without a custom assignment operator, the compiler-generated one will perform shallow copying of pointers, leading to double-free bugs or memory leaks. This violates the Rule of Three/Five in C++.
Either explicitly delete the assignment operator if copying should only happen via copy constructor, or implement a proper deep-copy assignment operator matching the copy constructor semantics.
Suggested change
| PgSQL_Event& PgSQL_Event::operator=(const PgSQL_Event& other) { | |
| if (this == &other) { | |
| return *this; | |
| } | |
| if (free_on_delete == true) { | |
| if (username != nullptr) { | |
| free(username); username = nullptr; | |
| } | |
| if (schemaname != nullptr) { | |
| free(schemaname); schemaname = nullptr; | |
| } | |
| if (query_ptr != nullptr) { | |
| free(query_ptr); query_ptr = nullptr; | |
| } | |
| if (server != nullptr) { | |
| free(server); server = nullptr; | |
| } | |
| if (client != nullptr) { | |
| free(client); client = nullptr; | |
| } | |
| if (extra_info != nullptr) { | |
| free(extra_info); extra_info = nullptr; | |
| } | |
| if (client_stmt_name != nullptr) { | |
| free(client_stmt_name); client_stmt_name = nullptr; | |
| } | |
| if (sqlstate != nullptr) { | |
| free(sqlstate); sqlstate = nullptr; | |
| } | |
| if (errmsg != nullptr) { | |
| free(errmsg); errmsg = nullptr; | |
| } | |
| } | |
| memcpy(this, &other, sizeof(PgSQL_Event)); | |
| if (other.username != nullptr) { | |
| username = strdup(other.username); | |
| } | |
| if (other.schemaname != nullptr) { | |
| schemaname = strdup(other.schemaname); | |
| } | |
| if (other.query_ptr != nullptr) { | |
| size_t maxQueryLen = get_pgsql_eventslog_buffer_max_query_length_runtime(); | |
| size_t lenToCopy = std::min(other.query_len, maxQueryLen); | |
| query_ptr = (char*)malloc(lenToCopy + 1); | |
| memcpy(query_ptr, other.query_ptr, lenToCopy); | |
| query_ptr[lenToCopy] = '\0'; | |
| query_len = lenToCopy; | |
| } | |
| if (other.server != nullptr) { | |
| server = (char*)malloc(server_len + 1); | |
| memcpy(server, other.server, server_len); | |
| server[server_len] = '\0'; | |
| } | |
| if (other.client != nullptr) { | |
| client = (char*)malloc(client_len + 1); | |
| memcpy(client, other.client, client_len); | |
| client[client_len] = '\0'; | |
| } | |
| if (other.extra_info != nullptr) { | |
| extra_info = strdup(other.extra_info); | |
| } | |
| if (other.client_stmt_name != nullptr) { | |
| client_stmt_name = strdup(other.client_stmt_name); | |
| } | |
| if (other.sqlstate != nullptr) { | |
| sqlstate = strdup(other.sqlstate); | |
| } | |
| if (other.errmsg != nullptr) { | |
| errmsg = strdup(other.errmsg); | |
| } | |
| free_on_delete = true; | |
| return *this; | |
| } |
Comment on lines
+1485
to
+1502
| std::unordered_map<std::string, unsigned long long> PgSQL_Logger::getAllMetrics() const { | ||
| std::unordered_map<std::string, unsigned long long> allMetrics; | ||
|
|
||
| allMetrics["memoryCopyCount"] = metrics.memoryCopyCount; | ||
| allMetrics["diskCopyCount"] = metrics.diskCopyCount; | ||
| allMetrics["getAllEventsCallsCount"] = metrics.getAllEventsCallsCount; | ||
| allMetrics["getAllEventsEventsCount"] = metrics.getAllEventsEventsCount; | ||
| allMetrics["totalMemoryCopyTimeMicros"] = metrics.totalMemoryCopyTimeMicros; | ||
| allMetrics["totalDiskCopyTimeMicros"] = metrics.totalDiskCopyTimeMicros; | ||
| allMetrics["totalGetAllEventsTimeMicros"] = metrics.totalGetAllEventsTimeMicros; | ||
| allMetrics["totalEventsCopiedToMemory"] = metrics.totalEventsCopiedToMemory; | ||
| allMetrics["totalEventsCopiedToDisk"] = metrics.totalEventsCopiedToDisk; | ||
| allMetrics["circularBufferEventsAddedCount"] = PgLogCB->getEventsAddedCount(); | ||
| allMetrics["circularBufferEventsDroppedCount"] = PgLogCB->getEventsDroppedCount(); | ||
| allMetrics["circularBufferEventsSize"] = PgLogCB->size(); | ||
|
|
||
| return allMetrics; | ||
| } |
There was a problem hiding this comment.
The getAllMetrics() method is marked const but calls PgLogCB->size(), PgLogCB->getEventsAddedCount(), and PgLogCB->getEventsDroppedCount(). The size() method acquires a mutex lock (line 1343), which is a non-const operation. This violates const-correctness - a const method should not modify object state (including acquiring locks on member objects).
While this may compile because PgLogCB is a pointer (not const-qualified), it's semantically incorrect and could lead to undefined behavior with const objects. Either:
- Remove const from getAllMetrics(), or
- Make the circular buffer methods const and use mutable mutex, or
- Return cached/snapshot values instead of calling into the circular buffer
Comment on lines
+1433
to
+1483
| int PgSQL_Logger::processEvents(SQLite3DB* statsdb, SQLite3DB* statsdb_disk) { | ||
| unsigned long long startTimeMicros = monotonic_time(); | ||
| std::vector<PgSQL_Event*> events = {}; | ||
| PgLogCB->get_all_events(events); | ||
| metrics.getAllEventsCallsCount++; | ||
| if (events.empty()) return 0; | ||
|
|
||
| unsigned long long afterGetAllEventsTimeMicros = monotonic_time(); | ||
| metrics.getAllEventsEventsCount += events.size(); | ||
| metrics.totalGetAllEventsTimeMicros += (afterGetAllEventsTimeMicros - startTimeMicros); | ||
|
|
||
| if (statsdb_disk != nullptr) { | ||
| unsigned long long diskStartTimeMicros = monotonic_time(); | ||
| insertPgSQLEventsIntoDb(statsdb_disk, "history_pgsql_query_events", events.size(), events.begin()); | ||
| unsigned long long diskEndTimeMicros = monotonic_time(); | ||
| metrics.diskCopyCount++; | ||
| metrics.totalDiskCopyTimeMicros += (diskEndTimeMicros - diskStartTimeMicros); | ||
| metrics.totalEventsCopiedToDisk += events.size(); | ||
| } | ||
|
|
||
| if (statsdb != nullptr) { | ||
| unsigned long long memoryStartTimeMicros = monotonic_time(); | ||
| size_t maxInMemorySize = get_pgsql_eventslog_table_memory_size_runtime(); | ||
| size_t numEventsToInsert = std::min(events.size(), maxInMemorySize); | ||
|
|
||
| if (events.size() >= maxInMemorySize) { | ||
| statsdb->execute("DELETE FROM stats_pgsql_query_events"); | ||
| } else { | ||
| int current_rows = statsdb->return_one_int((char*)"SELECT COUNT(*) FROM stats_pgsql_query_events"); | ||
| int rows_to_keep = maxInMemorySize - events.size(); | ||
| if (current_rows > rows_to_keep) { | ||
| int rows_to_delete = (current_rows - rows_to_keep); | ||
| std::string query = "SELECT MAX(id) FROM (SELECT id FROM stats_pgsql_query_events ORDER BY id LIMIT " + std::to_string(rows_to_delete) + ")"; | ||
| int maxIdToDelete = statsdb->return_one_int(query.c_str()); | ||
| std::string delete_stmt = "DELETE FROM stats_pgsql_query_events WHERE id <= " + std::to_string(maxIdToDelete); | ||
| statsdb->execute(delete_stmt.c_str()); | ||
| } | ||
| } | ||
| insertPgSQLEventsIntoDb(statsdb, "stats_pgsql_query_events", numEventsToInsert, events.begin()); | ||
| unsigned long long memoryEndTimeMicros = monotonic_time(); | ||
| metrics.memoryCopyCount++; | ||
| metrics.totalMemoryCopyTimeMicros += (memoryEndTimeMicros - memoryStartTimeMicros); | ||
| metrics.totalEventsCopiedToMemory += numEventsToInsert; | ||
| } | ||
|
|
||
| for (PgSQL_Event* event : events) { | ||
| delete event; | ||
| } | ||
|
|
||
| return events.size(); | ||
| } |
There was a problem hiding this comment.
The processEvents method lacks exception safety. If insertPgSQLEventsIntoDb throws an exception (e.g., due to SQLite errors at lines 1446 or 1471), the events vector will not be cleaned up and all PgSQL_Event pointers will leak memory. The cleanup loop at lines 1478-1480 will never execute.
Consider wrapping the events vector in a smart pointer container or using RAII (e.g., unique_ptr with custom deleter) to ensure cleanup happens even during exception unwinding. Alternatively, wrap the insertion calls in try-catch blocks with cleanup in the catch handler.
lib/PgSQL_Logger.cpp
Outdated
Comment on lines
+1461
to
+1464
| int current_rows = statsdb->return_one_int((char*)"SELECT COUNT(*) FROM stats_pgsql_query_events"); | ||
| int rows_to_keep = maxInMemorySize - events.size(); | ||
| if (current_rows > rows_to_keep) { | ||
| int rows_to_delete = (current_rows - rows_to_keep); |
There was a problem hiding this comment.
Potential signed/unsigned mismatch and overflow risk. The variable rows_to_keep is calculated as maxInMemorySize - events.size() where both operands are size_t (unsigned), but the result is stored in int (signed). If events.size() > maxInMemorySize (which shouldn't happen based on line 1456's std::min, but defensive programming matters), this would cause unsigned wraparound before conversion to signed int, resulting in a very large positive or negative value.
Similarly, current_rows and rows_to_delete are int but compared/used with size_t values. Consider using size_t or adding explicit bounds checking to prevent potential overflow or comparison issues.
Suggested change
| int current_rows = statsdb->return_one_int((char*)"SELECT COUNT(*) FROM stats_pgsql_query_events"); | |
| int rows_to_keep = maxInMemorySize - events.size(); | |
| if (current_rows > rows_to_keep) { | |
| int rows_to_delete = (current_rows - rows_to_keep); | |
| int current_rows_int = statsdb->return_one_int((char*)"SELECT COUNT(*) FROM stats_pgsql_query_events"); | |
| if (current_rows_int < 0) { | |
| current_rows_int = 0; | |
| } | |
| size_t current_rows = static_cast<size_t>(current_rows_int); | |
| size_t rows_to_keep = 0; | |
| if (maxInMemorySize > events.size()) { | |
| rows_to_keep = maxInMemorySize - events.size(); | |
| } | |
| if (current_rows > rows_to_keep) { | |
| size_t rows_to_delete = current_rows - rows_to_keep; |
…memory table Problem 1: - `PgSQL_Event::set_error()` stored borrowed pointers from connection-owned strings. - This could create lifetime hazards before/around event serialization and buffering. Fix: - Add explicit ownership tracking for error fields via `free_error_on_delete`. - Make `set_error()` duplicate incoming SQLSTATE/error strings. - Free owned error strings in destructor when the event does not own all fields. - Keep copy-constructor behavior for deep-copied buffered events and disable copy-assignment to avoid accidental shallow assignment. Problem 2: - `processEvents()` could repopulate `stats_pgsql_query_events` using the beginning of the drained batch after retention pruning. - This keeps older rows instead of the most recent rows when truncation is required. Fix: - Insert the tail of the drained batch (`events.end() - numEventsToInsert`) so memory table retention always preserves latest events. - Tighten row retention arithmetic using `size_t` to avoid signed/unsigned mismatch in keep/delete calculations.
…insert path Admin command behavior: - `DUMP PGSQL EVENTSLOG ...` returned `OK 0` when `GloPgSQL_Logger` was null. - This masked unavailable logger state from operators. Change: - Return an explicit admin error (`PgSQL logger not initialized`) when logger is unavailable. - Preserve normal `OK` response path when logger is present. Stats path behavior: - PgSQL logger metrics insertion into `stats_pgsql_global` used per-row string formatting and `execute()` calls. Change: - Replace per-row dynamic SQL construction with a prepared-statement row insert path. - Reuse `sqlite3_global_stats_row_step()` for consistent binding/reset semantics.
…time cleanup Test diagnostics: - Add explicit `diag()` output for successful-row assertion failures. - Include expected row count, observed row count, and query status for faster CI triage. Cleanup behavior: - Extend cleanup to restore: - `pgsql-eventslog_default_log=0` - `pgsql-eventslog_buffer_history_size=0` - Apply both runtime loads during cleanup: - `LOAD ADMIN VARIABLES TO RUNTIME` - `LOAD PGSQL VARIABLES TO RUNTIME` This avoids residual PGSQL eventslog settings leaking into subsequent TAP tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/tap/tests/pgsql_query_logging_autodump-t.cpp (1)
125-191: Ensure cleanup runs on early exits to avoid cross-test config leakage.
Severalreturn exit_status()paths occur after runtime variables are changed. A small cleanup helper (invoked before early returns and at the end) will keep TAP runs isolated.♻️ Suggested refactor (centralize cleanup)
@@ - bool setup_ok = true; + auto run_cleanup = [&]() { + if (!admin_conn) return; + bool cleanup_ok = true; + cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "SET admin-stats_pgsql_eventslog_sync_buffer_to_disk=0"); + cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "SET pgsql-eventslog_default_log=0"); + cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "SET pgsql-eventslog_buffer_history_size=0"); + cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "LOAD ADMIN VARIABLES TO RUNTIME"); + cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "LOAD PGSQL VARIABLES TO RUNTIME"); + ok(cleanup_ok, "Cleanup completed and auto-dump scheduler disabled"); + }; + + bool setup_ok = true; @@ - if (!setup_ok) { - return exit_status(); - } + if (!setup_ok) { + run_cleanup(); + return exit_status(); + } @@ - if (!baseline_ok) { - return exit_status(); - } + if (!baseline_ok) { + run_cleanup(); + return exit_status(); + } @@ - if (!run_queries_ok) { - return exit_status(); - } + if (!run_queries_ok) { + run_cleanup(); + return exit_status(); + } @@ - bool cleanup_ok = true; - cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "SET admin-stats_pgsql_eventslog_sync_buffer_to_disk=0"); - cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "SET pgsql-eventslog_default_log=0"); - cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "SET pgsql-eventslog_buffer_history_size=0"); - cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "LOAD ADMIN VARIABLES TO RUNTIME"); - cleanup_ok = cleanup_ok && exec_ok(admin_conn.get(), "LOAD PGSQL VARIABLES TO RUNTIME"); - ok(cleanup_ok, "Cleanup completed and auto-dump scheduler disabled"); + run_cleanup();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/pgsql_query_logging_autodump-t.cpp` around lines 125 - 191, The test currently returns early from multiple places (after setup and workload generation) without restoring runtime vars, causing cross-test leakage; add a small helper function (e.g., cleanup_test_env or restore_pgsql_eventslog_settings) that runs the existing cleanup sequence (the same calls using exec_ok with "SET admin-stats_pgsql_eventslog_sync_buffer_to_disk=0", "SET pgsql-eventslog_default_log=0", "SET pgsql-eventslog_buffer_history_size=0", "LOAD ADMIN VARIABLES TO RUNTIME", "LOAD PGSQL VARIABLES TO RUNTIME") and call this helper before every early return (the return exit_status() after setup_ok and baseline_ok and run_queries_ok failures) as well as at the end in place of the inline cleanup_ok block to centralize cleanup and prevent config leakage.lib/PgSQL_Logger.cpp (1)
222-235: Uncheckedmallocreturns in copy constructor (Cppcheck).Lines 222, 228, and 233 each call
mallocand immediately pass the result tomemcpywithout a null check. An OOM return frommallocwould produce a null-pointer dereference. This follows ProxySQL's established OOM-as-fatal convention, so it may be intentional, but the pattern warrants awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/PgSQL_Logger.cpp` around lines 222 - 235, The copy constructor in PgSQL_Logger (allocations for query_ptr, server, client) calls malloc and then memcpy without checking for null; add checks after each malloc (for query_ptr, server, client) and handle OOM consistently with the project's fatal-on-OOM policy (e.g., log an error via the logger or process logger and abort/exit or call the project-wide OOM helper), ensuring you do not call memcpy if malloc returned nullptr and that allocated lengths (query_len, server_len, client_len) remain consistent on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/PgSQL_Logger.cpp`:
- Around line 1059-1062: Change PgLogCB->buffer_size to be std::atomic<size_t>
to avoid the data race: update the declaration of buffer_size in class
PgSQL_Logger_CircularBuffer to std::atomic<size_t>, replace plain assignments in
setBufferSize() with atomic stores (while keeping the existing mutex-protected
drain logic), and leave reads in the hot path (e.g., the check in the block
using PgLogCB->buffer_size and any getBufferSize()) as atomic loads; this
preserves semantics for insert()/new PgSQL_Event(me) while eliminating the UB
race.
- Around line 328-342: The set_error method can overwrite strdup'd
sqlstate/errmsg on copy-constructed objects when free_on_delete == true; modify
PgSQL_Event::set_error to free any existing sqlstate and errmsg if
free_error_on_delete is true (regardless of free_on_delete) before assigning new
strdup'd values, ensuring you check each pointer for nullptr before free and
then strdup the incoming _sqlstate/_errmsg and set free_error_on_delete = true.
---
Duplicate comments:
In `@lib/PgSQL_Logger.cpp`:
- Around line 1492-1498: The fix to preserve the newest events is already
correctly implemented: when events.size() > numEventsToInsert the code sets
begin_it = events.end() - numEventsToInsert and passes that iterator into
insertPgSQLEventsIntoDb, so no code change is required for
insertPgSQLEventsIntoDb, begin_it, events or numEventsToInsert; mark this review
comment as duplicate/resolved and remove or close the redundant remark.
- Around line 328-342: The original dangling-pointer issue in
PgSQL_Event::set_error (sqlstate/errmsg receiving raw const char* from
temporaries) is resolved by duplicating inputs with strdup and setting
free_error_on_delete = true; verify that PgSQL_Event::set_error consistently
frees any previously owned sqlstate/errmsg before replacing them (the existing
conditional frees the previous buffers) and keep free_error_on_delete set to
true so the destructor/cleanup logic knows these buffers are owned and will be
freed.
---
Nitpick comments:
In `@lib/PgSQL_Logger.cpp`:
- Around line 222-235: The copy constructor in PgSQL_Logger (allocations for
query_ptr, server, client) calls malloc and then memcpy without checking for
null; add checks after each malloc (for query_ptr, server, client) and handle
OOM consistently with the project's fatal-on-OOM policy (e.g., log an error via
the logger or process logger and abort/exit or call the project-wide OOM
helper), ensuring you do not call memcpy if malloc returned nullptr and that
allocated lengths (query_len, server_len, client_len) remain consistent on
error.
In `@test/tap/tests/pgsql_query_logging_autodump-t.cpp`:
- Around line 125-191: The test currently returns early from multiple places
(after setup and workload generation) without restoring runtime vars, causing
cross-test leakage; add a small helper function (e.g., cleanup_test_env or
restore_pgsql_eventslog_settings) that runs the existing cleanup sequence (the
same calls using exec_ok with "SET
admin-stats_pgsql_eventslog_sync_buffer_to_disk=0", "SET
pgsql-eventslog_default_log=0", "SET pgsql-eventslog_buffer_history_size=0",
"LOAD ADMIN VARIABLES TO RUNTIME", "LOAD PGSQL VARIABLES TO RUNTIME") and call
this helper before every early return (the return exit_status() after setup_ok
and baseline_ok and run_queries_ok failures) as well as at the end in place of
the inline cleanup_ok block to centralize cleanup and prevent config leakage.
… internals Address two minor correctness/style follow-ups from final review: 1) `PgSQL_Event::set_error()` overwrite safety - Free previous `sqlstate` / `errmsg` buffers when either ownership mode is active (`free_on_delete` or `free_error_on_delete`) before replacing pointers. - This avoids latent leaks if `set_error()` is called on deep-copied events. 2) Explicit atomic access for circular-buffer capacity - Keep `buffer_size` atomic and use explicit `load/store` operations with `std::memory_order_relaxed` in hot-path and helper methods. - Update log-path check, insert path, getter, and setter to use explicit atomic operations for clearer thread-safety semantics.
Add the new autodump E2E test to the default-g4 test group configuration.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/PgSQL_Logger.cpp`:
- Around line 1474-1492: The code currently treats maxInMemorySize == 0 as a
valid size, causing the unconditional DELETE; change the logic in the block
using get_pgsql_eventslog_table_memory_size_runtime() so that when
maxInMemorySize == 0 (the "disabled" sentinel) you skip the
trimming/delete/insert logic entirely (e.g., early continue/return or bypass the
stats_pgsql_query_events maintenance) — update the code around statsdb,
maxInMemorySize, and events handling (the block using statsdb->execute and
return_one_int and the variable numEventsToInsert) to explicitly guard on
maxInMemorySize > 0 before performing any DELETE or row-pruning operations.
- Around line 219-236: In the copy constructor where query_ptr, server, and
client are allocated (using malloc with
get_pgsql_eventslog_buffer_max_query_length_runtime(), and variables query_len,
server_len, client_len), add null-checks for each malloc result: if malloc
returns nullptr, avoid calling memcpy/setting terminator; instead handle OOM by
either throwing std::bad_alloc (or returning/setting the member pointer to
nullptr and corresponding length to 0) and clean up any already-allocated
members to avoid leaks; ensure each allocation path validates the pointer before
memcpy and sets a deterministic state (pointer and length) on failure.
Comment on lines
+219
to
+236
| if (other.query_ptr != nullptr) { | ||
| size_t maxQueryLen = get_pgsql_eventslog_buffer_max_query_length_runtime(); | ||
| size_t lenToCopy = std::min(other.query_len, maxQueryLen); | ||
| query_ptr = (char*)malloc(lenToCopy + 1); | ||
| memcpy(query_ptr, other.query_ptr, lenToCopy); | ||
| query_ptr[lenToCopy] = '\0'; | ||
| query_len = lenToCopy; | ||
| } | ||
| if (other.server != nullptr) { | ||
| server = (char*)malloc(server_len + 1); | ||
| memcpy(server, other.server, server_len); | ||
| server[server_len] = '\0'; | ||
| } | ||
| if (other.client != nullptr) { | ||
| client = (char*)malloc(client_len + 1); | ||
| memcpy(client, other.client, client_len); | ||
| client[client_len] = '\0'; | ||
| } |
There was a problem hiding this comment.
Missing null-check after malloc in copy constructor.
Lines 223, 229, and 234: if malloc returns nullptr (OOM), the subsequent memcpy dereferences a null pointer (UB). All three strdup-based copies in the same constructor carry the same latent risk (cppcheck also flags lines 31, 86 in the broader file).
🛡️ Proposed defensive fix
if (other.query_ptr != nullptr) {
size_t maxQueryLen = get_pgsql_eventslog_buffer_max_query_length_runtime();
size_t lenToCopy = std::min(other.query_len, maxQueryLen);
query_ptr = (char*)malloc(lenToCopy + 1);
+ if (query_ptr == nullptr) { proxy_error("malloc failed for query_ptr\n"); return; }
memcpy(query_ptr, other.query_ptr, lenToCopy);
query_ptr[lenToCopy] = '\0';
query_len = lenToCopy;
}
if (other.server != nullptr) {
server = (char*)malloc(server_len + 1);
+ if (server == nullptr) { proxy_error("malloc failed for server\n"); return; }
memcpy(server, other.server, server_len);
server[server_len] = '\0';
}
if (other.client != nullptr) {
client = (char*)malloc(client_len + 1);
+ if (client == nullptr) { proxy_error("malloc failed for client\n"); return; }
memcpy(client, other.client, client_len);
client[client_len] = '\0';
}🧰 Tools
🪛 Cppcheck (2.19.0)
[warning] 223-223: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 224-224: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 229-229: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 230-230: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 234-234: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 235-235: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PgSQL_Logger.cpp` around lines 219 - 236, In the copy constructor where
query_ptr, server, and client are allocated (using malloc with
get_pgsql_eventslog_buffer_max_query_length_runtime(), and variables query_len,
server_len, client_len), add null-checks for each malloc result: if malloc
returns nullptr, avoid calling memcpy/setting terminator; instead handle OOM by
either throwing std::bad_alloc (or returning/setting the member pointer to
nullptr and corresponding length to 0) and clean up any already-allocated
members to avoid leaks; ensure each allocation path validates the pointer before
memcpy and sets a deterministic state (pointer and length) on failure.
Comment on lines
+1474
to
+1492
| if (statsdb != nullptr) { | ||
| unsigned long long memoryStartTimeMicros = monotonic_time(); | ||
| size_t maxInMemorySize = get_pgsql_eventslog_table_memory_size_runtime(); | ||
| size_t numEventsToInsert = std::min(events.size(), maxInMemorySize); | ||
|
|
||
| if (events.size() >= maxInMemorySize) { | ||
| statsdb->execute("DELETE FROM stats_pgsql_query_events"); | ||
| } else { | ||
| int current_rows_i = statsdb->return_one_int((char*)"SELECT COUNT(*) FROM stats_pgsql_query_events"); | ||
| size_t current_rows = (current_rows_i > 0) ? static_cast<size_t>(current_rows_i) : 0; | ||
| size_t rows_to_keep = maxInMemorySize - events.size(); | ||
| if (current_rows > rows_to_keep) { | ||
| size_t rows_to_delete = (current_rows - rows_to_keep); | ||
| std::string query = "SELECT MAX(id) FROM (SELECT id FROM stats_pgsql_query_events ORDER BY id LIMIT " + std::to_string(rows_to_delete) + ")"; | ||
| int maxIdToDelete = statsdb->return_one_int(query.c_str()); | ||
| std::string delete_stmt = "DELETE FROM stats_pgsql_query_events WHERE id <= " + std::to_string(maxIdToDelete); | ||
| statsdb->execute(delete_stmt.c_str()); | ||
| } | ||
| } |
There was a problem hiding this comment.
maxInMemorySize == 0 silently clears stats_pgsql_query_events on every call.
When get_pgsql_eventslog_table_memory_size_runtime() returns 0 and statsdb != nullptr, the condition events.size() >= maxInMemorySize is size_t >= 0 which is always true (unsigned), so DELETE FROM stats_pgsql_query_events runs every invocation and no rows are ever inserted. If 0 is meant as a "disabled" sentinel, guard it explicitly before reaching the delete/insert logic.
🛡️ Proposed fix
if (statsdb != nullptr) {
+ size_t maxInMemorySize = get_pgsql_eventslog_table_memory_size_runtime();
+ if (maxInMemorySize == 0) {
+ // Memory table disabled; skip entirely.
+ } else {
unsigned long long memoryStartTimeMicros = monotonic_time();
- size_t maxInMemorySize = get_pgsql_eventslog_table_memory_size_runtime();
...
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PgSQL_Logger.cpp` around lines 1474 - 1492, The code currently treats
maxInMemorySize == 0 as a valid size, causing the unconditional DELETE; change
the logic in the block using get_pgsql_eventslog_table_memory_size_runtime() so
that when maxInMemorySize == 0 (the "disabled" sentinel) you skip the
trimming/delete/insert logic entirely (e.g., early continue/return or bypass the
stats_pgsql_query_events maintenance) — update the code around statsdb,
maxInMemorySize, and events handling (the block using statsdb->execute and
return_one_int and the variable numEventsToInsert) to explicitly guard on
maxInMemorySize > 0 before performing any DELETE or row-pruning operations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces PostgreSQL advanced query logging with parity to MySQL eventslog behavior, including manual dump, scheduler auto-dump, docs, and validation coverage.
Key Changes
DUMP PGSQL EVENTSLOG ...command support in the shared Admin module.60326132databasefield naming for PostgreSQL query events.sqlstateanderror.Validation
Closes #5390
Summary by CodeRabbit
New Features
Documentation
Tests
Chores