Remove dependency on VacuumPage(Hit/Miss/Dirty) counters in do_analyze_rel

Started by Dilip Kumarover 1 year ago3 messages
#1Dilip Kumar
dilipbalaut@gmail.com
1 attachment(s)

As part of commit 5cd72cc0c5017a9d4de8b5d465a75946da5abd1d, the
dependency on global counters such as VacuumPage(Hit/Miss/Dirty) was
removed from the vacuum. However, do_analyze_rel() was still using
these counters, necessitating the tracking of global counters
alongside BufferUsage counters.

The attached patch addresses the issue by eliminating the need to
track VacuumPage(Hit/Miss/Dirty) counters in do_analyze_rel(), making
the global counters obsolete. This simplifies the code and improves
consistency.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Attachments:

v1-0001-Remove-duplicate-tracking-of-the-page-stats-durin.patchapplication/octet-stream; name=v1-0001-Remove-duplicate-tracking-of-the-page-stats-durin.patchDownload
From f88f0a85c9e073b719c4b4f0ebbc496184e65950 Mon Sep 17 00:00:00 2001
From: Dilip Kumar <dilip.kumar@enterprisedb.com>
Date: Thu, 6 Jun 2024 14:29:11 +0530
Subject: [PATCH v1] Remove duplicate tracking of the page stats during analyze

As part of commit 5cd72cc0c5017a9d4de8b5d465a75946da5abd1d, the dependency
on global counters such as VacuumPage(Hit/Miss/Dirty) was removed from the vacuum.
However, do_analyze_rel() was still using these counters, necessitating the
tracking of global counters alongside BufferUsage counters.

This commit addresses the issue by eliminating the need to track
VacuumPage(Hit/Miss/Dirty) counters in do_analyze_rel(), making the global
counters obsolete. This simplifies the code and improves consistency.

Dilip Kumar
---
 src/backend/commands/analyze.c        | 26 ++++++++++++--------------
 src/backend/commands/vacuum.c         |  3 ---
 src/backend/commands/vacuumparallel.c |  3 ---
 src/backend/storage/buffer/bufmgr.c   |  4 ----
 src/backend/utils/init/globals.c      |  4 ----
 src/include/miscadmin.h               |  4 ----
 6 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 7d2cd24997..8e6614eb32 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -303,9 +303,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	Oid			save_userid;
 	int			save_sec_context;
 	int			save_nestlevel;
-	int64		AnalyzePageHit = VacuumPageHit;
-	int64		AnalyzePageMiss = VacuumPageMiss;
-	int64		AnalyzePageDirty = VacuumPageDirty;
+	BufferUsage startbufferusage = pgBufferUsage;
 	PgStat_Counter startreadtime = 0;
 	PgStat_Counter startwritetime = 0;
 
@@ -730,6 +728,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 			long		delay_in_ms;
 			double		read_rate = 0;
 			double		write_rate = 0;
+			BufferUsage bufferusage;
 			StringInfoData buf;
 
 			/*
@@ -737,9 +736,8 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 			 * happened as part of the analyze by subtracting out the
 			 * pre-analyze values which we saved above.
 			 */
-			AnalyzePageHit = VacuumPageHit - AnalyzePageHit;
-			AnalyzePageMiss = VacuumPageMiss - AnalyzePageMiss;
-			AnalyzePageDirty = VacuumPageDirty - AnalyzePageDirty;
+			memset(&bufferusage, 0, sizeof(BufferUsage));
+			BufferUsageAccumDiff(&bufferusage, &pgBufferUsage, &startbufferusage);
 
 			/*
 			 * We do not expect an analyze to take > 25 days and it simplifies
@@ -765,10 +763,10 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 
 			if (delay_in_ms > 0)
 			{
-				read_rate = (double) BLCKSZ * AnalyzePageMiss / (1024 * 1024) /
-					(delay_in_ms / 1000.0);
-				write_rate = (double) BLCKSZ * AnalyzePageDirty / (1024 * 1024) /
-					(delay_in_ms / 1000.0);
+				read_rate = (double) BLCKSZ * (bufferusage.shared_blks_read + bufferusage.local_blks_read) /
+					(1024 * 1024) / (delay_in_ms / 1000.0);
+				write_rate = (double) BLCKSZ * (bufferusage.shared_blks_dirtied + bufferusage.local_blks_dirtied) /
+					(1024 * 1024) / (delay_in_ms / 1000.0);
 			}
 
 			/*
@@ -791,10 +789,10 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 			}
 			appendStringInfo(&buf, _("avg read rate: %.3f MB/s, avg write rate: %.3f MB/s\n"),
 							 read_rate, write_rate);
-			appendStringInfo(&buf, _("buffer usage: %lld hits, %lld misses, %lld dirtied\n"),
-							 (long long) AnalyzePageHit,
-							 (long long) AnalyzePageMiss,
-							 (long long) AnalyzePageDirty);
+			appendStringInfo(&buf, _("buffer usage_new: %lld hits, %lld misses, %lld dirtied\n"),
+							 (long long) (bufferusage.shared_blks_hit + bufferusage.local_blks_hit),
+							 (long long) (bufferusage.shared_blks_read + bufferusage.local_blks_read),
+							 (long long) (bufferusage.shared_blks_dirtied + bufferusage.local_blks_dirtied));
 			appendStringInfo(&buf, _("system usage: %s"), pg_rusage_show(&ru0));
 
 			ereport(LOG,
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..7d8e9d2045 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -603,9 +603,6 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 		VacuumFailsafeActive = false;
 		VacuumUpdateCosts();
 		VacuumCostBalance = 0;
-		VacuumPageHit = 0;
-		VacuumPageMiss = 0;
-		VacuumPageDirty = 0;
 		VacuumCostBalanceLocal = 0;
 		VacuumSharedCostBalance = NULL;
 		VacuumActiveNWorkers = NULL;
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index f26070bff2..22c057fe61 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -1043,9 +1043,6 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
 	/* Set cost-based vacuum delay */
 	VacuumUpdateCosts();
 	VacuumCostBalance = 0;
