From a847ed95de9bc3550ff6783d9aec2c81e064f6ff Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Fri, 27 Aug 2021 15:53:11 -0400 Subject: [PATCH 1/2] PG-223: Remove relations and num_relations from pgssSharedState. These variables can't be in shared state, as the following problem was taking place: 1. Process1 call pgss_ExecutorCheckPerms(), acquire lock, update relations and num_relations, release lock. 2. Process 2 call pgss_ExecutorCheckPerms(), acquire lock, update num_relations = 0; 3. Process 1 read num_relations = 0 in pgss_update_entry, this value is wrong as it was updated by Process 2. Even if we acquire the lock in pgss_update_entry to read num_relations and relations variable, Process 1 may end up acquiring the lock after Process 2 has ovewritten the variable values, leading to Process 1 reading of wrong data. By defining relations and num_relations to be static and global in pg_stat_monitor.c we take advantage that each individual PostgreSQL backend will have its own copy of this data, which allows us to remove the locking in pgss_ExecutorCheckPerms to update these variables, improving pg_stat_monitor overall performance. --- pg_stat_monitor.c | 22 +++++++++------------- pg_stat_monitor.h | 2 -- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index 9a4cea9..2876e1b 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -66,7 +66,8 @@ static int plan_nested_level = 0; /* The array to store outer layer query id*/ uint64 *nested_queryids; -FILE *qfile; +static char relations[REL_LST][REL_LEN]; +static int num_relations; /* Number of relation in the query */ static bool system_init = false; static struct rusage rusage_start; static struct rusage rusage_end; @@ -645,7 +646,6 @@ static void pgss_ExecutorEnd(QueryDesc *queryDesc) { uint64 queryId = queryDesc->plannedstmt->queryId; - pgssSharedState *pgss = pgsm_get_ss(); SysInfo sys_info; PlanInfo plan_info; @@ -693,7 +693,7 @@ pgss_ExecutorEnd(QueryDesc *queryDesc) prev_ExecutorEnd(queryDesc); else standard_ExecutorEnd(queryDesc); - pgss->num_relations = 0; + num_relations = 0; } #ifdef BENCHMARK @@ -713,13 +713,11 @@ static bool pgss_ExecutorCheckPerms(List *rt, bool abort) { ListCell *lr = NULL; - pgssSharedState *pgss = pgsm_get_ss(); int i = 0; int j = 0; Oid list_oid[20]; - LWLockAcquire(pgss->lock, LW_EXCLUSIVE); - pgss->num_relations = 0; + num_relations = 0; foreach(lr, rt) { @@ -744,14 +742,13 @@ pgss_ExecutorCheckPerms(List *rt, bool abort) namespace_name = get_namespace_name(get_rel_namespace(rte->relid)); relation_name = get_rel_name(rte->relid); if (rte->relkind == 'v') - snprintf(pgss->relations[i++], REL_LEN, "%s.%s*", namespace_name, relation_name); + snprintf(relations[i++], REL_LEN, "%s.%s*", namespace_name, relation_name); else - snprintf(pgss->relations[i++], REL_LEN, "%s.%s", namespace_name, relation_name); + snprintf(relations[i++], REL_LEN, "%s.%s", namespace_name, relation_name); } } } - pgss->num_relations = i; - LWLockRelease(pgss->lock); + num_relations = i; if (prev_ExecutorCheckPerms_hook) return prev_ExecutorCheckPerms_hook(rt, abort); @@ -1184,7 +1181,6 @@ pgss_update_entry(pgssEntry *entry, int index; char application_name[APPLICATIONNAME_LEN]; int application_name_len = pg_get_application_name(application_name); - pgssSharedState *pgss = pgsm_get_ss(); double old_mean; int message_len = error_info ? strlen (error_info->message) : 0; int comments_len = comments ? strlen (comments) : 0; @@ -1255,8 +1251,8 @@ pgss_update_entry(pgssEntry *entry, _snprintf(e->counters.planinfo.plan_text, plan_info->plan_text, plan_text_len, PLAN_TEXT_LEN); _snprintf(e->counters.info.application_name, application_name, application_name_len, APPLICATIONNAME_LEN); - e->counters.info.num_relations = pgss->num_relations; - _snprintf2(e->counters.info.relations, pgss->relations, pgss->num_relations, REL_LEN); + e->counters.info.num_relations = num_relations; + _snprintf2(e->counters.info.relations, relations, num_relations, REL_LEN); e->counters.info.cmd_type = cmd_type; diff --git a/pg_stat_monitor.h b/pg_stat_monitor.h index 1f9e6d4..62c33b9 100644 --- a/pg_stat_monitor.h +++ b/pg_stat_monitor.h @@ -310,8 +310,6 @@ typedef struct pgssSharedState uint64 prev_bucket_usec; uint64 bucket_entry[MAX_BUCKETS]; int64 query_buf_size_bucket; - char relations[REL_LST][REL_LEN]; - int num_relations; /* Number of relation in the query */ char bucket_start_time[MAX_BUCKETS][60]; /* start time of the bucket */ } pgssSharedState; From 549347025d47735725f97d00514acb1eb2d0c8b2 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Fri, 27 Aug 2021 16:12:07 -0400 Subject: [PATCH 2/2] PG-223: Use memcpy and strlcpy to copy relations to counters. We redefine macro _snprintf to use memcpy, which performs better, we also update call sites using this macro to add the null terminator '\0' to the source string length, this way memcpy also correctly copies the null terminator to the destination string. We update _snprintf2 macro to use strlcpy, the reason we don't use memcpy here is because in the place where this macro is called, pgss_update_entry, only the maximum string length of REL_LEN=1000 is specified as an upper bound to copy the relations string vector to the destination counters, but since this data is string, we don't need to copy 1k bytes for every entry, by using strlcpy the copy ends as soon as the null terminator '\0' is found in the source string. --- pg_stat_monitor.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index 2876e1b..4766de8 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -31,24 +31,14 @@ PG_MODULE_MAGIC; #define PGUNSIXBIT(val) (((val) & 0x3F) + '0') #define _snprintf(_str_dst, _str_src, _len, _max_len)\ -do \ -{ \ - int i; \ - for(i = 0; i < _len && i < _max_len; i++) \ - {\ - _str_dst[i] = _str_src[i]; \ - }\ -}while(0) + memcpy((void *)_str_dst, _str_src, _len < _max_len ? _len : _max_len) #define _snprintf2(_str_dst, _str_src, _len1, _len2)\ do \ { \ - int i,j; \ + int i; \ for(i = 0; i < _len1; i++) \ - for(j = 0; j < _len2; j++) \ - { \ - _str_dst[i][j] = _str_src[i][j]; \ - } \ + strlcpy((char *)_str_dst[i], _str_src[i], _len2); \ }while(0) /*---- Initicalization Function Declarations ----*/ @@ -1196,7 +1186,8 @@ pgss_update_entry(pgssEntry *entry, if (reset) memset(&entry->counters, 0, sizeof(Counters)); - _snprintf(e->counters.info.comments, comments, comments_len, COMMENTS_LEN); + if (comments_len > 0) + _snprintf(e->counters.info.comments, comments, comments_len + 1, COMMENTS_LEN); e->counters.state = kind; if (kind == PGSS_PLAN) { @@ -1248,8 +1239,11 @@ pgss_update_entry(pgssEntry *entry, e->counters.resp_calls[index]++; } - _snprintf(e->counters.planinfo.plan_text, plan_info->plan_text, plan_text_len, PLAN_TEXT_LEN); - _snprintf(e->counters.info.application_name, application_name, application_name_len, APPLICATIONNAME_LEN); + if (plan_text_len > 0) + _snprintf(e->counters.planinfo.plan_text, plan_info->plan_text, plan_text_len + 1, PLAN_TEXT_LEN); + + if (application_name_len > 0) + _snprintf(e->counters.info.application_name, application_name, application_name_len + 1, APPLICATIONNAME_LEN); e->counters.info.num_relations = num_relations; _snprintf2(e->counters.info.relations, relations, num_relations, REL_LEN);