From 2ceb47e3cd0435a1e14ae696df418283086b6f93 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Tue, 18 Apr 2023 18:01:34 +0500 Subject: [PATCH 1/4] PG-613: Postgresql crashes with Segmentation fault when query plan is enabled on large queries The return value for snprintf was incorrectly being recorded as plan length. That's been resolved. As part of this fix, we've also elminated the possibility of a potential memory leak when plan text was being saved. Co-authored-by: Hamid Akhtar Co-authored-by: Muhammad Usama --- pg_stat_monitor.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index a76a6a7..5836198 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -692,9 +692,30 @@ pgsm_ExecutorEnd(QueryDesc *queryDesc) /* Extract the plan information in case of SELECT statement */ if (queryDesc->operation == CMD_SELECT && pgsm_enable_query_plan) { - plan_info.plan_len = snprintf(plan_info.plan_text, PLAN_TEXT_LEN, "%s", pgsm_explain(queryDesc)); - plan_info.planid = pgsm_hash_string(plan_info.plan_text, plan_info.plan_len); - plan_ptr = &plan_info; + int rv; + MemoryContext oldctx; + + /* + * Making sure it is a per query context so that there's no memory + * leak when executor ends. + */ + oldctx = MemoryContextSwitchTo(queryDesc->estate->es_query_cxt); + + rv = snprintf(plan_info.plan_text, PLAN_TEXT_LEN, "%s", pgsm_explain(queryDesc)); + + /* + * If snprint didn't write anything or there was an error, let's keep + * planinfo as NULL. + */ + if (rv > 0) + { + plan_info.plan_len = (rv < PLAN_TEXT_LEN) ? rv : PLAN_TEXT_LEN - 1; + plan_info.planid = pgsm_hash_string(plan_info.plan_text, plan_info.plan_len); + plan_ptr = &plan_info; + } + + /* Switch back to old context */ + MemoryContextSwitchTo(oldctx); } if (queryId != UINT64CONST(0) && queryDesc->totaltime && pgsm_enabled(exec_nested_level)) From 39d9419bd08b800a4b83910bcb8021d511aeb9d8 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Fri, 12 May 2023 21:51:55 +0500 Subject: [PATCH 2/4] PG-624: pg_stat_monitor: Possible server crash when running pgbench with pg_stat_monitor loaded (#396) PG-624: pg_stat_monitor: Possible server crash when running pgbench with pg_stat_monitor loaded It appears that this issue was being caused by improper handling of dynamic number of buckets. This commit resolves the issue. Also, as part of a larger cleanup, memory context has been moved to local space from shared storage. Also, some unwanted and no-longer-needed variables are removed. Co-authored-by: Muhammad Usama --- hash_query.c | 21 ++++++++++++++------- pg_stat_monitor.c | 7 ++++--- pg_stat_monitor.h | 15 ++++----------- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/hash_query.c b/hash_query.c index e25a486..c18e20f 100644 --- a/hash_query.c +++ b/hash_query.c @@ -23,6 +23,9 @@ static PGSM_HASH_TABLE_HANDLE pgsm_create_bucket_hash(pgsmSharedState * pgsm, ds static Size pgsm_get_shared_area_size(void); static void InitializeSharedState(pgsmSharedState * pgsm); +#define PGSM_BUCKET_INFO_SIZE (sizeof(TimestampTz) * pgsm_max_buckets) +#define PGSM_SHARED_STATE_SIZE (sizeof(pgsmSharedState) + PGSM_BUCKET_INFO_SIZE) + #if USE_DYNAMIC_HASH /* parameter for the shared hash */ static dshash_parameters dsh_params = { @@ -56,7 +59,7 @@ pgsm_query_area_size(void) Size pgsm_ShmemSize(void) { - Size sz = MAXALIGN(sizeof(pgsmSharedState)); + Size sz = MAXALIGN(PGSM_SHARED_STATE_SIZE); sz = add_size(sz, MAX_QUERY_BUF); #if USE_DYNAMIC_HASH @@ -114,7 +117,7 @@ pgsm_startup(void) SpinLockInit(&pgsm->mutex); InitializeSharedState(pgsm); /* the allocation of pgsmSharedState itself */ - p += MAXALIGN(sizeof(pgsmSharedState)); + p += MAXALIGN(PGSM_SHARED_STATE_SIZE); pgsm->raw_dsa_area = p; dsa = dsa_create_in_place(pgsm->raw_dsa_area, pgsm_query_area_size(), @@ -138,6 +141,10 @@ pgsm_startup(void) * references. */ dsa_detach(dsa); + + pgsmStateLocal.pgsm_mem_cxt = AllocSetContextCreate(TopMemoryContext, + "pg_stat_monitor local store", + ALLOCSET_DEFAULT_SIZES); } #ifdef BENCHMARK @@ -158,10 +165,6 @@ InitializeSharedState(pgsmSharedState * pgsm) { pg_atomic_init_u64(&pgsm->current_wbucket, 0); pg_atomic_init_u64(&pgsm->prev_bucket_sec, 0); - memset(&pgsm->bucket_entry, 0, MAX_BUCKETS * sizeof(uint64)); - pgsm->pgsm_mem_cxt = AllocSetContextCreate(TopMemoryContext, - "pg_stat_monitor local store", - ALLOCSET_DEFAULT_SIZES); } @@ -235,6 +238,11 @@ pgsm_attach_shmem(void) MemoryContextSwitchTo(oldcontext); } +MemoryContext GetPgsmMemoryContext(void) +{ + return pgsmStateLocal.pgsm_mem_cxt; +} + dsa_area * get_dsa_area_for_query_text(void) { @@ -290,7 +298,6 @@ hash_entry_alloc(pgsmSharedState * pgsm, pgsmHashKey * key, int encoding) elog(DEBUG1, "[pg_stat_monitor] hash_entry_alloc: OUT OF MEMORY."); else if (!found) { - pgsm->bucket_entry[pg_atomic_read_u64(&pgsm->current_wbucket)]++; /* New entry, initialize it */ /* reset the statistics */ memset(&entry->counters, 0, sizeof(Counters)); diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index 5836198..8bd82d4 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -1590,7 +1590,7 @@ static void pgsm_add_to_list(pgsmEntry * entry, char *query_text, int query_len) { /* Switch to pgsm memory context */ - MemoryContext oldctx = MemoryContextSwitchTo(pgsm_get_ss()->pgsm_mem_cxt); + MemoryContext oldctx = MemoryContextSwitchTo(GetPgsmMemoryContext()); entry->query_text.query_pointer = pnstrdup(query_text, query_len); lentries = lappend(lentries, entry); @@ -1645,7 +1645,8 @@ static void pgsm_cleanup_callback(void *arg) { /* Reset the memory context holding the list */ - MemoryContextReset(pgsm_get_ss()->pgsm_mem_cxt); + MemoryContextReset(GetPgsmMemoryContext()); + lentries = NIL; callback_setup = false; } @@ -1666,7 +1667,7 @@ pgsm_create_hash_entry(uint64 bucket_id, uint64 queryid, PlanInfo * plan_info) char *username = NULL; /* Create an entry in the pgsm memory context */ - oldctx = MemoryContextSwitchTo(pgsm_get_ss()->pgsm_mem_cxt); + oldctx = MemoryContextSwitchTo(GetPgsmMemoryContext()); entry = palloc0(sizeof(pgsmEntry)); /* diff --git a/pg_stat_monitor.h b/pg_stat_monitor.h index 0495a3b..339f647 100644 --- a/pg_stat_monitor.h +++ b/pg_stat_monitor.h @@ -77,7 +77,6 @@ #define HISTOGRAM_MAX_TIME 50000000 #define MAX_RESPONSE_BUCKET 50 #define INVALID_BUCKET_ID -1 -#define MAX_BUCKETS 10 #define TEXT_LEN 255 #define ERROR_MESSAGE_LEN 100 #define REL_TYPENAME_LEN 64 @@ -388,8 +387,6 @@ typedef struct pgsmSharedState slock_t mutex; /* protects following fields only: */ pg_atomic_uint64 current_wbucket; pg_atomic_uint64 prev_bucket_sec; - uint64 bucket_entry[MAX_BUCKETS]; - TimestampTz bucket_start_time[MAX_BUCKETS]; /* start time of the bucket */ int hash_tranche_id; void *raw_dsa_area; /* DSA area pointer to store query texts. * dshash also lives in this memory when @@ -400,16 +397,9 @@ typedef struct pgsmSharedState * hash table handle. can be either classic shared memory hash or dshash * (if we are using USE_DYNAMIC_HASH) */ - MemoryContext pgsm_mem_cxt; - /* - * context to store stats in local memory until they are pushed to shared - * hash - */ - int resp_calls[MAX_RESPONSE_BUCKET + 2]; /* execution time's in - * msec; including 2 - * outlier buckets */ bool pgsm_oom; + TimestampTz bucket_start_time[]; /* start time of the bucket */ } pgsmSharedState; typedef struct pgsmLocalState @@ -418,6 +408,8 @@ typedef struct pgsmLocalState dsa_area *dsa; /* local dsa area for backend attached to the * dsa area created by postmaster at startup. */ PGSM_HASH_TABLE *shared_hash; + MemoryContext pgsm_mem_cxt; + } pgsmLocalState; #if PG_VERSION_NUM < 140000 @@ -479,6 +471,7 @@ void pgsm_startup(void); /* hash_query.c */ void pgsm_startup(void); +MemoryContext GetPgsmMemoryContext(void); /* guc.c */ void init_guc(void); From 639bf6f1581cc19fb9017a5d885ff8f1837ab114 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Mon, 15 May 2023 17:56:54 +0500 Subject: [PATCH 3/4] Version bumped for the 2.0.1 release. --- META.json | 6 +++--- pg_stat_monitor.c | 2 +- regression/expected/version.out | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/META.json b/META.json index 673fb3a..0475907 100644 --- a/META.json +++ b/META.json @@ -2,7 +2,7 @@ "name": "pg_stat_monitor", "abstract": "PostgreSQL Query Performance Monitoring Tool", "description": "pg_stat_monitor is a PostgreSQL Query Performance Monitoring tool, based on PostgreSQL's contrib module pg_stat_statements. PostgreSQL’s pg_stat_statements provides the basic statistics, which is sometimes not enough. The major shortcoming in pg_stat_statements is that it accumulates all the queries and their statistics and does not provide aggregated statistics nor histogram information. In this case, a user would need to calculate the aggregates, which is quite an expensive operation.", - "version": "2.0.0", + "version": "2.0.1", "maintainer": [ "ibrar.ahmed@percona.com" ], @@ -10,9 +10,9 @@ "provides": { "pg_stat_monitor": { "abstract": "PostgreSQL Query Performance Monitoring Tool", - "file": "pg_stat_monitor--1.0.sql", + "file": "pg_stat_monitor--2.0.sql", "docfile": "README.md", - "version": "2.0.0" + "version": "2.0.1" } }, "prereqs": { diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index 8bd82d4..3f1c693 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -36,7 +36,7 @@ typedef enum pgsmVersion PG_MODULE_MAGIC; -#define BUILD_VERSION "2.0.0" +#define BUILD_VERSION "2.0.1" /* Number of output arguments (columns) for various API versions */ #define PG_STAT_MONITOR_COLS_V1_0 52 diff --git a/regression/expected/version.out b/regression/expected/version.out index fe02474..60b6014 100644 --- a/regression/expected/version.out +++ b/regression/expected/version.out @@ -2,7 +2,7 @@ CREATE EXTENSION pg_stat_monitor; SELECT pg_stat_monitor_version(); pg_stat_monitor_version ------------------------- - 2.0.0 + 2.0.1 (1 row) DROP EXTENSION pg_stat_monitor; From d2f133657f811658bce90b0d4a5b9efc0c77afeb Mon Sep 17 00:00:00 2001 From: EvgeniyPatlan Date: Wed, 17 May 2023 16:38:43 +0300 Subject: [PATCH 4/4] ENG-7 Update pg-stat-monitor.spec --- rpm/pg-stat-monitor.spec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rpm/pg-stat-monitor.spec b/rpm/pg-stat-monitor.spec index 8be12cf..c9a2e58 100644 --- a/rpm/pg-stat-monitor.spec +++ b/rpm/pg-stat-monitor.spec @@ -33,7 +33,7 @@ It provides all the features of pg_stat_statment plus its own feature set. %build -sed -i 's:PG_CONFIG = pg_config:PG_CONFIG = /usr/pgsql-%{pgrel}/bin/pg_config:' Makefile +sed -i 's:PG_CONFIG ?= pg_config:PG_CONFIG = /usr/pgsql-%{pgrel}/bin/pg_config:' Makefile %{__make} USE_PGXS=1 %{?_smp_mflags}