Add backend_pid and backend_state to stats_pgsql_processlist and create stats_pgsql_stat_activity view#5156
Conversation
…ctivity-style view - Added `backend_pid` and `backend_state` columns to `stats_pgsql_processlist` to display PostgreSQL backend process ID and connection state. - Created `stats_pgsql_stat_activity` view on top of `stats_pgsql_processlist` with column aliases matching PostgreSQL's `pg_stat_activity` for consistency.
Summary of ChangesHello @rahim-kanji, 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 improves PostgreSQL monitoring capabilities by adding more detailed process information to the existing statistics table and introducing a new view that mirrors PostgreSQL's Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request enhances PostgreSQL process monitoring by adding backend_pid and backend_state to the stats_pgsql_processlist table and creating a new stats_pgsql_stat_activity view for compatibility with PostgreSQL's native pg_stat_activity. The changes are well-implemented and functional.
My review includes a few suggestions to improve code quality:
- A correction for a typo in a macro name to ensure consistency.
- Recommendations to refactor parts of the code that manually manage array indices by using enums. This will improve readability and make the code less prone to errors in future modifications.
Overall, this is a valuable addition. Addressing the suggested points will enhance the maintainability of the code.
|
|
||
| SQLite3_result* PgSQL_Threads_Handler::SQL3_Processlist() { | ||
| const int colnum = 16; | ||
| const int colnum = 18; |
There was a problem hiding this comment.
The manual management of array indices for the pta array is error-prone and hard to maintain, especially when adding or removing columns as done in this PR. To improve readability and maintainability, consider defining an enum for the column indices. This would make the code self-documenting and less fragile to future changes.
For example:
enum PgSQLProcesslistColumn {
PGL_THREAD_ID,
PGL_SESSION_ID,
PGL_USER,
PGL_DATABASE,
PGL_CLI_HOST,
PGL_CLI_PORT,
PGL_HOSTGROUP,
PGL_L_SRV_HOST,
PGL_L_SRV_PORT,
PGL_SRV_HOST,
PGL_SRV_PORT,
PGL_BACKEND_PID,
PGL_BACKEND_STATE,
PGL_COMMAND,
PGL_TIME_MS,
PGL_INFO,
PGL_STATUS_FLAGS,
PGL_EXTENDED_INFO,
PGL_COLNUM
};
// In SQL3_Processlist():
const int colnum = PGL_COLNUM;
// ...
// Then use the enum members as indices:
// pta[PGL_SESSION_ID] = strdup(buf);
// ...
// pta[PGL_COMMAND] = strdup("Query");| query1 = (char*)"INSERT OR IGNORE INTO stats_pgsql_processlist VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17, ?18)"; | ||
| query32s = "INSERT OR IGNORE INTO stats_pgsql_processlist VALUES " + generate_multi_rows_query(32, 18); | ||
| query32 = (char*)query32s.c_str(); |
There was a problem hiding this comment.
The manual management of array indices for the prepared statement is error-prone and hard to maintain. With the addition of new columns, many indices had to be shifted manually, which increases the risk of bugs. This applies to both the bulk insert (statement32) and single-row insert (statement1) logic.
To improve readability and maintainability, I suggest defining an enum for the column indices, similar to the suggestion for lib/PgSQL_Thread.cpp. This would make the code self-documenting and less fragile to future changes.
Example:
enum PgSQLProcesslistColumn {
PGL_THREAD_ID = 1,
PGL_SESSION_ID,
// ...
PGL_BACKEND_PID,
PGL_BACKEND_STATE,
PGL_COMMAND,
// ...
};
// In stats___pgsql_processlist():
// ...
// For bulk insert:
rc = (*proxy_sqlite3_bind_int64)(statement32, (idx * 18) + PGL_THREAD_ID, atoll(r1->fields[0]));
// For single insert:
rc = (*proxy_sqlite3_bind_int64)(statement1, PGL_THREAD_ID, atoll(r1->fields[0]));
|
Summary
Enhances PostgreSQL process monitoring by extending the
stats_pgsql_processlisttable and adding a compatibility view that mirrors PostgreSQL’s nativepg_stat_activity.Changes Introduced
Added new columns to
stats_pgsql_processlist:idle,active,in transaction, etc.).Created new view:
stats_pgsql_stat_activitystats_pgsql_processlist.pg_stat_activityfor better familiarity and easier integration with monitoring tools.Closes #4682