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