From 1662e9efa1947f4b1a1966dccc051e9b16ad98b2 Mon Sep 17 00:00:00 2001 From: Hamid Akhtar Date: Fri, 20 Jan 2023 19:21:11 +0500 Subject: [PATCH] PG-562: Histogram Ranges/Buckets are not correct. Replaced the error on server start with a warning. The functionality now handles "pgsm_histogram_buckets" as the maximum number of histogram buckets to be created. On init, pg_stat_monitor calculates the max number of buckets that can be created within the given min/max time range. If the number is below the user configuration, it emits a warning in the log file stating the number of max buckets set. --- pg_stat_monitor.c | 64 ++++++++++++++++++++++++++++++++++++----------- pg_stat_monitor.h | 8 ------ 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index 26e4da6..49dc08a 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -67,6 +67,7 @@ do \ void _PG_init(void); void _PG_fini(void); + /*---- Local variables ----*/ /* Current nesting depth of ExecutorRun+ProcessUtility calls */ @@ -77,6 +78,12 @@ volatile bool __pgsm_do_not_capture_error = false; static int plan_nested_level = 0; #endif +/* Histogram bucket variables */ +static double hist_bucket_min; +static double hist_bucket_max; +static int hist_bucket_count_user; +static int hist_bucket_count_total; + /* The array to store outer layer query id*/ uint64 *nested_queryids; char **nested_query_txts; @@ -84,11 +91,13 @@ char **nested_query_txts; /* Regex object used to extract query comments. */ static regex_t preg_query_comments; 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; + /* Query buffer, store queries' text. */ static char *pgss_explain(QueryDesc *queryDesc); @@ -255,21 +264,46 @@ _PG_init(void) /* Inilize the GUC variables */ init_guc(); - /* Validate histogram values and in case of failure, revert to defaults */ + /* Validate histogram values and find the max number of histogram buckets that can be created */ { - int64 b1_start; int64 b2_start; - int64 b1_end; int64 b2_end; + int b_count = hist_bucket_count_user; + + hist_bucket_min = PGSM_HISTOGRAM_MIN; + hist_bucket_max = PGSM_HISTOGRAM_MAX; + hist_bucket_count_user = PGSM_HISTOGRAM_BUCKETS_USER; if (PGSM_HISTOGRAM_BUCKETS_USER >= 2) { - histogram_bucket_timings(1, &b1_start, &b1_end); - histogram_bucket_timings(2, &b2_start, &b2_end); + for (; hist_bucket_count_user > 0; hist_bucket_count_user--) + { + histogram_bucket_timings(2, &b2_start, &b2_end); - /* Checking if histograms buckets overlap */ - if (b2_start < b1_end || b2_start == b2_end) - elog(ERROR, "pg_stat_monitor: Histogram buckets are overlapping, please fix histogram configuration."); + /* + * The first bucket size will always be one or greater as we're doing min value + e^0; and e^0 = 1. + * Checking if histograms buckets overlap. That can only happen if the second bucket size is zero + * as we using exponential bucket sizes. Therefore, if the second bucket size is greater than 1, we'll + * never have overlapping buckets. + */ + if (b2_start != b2_end) + { + break; + } + } + + /* + * Important that we keep user bucket count separate for calculations, but must add 1 + * for max outlier queries. However, for min, bucket should only be added + * if the minimum value provided by user is greater than 0 + */ + hist_bucket_count_total = (hist_bucket_count_user + 1 + (int)(hist_bucket_min > 0)); + + if (b_count != hist_bucket_count_user) + ereport(WARNING, + errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("pg_stat_monitor: Histogram buckets are overlapping."), + errdetail("Histogram bucket size is set to %d [not including outlier buckets].", hist_bucket_count_user)); } } @@ -2079,7 +2113,7 @@ pg_stat_monitor_internal(FunctionCallInfo fcinfo, } /* resp_calls at column number 41 */ - values[i++] = IntArrayGetTextDatum(tmp.resp_calls, PGSM_HISTOGRAM_BUCKETS); + values[i++] = IntArrayGetTextDatum(tmp.resp_calls, hist_bucket_count_total); /* utime at column number 42 */ values[i++] = Float8GetDatumFast(roundf(tmp.sysinfo.utime, 4)); @@ -3263,10 +3297,10 @@ unpack_sql_state(int sql_state) static void histogram_bucket_timings(int index, int64 *b_start, int64 *b_end) { - double q_min = PGSM_HISTOGRAM_MIN; - double q_max = PGSM_HISTOGRAM_MAX; - int b_count = PGSM_HISTOGRAM_BUCKETS; - int b_count_user = PGSM_HISTOGRAM_BUCKETS_USER; + double q_min = hist_bucket_min; + double q_max = hist_bucket_max; + int b_count = hist_bucket_count_total; + int b_count_user = hist_bucket_count_user; double bucket_size; /* @@ -3306,7 +3340,7 @@ get_histogram_bucket(double q_time) { int64 b_start; int64 b_end; - int b_count = PGSM_HISTOGRAM_BUCKETS; + int b_count = hist_bucket_count_total; int index = 0; for (index = 0; index < b_count; index++) @@ -3333,7 +3367,7 @@ get_histogram_timings(PG_FUNCTION_ARGS) { int64 b_start; int64 b_end; - int b_count = PGSM_HISTOGRAM_BUCKETS; + int b_count = hist_bucket_count_total; int index = 0; char *tmp_str = palloc0(MAX_STRING_LEN); char *text_str = palloc0(MAX_STRING_LEN); diff --git a/pg_stat_monitor.h b/pg_stat_monitor.h index 0336643..ced517c 100644 --- a/pg_stat_monitor.h +++ b/pg_stat_monitor.h @@ -485,15 +485,7 @@ static const struct config_enum_entry track_options[] = #define PGSM_BUCKET_TIME get_conf(5)->guc_variable #define PGSM_HISTOGRAM_MIN get_conf(6)->guc_variable #define PGSM_HISTOGRAM_MAX get_conf(7)->guc_variable - -/* - * Important that we keep user bucket values separate but must add 1 - * for max outlier queries. However, for min, bucket should only be added - * if the minimum value provided by user is greater than 0 - */ #define PGSM_HISTOGRAM_BUCKETS_USER get_conf(8)->guc_variable -#define PGSM_HISTOGRAM_BUCKETS (PGSM_HISTOGRAM_BUCKETS_USER + 1 + (int)(PGSM_HISTOGRAM_MIN > 0)) - #define PGSM_QUERY_SHARED_BUFFER get_conf(9)->guc_variable #define PGSM_OVERFLOW_TARGET get_conf(10)->guc_variable #define PGSM_QUERY_PLAN get_conf(11)->guc_variable