Documentation additions and bug fix for vacuum_stats() #5258
Conversation
…stat_activity This commit documents: 1. The vacuum_stats() function's purpose, behavior, and the reason why stats_pgsql_stat_activity is excluded from bulk deletion operations 2. The fact that stats_pgsql_stat_activity is a SQL VIEW (not a table) and attempting DELETE on it would cause SQLite error: "cannot modify stats_pgsql_stat_activity because it is a view" The documentation explains: - Why TRUNCATE stats_mysql_query_digest triggers vacuum_stats(true) - Why both MySQL and PostgreSQL tables are cleared regardless of protocol - How the view is automatically cleared via its underlying table stats_pgsql_processlist - The importance of keeping the view excluded from deletion lists
Add comprehensive documentation for the on‑demand core dump generation feature introduced in PR #4225. The documentation covers: - coredump_filters and runtime_coredump_filters table definitions - LOAD COREDUMP TO RUNTIME admin command and its aliases - Configuration variables admin‑coredump_generation_threshold and admin‑coredump_generation_interval_ms - Usage examples with INSERT and LOAD commands - Important notes about case‑sensitive filenames, runtime‑only behavior, rate limiting, and platform support - Troubleshooting guidance and best practices The documentation addresses the missing documentation reported in GitHub issue #5213.
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 ProxySQL's documentation by adding comprehensive guides for several key features, including the use of regex capture groups in query rules, the two-phase process behind 'killed client connection' warnings, and the on-demand core dump generation ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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 is a great addition, providing extensive documentation for several features and fixing a bug in vacuum_stats(). The new documentation for query rules, killed connections, and coredump filters is comprehensive and will be very helpful for users. The bug fix correctly prevents DELETE operations on a SQL VIEW, and the associated code comments are clear and informative. I've identified a few issues, primarily in the new documentation files, related to the use of non-standard characters (non-breaking hyphens) which could cause issues for users copying and pasting commands. I've also found an incomplete code example. Addressing these will improve the quality and usability of the documentation.
| | `admin‑coredump_generation_threshold` | 10 | 1–500 | Maximum number of core files that can be generated during the lifetime of the ProxySQL process. | | ||
| | `admin‑coredump_generation_interval_ms` | 30000 (30 seconds) | 0–INT_MAX | Minimum time between two consecutive core dump generations. A value of `0` disables the interval check. | |
There was a problem hiding this comment.
The variable names like admin‑coredump_generation_threshold use a non-breaking hyphen (U+2011) instead of a standard hyphen-minus (U+002D). This can cause copy-paste errors for users trying to run commands. Please replace all non-breaking hyphens in variable names and code blocks with standard hyphens. For example, admin‑coredump_generation_threshold should be admin-coredump_generation_threshold. This issue appears in multiple places in this file (e.g., lines 58, 59, 108, 168, 170, 181, 237, 244) and other documentation files in this PR.
| **Investigation Steps**: | ||
| 1. **Enable audit logging**: | ||
| ```sql | ||
| SET mysql-auditlog_filename='/var/log/proxysql_audit.log'; |
There was a problem hiding this comment.
The example for enabling audit logging is incomplete. It's missing SET mysql-auditlog=true;. Without this, setting the filename will not enable logging. I suggest adding it for the example to be functional, similar to the more complete example in the 'Best Practices' section (lines 154-159).
| SET mysql-auditlog_filename='/var/log/proxysql_audit.log'; | |
| SET mysql-auditlog_filename='/var/log/proxysql_audit.log'; | |
| SET mysql-auditlog=true; |
| | `mysql‑wait_timeout` | 28800000 ms (8 hours) | Maximum idle time before connection is killed. | | ||
| | `mysql‑max_transaction_idle_time` | 0 (disabled) | Maximum idle time for an open transaction. | | ||
| | `mysql‑max_transaction_time` | 0 (disabled) | Maximum total time a transaction can run. | | ||
| | `mysql‑session_fast_forward` | false | Enable fast‑forward mode (kills connections if backends go OFFLINE). | | ||
| | `mysql‑throttle_max_transaction_time` | 0 (disabled) | Alternative to `max_transaction_time`; throttles instead of kills. | |
There was a problem hiding this comment.
The variable names in this table use a non-breaking hyphen (U+2011) instead of a standard hyphen-minus (U+002D). This can cause copy-paste errors for users. Please replace all non-breaking hyphens in variable names (e.g., mysql‑wait_timeout) and code blocks with standard hyphens (mysql-wait_timeout). This issue appears in multiple places in this file (e.g., lines 34, 41, 47, 53, 131-135, 182, 195).
|
|
||
| Suppose you want to generate a core dump when the function `MySQL_Data_Stream::check_data_flow()` reaches line 485 (where `generate_coredump()` is called). | ||
|
|
||
| First, find the exact file name as used in the source code. The macro `LOCATION()` expands to `__FILE__ ":" __LINE__`. For the file `lib/mysql_data_stream.cpp` line 485: |
There was a problem hiding this comment.
The explanation of the LOCATION() macro is slightly misleading. The expression __FILE__ ":" __LINE__ is not valid C preprocessor syntax for concatenating a string and an integer macro. The actual implementation uses stringification macros. To avoid confusion, I suggest rephrasing this to focus on the result rather than the implementation detail.
| First, find the exact file name as used in the source code. The macro `LOCATION()` expands to `__FILE__ ":" __LINE__`. For the file `lib/mysql_data_stream.cpp` line 485: | |
| First, find the exact file name as used in the source code. The internal macro `LOCATION()` creates a string literal by combining the filename and line number (e.g., `"lib/mysql_data_stream.cpp:485"`). For the file `lib/mysql_data_stream.cpp` line 485: |
|
|
||
| 1. **Use for debugging only**: Enable coredump filters only during debugging sessions. Remove filters afterward to avoid unnecessary overhead. | ||
| 2. **Limit the threshold**: Keep `admin‑coredump_generation_threshold` low (e.g., 1‑5) unless you are investigating a recurring issue. | ||
| 3. **Set a reasonable interval**: A minimum interval of several seconds (e.g., 30000 ms) prevents storm generation. |
There was a problem hiding this comment.
The text 30000 ms uses a narrow no-break space (U+202F) instead of a regular space. This can be invisible to users and might cause issues. It's better to use a standard space for consistency and to avoid potential copy-paste or rendering problems.
| 3. **Set a reasonable interval**: A minimum interval of several seconds (e.g., 30000 ms) prevents storm generation. | |
| 3. **Set a reasonable interval**: A minimum interval of several seconds (e.g., 30000 ms) prevents storm generation. |
|
Summary
This pull request adds comprehensive documentation for several ProxySQL features and includes a bug fix:
Query rules capture groups and backreferences (
6966b79d)"Closing killed client connection" warnings (
de214cb5)Doxygen documentation for vacuum_stats() and stats_pgsql_stat_activity (
efe0d4fe)stats_pgsql_stat_activityfrom the deletion list invacuum_stats()because it's a SQL VIEW, not a tableDELETEon the view would cause SQLite error:"cannot modify stats_pgsql_stat_activity because it is a view"stats_pgsql_processlistcoredump_filters feature documentation (
cf8cbfd8) - Addresses issue Document coredump_filters feature and LOAD/SAVE COREDUMP commands #5213LOAD COREDUMP TO RUNTIME), configuration variables, usage examples, and platform supportBug Fix Details
Commit
efe0d4fefixes a bug wherevacuum_stats()would attempt to delete rows fromstats_pgsql_stat_activity, which is a SQL VIEW. This would cause the SQLite error:The view is now properly excluded from the deletion list (commented out), while the documentation explains that clearing the underlying
stats_pgsql_processlisttable automatically clears the view's content.