On Sun, Feb 26, 2023 at 1:52 PM Tom Lane <[email protected]> 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 <[email protected]>
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
