From 82c7e77a4b221ed5dcb1685a937af7fef8e1fee4 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Tue, 4 Aug 2026 00:55:27 +0200 Subject: [PATCH 1/2] PG-2395 Remove code for colecting query stats in ExectuorEnd As the query stats object is now guaranteed, since commit d25da647db1cf5674433849e8de2220607c8b465, to have been created in the ExecutorStart hook there is no need to also try to create it in the ExecutorEnd hook. This makes to code clearer for the reader, plus makes it obvious that we no longer can do any catcache updates before accessing the totaltime object. --- src/pg_stat_monitor.c | 44 +++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/pg_stat_monitor.c b/src/pg_stat_monitor.c index d07be412..4d9630de 100644 --- a/src/pg_stat_monitor.c +++ b/src/pg_stat_monitor.c @@ -234,7 +234,8 @@ static void pgsm_fill_query_exec_info(pgsmQueryExecInfo *info); static pgsmQueryStats *pgsm_add_query_stats(int64 queryid, int64 planid, int64 pgsm_query_id, const char *query_text, int query_len, CmdType cmd_type); static void pgsm_fill_query_stats(pgsmQueryStats *stats, const pgsmQueryExecInfo *info, int64 queryid, int64 planid, int64 pgsm_query_id, const char *query_text, CmdType cmd_type); static void pgsm_delete_query_stats(uint64 queryid); -static pgsmQueryStats *pgsm_get_query_stats(int64 queryid, int64 planid, const char *query_text, CmdType cmd_type); +static pgsmQueryStats *pgsm_get_query_stats(int64 queryid); +static pgsmQueryStats *pgsm_get_or_add_query_stats(int64 queryid, int64 planid, const char *query_text, CmdType cmd_type); static int64 get_pgsm_query_id_hash(const char *norm_query, int len); static void pgsm_cleanup_callback(void *arg); @@ -550,8 +551,8 @@ pgsm_ExecutorStart(QueryDesc *queryDesc, int eflags) * snapshot of the execution info (application_name, user) reflects * the state at statement start. */ - (void) pgsm_get_query_stats(queryDesc->plannedstmt->queryId, 0, - queryDesc->sourceText, queryDesc->operation); + (void) pgsm_get_or_add_query_stats(queryDesc->plannedstmt->queryId, 0, + queryDesc->sourceText, queryDesc->operation); /* * Set up to track total elapsed time in ExecutorRun. Make sure the @@ -700,7 +701,9 @@ pgsm_ExecutorEnd(QueryDesc *queryDesc) SysInfo sys_info; int64 planid = plan_ptr ? plan_ptr->planid : 0; - stats = pgsm_get_query_stats(queryId, planid, queryDesc->sourceText, queryDesc->operation); + /* We know at least pgsm_ExecutorStart() has created query stats */ + stats = pgsm_get_query_stats(queryId); + Assert(stats != NULL); if (stats->key.planid == 0 && planid != 0) stats->key.planid = planid; @@ -865,7 +868,7 @@ pgsm_planner_hook(Query *parse, const char *query_string, int cursorOptions, Par walusage_start = pgWalUsage; INSTR_TIME_SET_CURRENT(start); - stats = pgsm_get_query_stats(queryId, 0, query_string, parse->commandType); + stats = pgsm_get_or_add_query_stats(queryId, 0, query_string, parse->commandType); #if PG_VERSION_NUM >= 170000 nesting_level++; @@ -1628,23 +1631,37 @@ pgsm_delete_query_stats(uint64 queryid) } } -/* - * Function to get a pgsmQueryStats structure from the local list. - */ static pgsmQueryStats * -pgsm_get_query_stats(int64 queryid, int64 planid, const char *query_text, CmdType cmd_type) +pgsm_get_or_add_query_stats(int64 queryid, int64 planid, const char *query_text, CmdType cmd_type) { pgsmQueryStats *stats; int query_len; Assert(query_text != NULL); + stats = pgsm_get_query_stats(queryid); + if (stats != NULL) + return stats; + + query_len = strlen(query_text); + return pgsm_add_query_stats(queryid, planid, + get_pgsm_query_id_hash(query_text, query_len), + query_text, query_len, cmd_type); +} + +/* + * Function to get a pgsmQueryStats structure from the local list. + */ +static pgsmQueryStats * +pgsm_get_query_stats(int64 queryid) +{ if (lentries != NIL) { + pgsmQueryStats *stats; ListCell *lc; /* First bet is on the last item */ - stats = (pgsmQueryStats *) llast(lentries); + stats = llast(lentries); if (stats->key.queryid == queryid) return stats; @@ -1656,12 +1673,7 @@ pgsm_get_query_stats(int64 queryid, int64 planid, const char *query_text, CmdTyp } } - query_len = strlen(query_text); - stats = pgsm_add_query_stats(queryid, planid, - get_pgsm_query_id_hash(query_text, query_len), - query_text, query_len, cmd_type); - - return stats; + return NULL; } static void From 85ce18197b1ebcb16021d075b24afd928b0594d5 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Tue, 4 Aug 2026 01:01:47 +0200 Subject: [PATCH 2/2] PG-2395 Add changelog item about how hopefully an RDS bug is fixed One of our users reported a likely use after free bug on Amazon RDS that we could not reproduce where queryDesc->totaltime was freed when we accessed it. And while we failed to reproduce the bug a theory of ours is that in some cases a catalog cache lookup in ExecutorEnd could cause interrupts to be processed which then would for unknown reasons cause the totaltime object to be freed. If this theory is correct then as we no longer do any catalog cache lookups in the ExecutorEnd hook then the bug should be gone. This is of course all very speculative. Bug report: https://github.com/percona/pg_stat_monitor/issues/651 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d3a59b6..237fd886 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,3 +33,4 @@ - Do not access `PlannedStmt` after we call `standard_ProcessUtility()` ([PG-2486](https://perconadev.atlassian.net/browse/PG-2486)) - Various improvements to the stability of our test suite - Make sure that for prepared statements utility statement exec info read at the executor start hook, where data is not yet modified by query itself +- Attempt to fix use after free when running on RDS ([PG-2395](https://perconadev.atlassian.net/browse/PG-2395))