Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Hi,

On 2023-02-26 13:20:00 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Pushed the first (and biggest) commit. More tomorrow.
> 
> I hadn't run my buildfarm-compile-warning scraper for a little while,
> but I just did, and I find that this commit is causing warnings on
> no fewer than 14 buildfarm animals.  They all look like
> 
>  ayu           | 2023-02-25 23:02:08 | pgstat_io.c:40:14: warning: comparison of constant 2 with expression of type
'IOObject'(aka 'enum IOObject') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstat_io.c:43:16: warning: comparison of constant 4 with expression of type
'IOContext'(aka 'enum IOContext') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstat_io.c:70:19: warning: comparison of constant 2 with expression of type
'IOObject'(aka 'enum IOObject') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstat_io.c:71:20: warning: comparison of constant 4 with expression of type
'IOContext'(aka 'enum IOContext') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstat_io.c:115:14: warning: comparison of constant 2 with expression of type
'IOObject'(aka 'enum IOObject') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstat_io.c:118:16: warning: comparison of constant 4 with expression of type
'IOContext'(aka 'enum IOContext') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstatfuncs.c:1329:12: warning: comparison of constant 2 with expression of
type'IOObject' (aka 'enum IOObject') is always true [-Wtautological-constant-out-of-range-compare]
 
>  ayu           | 2023-02-25 23:02:08 | pgstatfuncs.c:1334:17: warning: comparison of constant 4 with expression of
type'IOContext' (aka 'enum IOContext') is always true [-Wtautological-constant-out-of-range-compare]
 

What other animals? If it had been just ayu / clang 4, I'd not be sure it's
worth doing much here.


> That is, these compilers think that comparisons like
> 
>     io_object < IOOBJECT_NUM_TYPES
>     io_context < IOCONTEXT_NUM_TYPES
> 
> are constant-true.  This seems not good; if they were to actually
> act on this observation, by removing those loop-ending tests,
> we'd have a problem.

It'd at least be obvious breakage :/


> The issue seems to be that code like this:
> 
> typedef enum IOContext
> {
>     IOCONTEXT_BULKREAD,
>     IOCONTEXT_BULKWRITE,
>     IOCONTEXT_NORMAL,
>     IOCONTEXT_VACUUM,
> } IOContext;
> 
> #define IOCONTEXT_FIRST IOCONTEXT_BULKREAD
> #define IOCONTEXT_NUM_TYPES (IOCONTEXT_VACUUM + 1)
> 
> is far too cute for its own good.  I'm not sure about how to fix it
> either.  I thought of defining
> 
> #define IOCONTEXT_LAST IOCONTEXT_VACUUM
> 
> and make the loop conditions like "io_context <= IOCONTEXT_LAST",
> but that doesn't actually fix the problem.
> 
> (Even aside from that, I do not find this coding even a little bit
> mistake-proof: you still have to remember to update the #define
> when adding another enum value.)

But the alternative is going around and updating N places, or having a LAST
member in the enum, which then precludes means either adding pointless case
statements or adding default: cases, which prevents the compiler from warning
when a new case is added.

I haven't dug up an old enough compiler yet, what happens if
IOCONTEXT_NUM_TYPES is redefined to ((int)IOOBJECT_TEMP_RELATION + 1)?


> We have similar code involving enum ForkNumber but it looks to me
> like the loop variables are always declared as plain "int".  That
> might be the path of least resistance here.

IIRC that caused some even longer lines due to casting the integer to the enum
in some other lines. Perhaps we should just case for the < comparison?

Greetings,

Andres Freund