On Sun, Feb 26, 2023 at 1:52 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> I wrote:
> > The issue seems to be that code like this:
> > ...
> > is far too cute for its own good.
>
> Oh, there's another thing here that qualifies as too-cute: loops like
>
>     for (IOObject io_object = IOOBJECT_FIRST;
>          io_object < IOOBJECT_NUM_TYPES; io_object++)
>
> make it look like we could define these enums as 1-based rather
> than 0-based, but if we did this code would fail, because it's
> confusing "the number of values" with "1 more than the last value".
>
> Again, we could fix that with tests like "io_context <= IOCONTEXT_LAST",
> but I don't see the point of adding more macros rather than removing
> some.  We do need IOOBJECT_NUM_TYPES to declare array sizes with,
> so I think we should nuke the "xxx_FIRST" macros as being not worth
> the electrons they're written on, and write these loops like
>
>     for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
>
> which is not actually adding any assumptions that you don't already
> make by using io_object as a C array subscript.

Attached is a patch to remove the *_FIRST macros.
I was going to add in code to change

    for (IOObject io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
    to
    for (IOObject io_object = 0; (int) io_object < IOOBJECT_NUM_TYPES;
io_object++)

but then I couldn't remember why we didn't just do

    for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)

I recall that when passing that loop variable into a function I was
getting a compiler warning that required me to cast the value back to an
enum to silence it:

            pgstat_tracks_io_op(bktype, (IOObject) io_object,
io_context, io_op))

However, I am now unable to reproduce that warning.
Moreover, I see in cases like table_block_relation_size() with
ForkNumber, the variable i is passed with no cast to smgrnblocks().

- Melanie
From cce6dc75e9e4fc9adc018a1d05874be5f3be96ae Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Mon, 27 Feb 2023 08:22:53 -0500
Subject: [PATCH v1 1/2] Remove potentially misleading *_FIRST macros

28e626bde00ef introduced IO statistic enums IOOp, IOObject, and
IOContext along with macros *_FIRST intended for use when looping
through the enumerated values of each. Per discussion in [1] these
macros are confusing and error-prone. Remove them.

[1] https://www.postgresql.org/message-id/23770.1677437567%40sss.pgh.pa.us
---
 src/backend/utils/activity/pgstat_io.c | 17 ++++++-----------
 src/backend/utils/adt/pgstatfuncs.c    | 10 ++++------
 src/include/pgstat.h                   |  3 ---
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 0e07e0848d..c478b126fa 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -36,18 +36,16 @@ pgstat_bktype_io_stats_valid(PgStat_BktypeIO *backend_io,
 {
 	bool		bktype_tracked = pgstat_tracks_io_bktype(bktype);
 
-	for (IOObject io_object = IOOBJECT_FIRST;
-		 io_object < IOOBJECT_NUM_TYPES; io_object++)
+	for (IOObject io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
 	{
-		for (IOContext io_context = IOCONTEXT_FIRST;
-			 io_context < IOCONTEXT_NUM_TYPES; io_context++)
+		for (IOContext io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
 		{
 			/*
 			 * Don't bother trying to skip to the next loop iteration if
 			 * pgstat_tracks_io_object() would return false here. We still
 			 * need to validate that each counter is zero anyway.
 			 */
-			for (IOOp io_op = IOOP_FIRST; io_op < IOOP_NUM_TYPES; io_op++)
+			for (IOOp io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
 			{
 				/* No stats, so nothing to validate */
 				if (backend_io->data[io_object][io_context][io_op] == 0)
@@ -111,14 +109,11 @@ pgstat_flush_io(bool nowait)
 	else if (!LWLockConditionalAcquire(bktype_lock, LW_EXCLUSIVE))
 		return true;
 
-	for (IOObject io_object = IOOBJECT_FIRST;
-		 io_object < IOOBJECT_NUM_TYPES; io_object++)
+	for (IOObject io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
 	{
-		for (IOContext io_context = IOCONTEXT_FIRST;
-			 io_context < IOCONTEXT_NUM_TYPES; io_context++)
+		for (IOContext io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
 		{
-			for (IOOp io_op = IOOP_FIRST;
-				 io_op < IOOP_NUM_TYPES; io_op++)
+			for (IOOp io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
 				bktype_shstats->data[io_object][io_context][io_op] +=
 					PendingIOStats.data[io_object][io_context][io_op];
 		}
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9d707c3521..12eda4ade0 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1306,7 +1306,7 @@ pg_stat_get_io(PG_FUNCTION_ARGS)
 
 	reset_time = TimestampTzGetDatum(backends_io_stats->stat_reset_timestamp);
 
-	for (BackendType bktype = B_INVALID; bktype < BACKEND_NUM_TYPES; bktype++)
+	for (BackendType bktype = 0; bktype < BACKEND_NUM_TYPES; bktype++)
 	{
 		Datum		bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype));
 		PgStat_BktypeIO *bktype_stats = &backends_io_stats->stats[bktype];
@@ -1325,13 +1325,11 @@ pg_stat_get_io(PG_FUNCTION_ARGS)
 		if (!pgstat_tracks_io_bktype(bktype))
 			continue;
 
-		for (IOObject io_obj = IOOBJECT_FIRST;
-			 io_obj < IOOBJECT_NUM_TYPES; io_obj++)
+		for (IOObject io_obj = 0; io_obj < IOOBJECT_NUM_TYPES; io_obj++)
 		{
 			const char *obj_name = pgstat_get_io_object_name(io_obj);
 
-			for (IOContext io_context = IOCONTEXT_FIRST;
-				 io_context < IOCONTEXT_NUM_TYPES; io_context++)
+			for (IOContext io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
 			{
 				const char *context_name = pgstat_get_io_context_name(io_context);
 
@@ -1359,7 +1357,7 @@ pg_stat_get_io(PG_FUNCTION_ARGS)
 				 */
 				values[IO_COL_CONVERSION] = Int64GetDatum(BLCKSZ);
 
-				for (IOOp io_op = IOOP_FIRST; io_op < IOOP_NUM_TYPES; io_op++)
+				for (IOOp io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
 				{
 					int			col_idx = pgstat_get_io_op_index(io_op);
 
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index db9675884f..f43fac09ed 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -287,7 +287,6 @@ typedef enum IOObject
 	IOOBJECT_TEMP_RELATION,
 } IOObject;
 
-#define IOOBJECT_FIRST IOOBJECT_RELATION
 #define IOOBJECT_NUM_TYPES (IOOBJECT_TEMP_RELATION + 1)
 
 typedef enum IOContext
@@ -298,7 +297,6 @@ typedef enum IOContext
 	IOCONTEXT_VACUUM,
 } IOContext;
 
-#define IOCONTEXT_FIRST IOCONTEXT_BULKREAD
 #define IOCONTEXT_NUM_TYPES (IOCONTEXT_VACUUM + 1)
 
 typedef enum IOOp
@@ -311,7 +309,6 @@ typedef enum IOOp
 	IOOP_WRITE,
 } IOOp;
 
-#define IOOP_FIRST IOOP_EVICT
 #define IOOP_NUM_TYPES (IOOP_WRITE + 1)
 
 typedef struct PgStat_BktypeIO
-- 
2.37.2

Reply via email to