-	VacuumPageHit = 0;
-	VacuumPageMiss = 0;
-	VacuumPageDirty = 0;
 	VacuumCostBalanceLocal = 0;
 	VacuumSharedCostBalance = &(shared->cost_balance);
 	VacuumActiveNWorkers = &(shared->active_nworkers);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 49637284f9..996ee6fdc8 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1127,7 +1127,6 @@ PinBufferForBlock(Relation rel,
 	}
 	if (*foundPtr)
 	{
-		VacuumPageHit++;
 		pgstat_count_io_op(io_object, io_context, IOOP_HIT);
 		if (VacuumCostActive)
 			VacuumCostBalance += VacuumCostPageHit;
@@ -1519,7 +1518,6 @@ WaitReadBuffers(ReadBuffersOperation *operation)
 											  false);
 		}
 
-		VacuumPageMiss += io_buffers_len;
 		if (VacuumCostActive)
 			VacuumCostBalance += VacuumCostPageMiss * io_buffers_len;
 	}
@@ -2513,7 +2511,6 @@ MarkBufferDirty(Buffer buffer)
 	 */
 	if (!(old_buf_state & BM_DIRTY))
 	{
-		VacuumPageDirty++;
 		pgBufferUsage.shared_blks_dirtied++;
 		if (VacuumCostActive)
 			VacuumCostBalance += VacuumCostPageDirty;
@@ -5036,7 +5033,6 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
 
 		if (dirtied)
 		{
-			VacuumPageDirty++;
 			pgBufferUsage.shared_blks_dirtied++;
 			if (VacuumCostActive)
 				VacuumCostBalance += VacuumCostPageDirty;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index cc61937eef..ba7f45e8a2 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -151,10 +151,6 @@ int			VacuumCostPageDirty = 20;
 int			VacuumCostLimit = 200;
 double		VacuumCostDelay = 0;
 
-int64		VacuumPageHit = 0;
-int64		VacuumPageMiss = 0;
-int64		VacuumPageDirty = 0;
-
 int			VacuumCostBalance = 0;	/* working state for vacuum */
 bool		VacuumCostActive = false;
 
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 90f9b21b25..6d07005d39 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -283,10 +283,6 @@ extern PGDLLIMPORT int VacuumCostPageDirty;
 extern PGDLLIMPORT int VacuumCostLimit;
 extern PGDLLIMPORT double VacuumCostDelay;
 
-extern PGDLLIMPORT int64 VacuumPageHit;
-extern PGDLLIMPORT int64 VacuumPageMiss;
-extern PGDLLIMPORT int64 VacuumPageDirty;
-
 extern PGDLLIMPORT int VacuumCostBalance;
 extern PGDLLIMPORT bool VacuumCostActive;
 
-- 
2.39.2 (Apple Git-143)

#2Anthonin Bonnefoy
anthonin.bonnefoy@datadoghq.com
In reply to: Dilip Kumar (#1)
Re: Remove dependency on VacuumPage(Hit/Miss/Dirty) counters in do_analyze_rel

Hi,

I sent a similar patch for this in
/messages/by-id/CAO6_Xqr__kTTCLkftqS0qSCm-J7_xbRG3Ge2rWhucxQJMJhcRA@mail.gmail.com

Regards,
Anthonin

On Thu, Jun 6, 2024 at 11:10 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Show quoted text

As part of commit 5cd72cc0c5017a9d4de8b5d465a75946da5abd1d, the
dependency on global counters such as VacuumPage(Hit/Miss/Dirty) was
removed from the vacuum. However, do_analyze_rel() was still using
these counters, necessitating the tracking of global counters
alongside BufferUsage counters.

The attached patch addresses the issue by eliminating the need to
track VacuumPage(Hit/Miss/Dirty) counters in do_analyze_rel(), making
the global counters obsolete. This simplifies the code and improves
consistency.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

#3Dilip Kumar
dilipbalaut@gmail.com
In reply to: Anthonin Bonnefoy (#2)
Re: Remove dependency on VacuumPage(Hit/Miss/Dirty) counters in do_analyze_rel

On Thu, Jun 6, 2024 at 3:23 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:

Hi,

I sent a similar patch for this in /messages/by-id/CAO6_Xqr__kTTCLkftqS0qSCm-J7_xbRG3Ge2rWhucxQJMJhcRA@mail.gmail.com

Okay, I see, In that case, we can just discard mine, thanks for notifying me.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com