From 1881fd737b5a4b788b7872b6a491b163b796eb9a Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Wed, 22 Dec 2021 12:46:58 -0300 Subject: [PATCH] PG-291: Fix query call count. The issue with wrong query call count was taking place during transition to a new bucket, the process is shortly describe bellow: 1. Scan for pending queries in previous bucket. 2. Add pending queries to the new bucket id. 3. Remove pending queries from previous bucket id. The problem is that when switching to a new bucket, we reset query statistics for a given entry being processed, so, for example, if the pending query had a call count of 10 (9 of which were finished, 10th is the pending one), if we move this query to the new bucket, the entry will have its stats reseted, clearing the query call count to zero. To solve the problem, whenever a pending query is detected, if the entry has a call count > 1, we mark it as finished, and don't remove it from the previous bucket in order to keep its statistics, then we move just the pending query (10th in the example) to the new bucket id. Another issue is that when moving a entry to a new bucket, we missed copying the query position from the previous entry, which is used to locate the query text in the query buffer: hash_entry_dealloc():291 new_entry->query_pos = old_entry->query_pos; --- hash_query.c | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/hash_query.c b/hash_query.c index 4006544..809c829 100644 --- a/hash_query.c +++ b/hash_query.c @@ -216,9 +216,21 @@ hash_entry_dealloc(int new_bucket_id, int old_bucket_id, unsigned char *query_bu pgssEntry *bkp_entry = malloc(sizeof(pgssEntry)); if (!bkp_entry) { - /* No memory, remove pending query entry from the previous bucket. */ - elog(ERROR, "hash_entry_dealloc: out of memory"); - entry = hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL); + pgsm_log_error("hash_entry_dealloc: out of memory"); + /* + * No memory, If the entry has calls > 1 then we change the state to finished, + * as the pending query will likely finish execution during the new bucket + * time window. The pending query will vanish in this case, can't list it + * until it completes. + * + * If there is only one call to the query and it's pending, remove the + * entry from the previous bucket and allow it to finish in the new bucket, + * in order to avoid the query living in the old bucket forever. + */ + if (entry->counters.calls.calls > 1) + entry->counters.state = PGSS_FINISHED; + else + entry = hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL); continue; } @@ -231,8 +243,20 @@ hash_entry_dealloc(int new_bucket_id, int old_bucket_id, unsigned char *query_bu /* Add the entry to a list of nodes to be processed later. */ pending_entries = lappend(pending_entries, bkp_entry); - /* Finally remove the pending query from the expired bucket id. */ - entry = hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL); + /* + * If the entry has calls > 1 then we change the state to finished in + * the previous bucket, as the pending query will likely finish execution + * during the new bucket time window. Can't remove it from the previous bucket + * as it may have many calls and we would lose the query statistics. + * + * If there is only one call to the query and it's pending, remove the entry + * from the previous bucket and allow it to finish in the new bucket, + * in order to avoid the query living in the old bucket forever. + */ + if (entry->counters.calls.calls > 1) + entry->counters.state = PGSS_FINISHED; + else + entry = hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL); } } } @@ -255,6 +279,7 @@ hash_entry_dealloc(int new_bucket_id, int old_bucket_id, unsigned char *query_bu new_entry->counters = old_entry->counters; SpinLockInit(&new_entry->mutex); new_entry->encoding = old_entry->encoding; + new_entry->query_pos = old_entry->query_pos; } free(old_entry);