From 8b50e95dc85974ff2f34fd94cc01b90530cc1ea3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Halil=20Ozan=20Akg=C3=BCl?= <hozanakgul@gmail.com>
Date: Tue, 11 Apr 2023 17:40:07 +0300
Subject: [PATCH] Fix citus_stat_tenants period updating bug (#6833)

Fixes the bug that causes updating the citus_stat_tenants periods
incorrectly.

`TimestampDifferenceExceeds` expects the difference in milliseconds but
it was microseconds, this is fixed.
`tenantStats->lastQueryTime` was updated during monitoring too, now it's
updated only when there are tenant queries.
---
 .../distributed/utils/citus_stat_tenants.c        | 15 ++++++++-------
 src/test/regress/expected/citus_stat_tenants.out  | 13 +++++++++++++
 src/test/regress/sql/citus_stat_tenants.sql       |  4 ++++
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/src/backend/distributed/utils/citus_stat_tenants.c b/src/backend/distributed/utils/citus_stat_tenants.c
index 88e16e54d..4bf66cb73 100644
--- a/src/backend/distributed/utils/citus_stat_tenants.c
+++ b/src/backend/distributed/utils/citus_stat_tenants.c
@@ -56,7 +56,7 @@ static int CompareTenantScore(const void *leftElement, const void *rightElement)
 static void UpdatePeriodsIfNecessary(TenantStats *tenantStats, TimestampTz queryTime);
 static void ReduceScoreIfNecessary(TenantStats *tenantStats, TimestampTz queryTime);
 static void EvictTenantsIfNecessary(TimestampTz queryTime);
-static void RecordTenantStats(TenantStats *tenantStats);
+static void RecordTenantStats(TenantStats *tenantStats, TimestampTz queryTime);
 static void CreateMultiTenantMonitor(void);
 static MultiTenantMonitor * CreateSharedMemoryForMultiTenantMonitor(void);
 static MultiTenantMonitor * GetMultiTenantMonitor(void);
@@ -345,7 +345,7 @@ AttributeMetricsIfApplicable()
 
 		UpdatePeriodsIfNecessary(tenantStats, queryTime);
 		ReduceScoreIfNecessary(tenantStats, queryTime);
-		RecordTenantStats(tenantStats);
+		RecordTenantStats(tenantStats, queryTime);
 
 		LWLockRelease(&tenantStats->lock);
 	}
@@ -372,7 +372,7 @@ AttributeMetricsIfApplicable()
 
 			UpdatePeriodsIfNecessary(tenantStats, queryTime);
 			ReduceScoreIfNecessary(tenantStats, queryTime);
-			RecordTenantStats(tenantStats);
+			RecordTenantStats(tenantStats, queryTime);
 
 			LWLockRelease(&tenantStats->lock);
 		}
@@ -396,6 +396,7 @@ static void
 UpdatePeriodsIfNecessary(TenantStats *tenantStats, TimestampTz queryTime)
 {
 	long long int periodInMicroSeconds = StatTenantsPeriod * USECS_PER_SEC;
+	long long int periodInMilliSeconds = StatTenantsPeriod * 1000;
 	TimestampTz periodStart = queryTime - (queryTime % periodInMicroSeconds);
 
 	/*
@@ -416,14 +417,12 @@ UpdatePeriodsIfNecessary(TenantStats *tenantStats, TimestampTz queryTime)
 	 * If the last query is more than two periods ago, we clean the last period counts too.
 	 */
 	if (TimestampDifferenceExceeds(tenantStats->lastQueryTime, periodStart,
-								   periodInMicroSeconds))
+								   periodInMilliSeconds))
 	{
 		tenantStats->writesInLastPeriod = 0;
 
 		tenantStats->readsInLastPeriod = 0;
 	}
-
-	tenantStats->lastQueryTime = queryTime;
 }
 
 
@@ -503,7 +502,7 @@ EvictTenantsIfNecessary(TimestampTz queryTime)
  * RecordTenantStats records the query statistics for the tenant.
  */
 static void
-RecordTenantStats(TenantStats *tenantStats)
+RecordTenantStats(TenantStats *tenantStats, TimestampTz queryTime)
 {
 	if (tenantStats->score < LLONG_MAX - ONE_QUERY_SCORE)
 	{
@@ -524,6 +523,8 @@ RecordTenantStats(TenantStats *tenantStats)
 	{
 		tenantStats->writesInThisPeriod++;
 	}
+
+	tenantStats->lastQueryTime = queryTime;
 }
 
 
diff --git a/src/test/regress/expected/citus_stat_tenants.out b/src/test/regress/expected/citus_stat_tenants.out
index debe8b4c8..44376dc3d 100644
--- a/src/test/regress/expected/citus_stat_tenants.out
+++ b/src/test/regress/expected/citus_stat_tenants.out
@@ -263,6 +263,19 @@ SELECT tenant_attribute, read_count_in_this_period, read_count_in_last_period, q
  5                |                         0 |                         0 |                          0 |                          1
 (2 rows)
 
+SELECT sleep_until_next_period();
+ sleep_until_next_period
+---------------------------------------------------------------------
+
+(1 row)
+
+SELECT tenant_attribute, read_count_in_this_period, read_count_in_last_period, query_count_in_this_period, query_count_in_last_period FROM citus_stat_tenants_local ORDER BY tenant_attribute;
+ tenant_attribute | read_count_in_this_period | read_count_in_last_period | query_count_in_this_period | query_count_in_last_period
+---------------------------------------------------------------------
+ 1                |                         0 |                         0 |                          0 |                          0
+ 5                |                         0 |                         0 |                          0 |                          0
+(2 rows)
+
 \c - - - :master_port
 SET search_path TO citus_stat_tenants;
 -- test logs
diff --git a/src/test/regress/sql/citus_stat_tenants.sql b/src/test/regress/sql/citus_stat_tenants.sql
index 61de57511..eb1e0af42 100644
--- a/src/test/regress/sql/citus_stat_tenants.sql
+++ b/src/test/regress/sql/citus_stat_tenants.sql
@@ -92,6 +92,10 @@ SELECT sleep_until_next_period();
 
 SELECT tenant_attribute, read_count_in_this_period, read_count_in_last_period, query_count_in_this_period, query_count_in_last_period FROM citus_stat_tenants_local ORDER BY tenant_attribute;
 
+SELECT sleep_until_next_period();
+
+SELECT tenant_attribute, read_count_in_this_period, read_count_in_last_period, query_count_in_this_period, query_count_in_last_period FROM citus_stat_tenants_local ORDER BY tenant_attribute;
+
 \c - - - :master_port
 SET search_path TO citus_stat_tenants;