On Mon, Feb 27, 2023 at 10:30 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Melanie Plageman <melanieplage...@gmail.com> writes: > > 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++) > > I don't really like that proposal. ISTM it's just silencing the > messenger rather than addressing the underlying problem, namely that > there's no guarantee that an IOObject variable can hold the value > IOOBJECT_NUM_TYPES, which it had better do if you want the loop to > terminate. Admittedly it's quite unlikely that these three enums would > grow to the point that that becomes an actual hazard for them --- but > IMO it's still bad practice and a bad precedent for future code.
That's fair. Patch attached. > > 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(). > > Yeah, my druthers would be to just do it the way we do comparable > things with ForkNumber. I don't feel like we need to invent a > better way here. > > The risk of needing to cast when using the "int" loop variable > as an enum is obviously the downside of that approach, but we have > not seen any indication that any compilers actually do warn. > It's interesting that you did see such a warning ... I wonder which > compiler you were using at the time? so, pretty much any version of clang I tried with -Wsign-conversion produces a warning. <source>:35:32: warning: implicit conversion changes signedness: 'int' to 'IOOp' (aka 'enum IOOp') [-Wsign-conversion] I didn't do the casts in the attached patch since they aren't done elsewhere. - Melanie
From 34fee4a3d1d1353aa38a95b3afc2d302a5f586ff Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Mon, 27 Feb 2023 08:48:11 -0500 Subject: [PATCH v1 2/2] Change IO stats enum loop variables to ints Per [1], using an enum as the loop variable with a macro-defined termination condition of #_enum_values + 1 is not guaranteed to be safe - as compilers are free to make enums as small as a char. Some (older) compilers will notice this when building with -Wtautological-constant-out-of-range-compare. [1] https://www.postgresql.org/message-id/354645.1677511842%40sss.pgh.pa.us --- src/backend/utils/activity/pgstat_io.c | 12 ++++++------ src/backend/utils/adt/pgstatfuncs.c | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c index c478b126fa..c4199d18c8 100644 --- a/src/backend/utils/activity/pgstat_io.c +++ b/src/backend/utils/activity/pgstat_io.c @@ -36,16 +36,16 @@ pgstat_bktype_io_stats_valid(PgStat_BktypeIO *backend_io, { bool bktype_tracked = pgstat_tracks_io_bktype(bktype); - for (IOObject io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++) + for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++) { - for (IOContext io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + for (int 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 = 0; io_op < IOOP_NUM_TYPES; io_op++) + for (int 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) @@ -109,11 +109,11 @@ pgstat_flush_io(bool nowait) else if (!LWLockConditionalAcquire(bktype_lock, LW_EXCLUSIVE)) return true; - for (IOObject io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++) + for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++) { - for (IOContext io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) { - for (IOOp io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) + for (int 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 12eda4ade0..b61a12382b 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 = 0; bktype < BACKEND_NUM_TYPES; bktype++) + for (int bktype = 0; bktype < BACKEND_NUM_TYPES; bktype++) { Datum bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype)); PgStat_BktypeIO *bktype_stats = &backends_io_stats->stats[bktype]; @@ -1325,11 +1325,11 @@ pg_stat_get_io(PG_FUNCTION_ARGS) if (!pgstat_tracks_io_bktype(bktype)) continue; - for (IOObject io_obj = 0; io_obj < IOOBJECT_NUM_TYPES; io_obj++) + for (int 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 = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) + for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++) { const char *context_name = pgstat_get_io_context_name(io_context); @@ -1357,7 +1357,7 @@ pg_stat_get_io(PG_FUNCTION_ARGS) */ values[IO_COL_CONVERSION] = Int64GetDatum(BLCKSZ); - for (IOOp io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) + for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++) { int col_idx = pgstat_get_io_op_index(io_op); -- 2.37.2
