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

On Wed, Nov 23, 2022 at 12:43 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Note that 001 fails to compile without 002:
>
> ../src/backend/storage/buffer/bufmgr.c:1257:43: error: ‘from_ring’ undeclared (first use in this function)
>  1257 |       StrategyRejectBuffer(strategy, buf, from_ring))

Thanks!
I fixed this in version 38 attached in response to Andres upthread [1].

> My "warnings" script informed me about these gripes from MSVC:
>
> [03:42:30.607] c:\cirrus>call sh -c 'if grep ": warning " build.txt; then exit 1; fi; exit 0'
> [03:42:30.749] c:\cirrus\src\backend\storage\buffer\freelist.c(699) : warning C4715: 'IOContextForStrategy': not all control paths return a value
> [03:42:30.749] c:\cirrus\src\backend\utils\activity\pgstat_io_ops.c(190) : warning C4715: 'pgstat_io_context_desc': not all control paths return a value
> [03:42:30.749] c:\cirrus\src\backend\utils\activity\pgstat_io_ops.c(204) : warning C4715: 'pgstat_io_object_desc': not all control paths return a value
> [03:42:30.749] c:\cirrus\src\backend\utils\activity\pgstat_io_ops.c(226) : warning C4715: 'pgstat_io_op_desc': not all control paths return a value
> [03:42:30.749] c:\cirrus\src\backend\utils\adt\pgstatfuncs.c(1816) : warning C4715: 'pgstat_io_op_get_index': not all control paths return a value

Thanks, I forgot to look at those warnings in CI.
I added pg_unreachable() and think it silenced the warnings.

> In the docs table, you say things like:
> | io_context vacuum refers to the IO operations incurred while vacuuming and analyzing.
>
> ..but it's a bit unclear (maybe due to the way the docs are rendered).
> I think it may be more clear to say "when <io_context> is
> <vacuum>, ..."

So, because I use this language [column name] [column value] so often in
the docs, I would prefer a pattern that is as concise as possible. I
agree it may be hard to see due to the rendering. Currently, I am using
<varname> tags for the column name and <literal> tags for the column
value. Is there another tag type I could use to perhaps make this more
clear without adding additional words?

This is what the code looks like for the above docs text:
<varname>io_context</varname> <literal>vacuum</literal> refers to the IO

> | acquiring the equivalent number of shared buffers
>
> I don't think "equivelent" fits here, since it's actually acquiring a
> different number of buffers.

I'm planning to do docs changes in a separate patchset after addressing
code feedback. I plan to change "equivalent" to "corresponding" here.

> There's a missing period before " The difference is"
>
> The sentence beginning "read plus extended for backend_types" is difficult to
> parse due to having a bulleted list in its middle.

Will address in future version.

> There aren't many references to "IOOps", which is good, because I
> started to read it as "I oops".

Grep'ing for this in the code, I only use the word IOOp(s) in the code
when I very clearly want to use the type name -- and never in the docs.
But, yes, it does look like "I oops" :)

>
> +        * Flush IO Operations statistics now. pgstat_report_stat() will flush IO
> +        * Operation stats, however this will not be called after an entire
>
> => I think that's intended to say *until* after ?

Fixed in v38.

> + * Functions to assert that invalid IO Operation counters are zero.
>
> => There's a missing newline above this comment.

Fixed in v38.

> +       Assert(counters->evictions == 0 && counters->extends == 0 &&
> +                       counters->fsyncs == 0 && counters->reads == 0 && counters->reuses
> +                       == 0 && counters->writes == 0);
>
> => It'd be more readable and also maybe help debugging if these were separate
> assertions.

I have made this change.

> +pgstat_io_op_stats_collected(BackendType bktype)
> +{
> +       return bktype != B_INVALID && bktype != B_ARCHIVER && bktype != B_LOGGER &&
> +               bktype != B_WAL_RECEIVER && bktype != B_WAL_WRITER;
>
> Similar: I'd prefer to see this as 5 "ifs" or a "switch" to return
> false, else return true.  But YMMV.

I don't know that separating it into multiple if statements or a switch
would make it more clear to me or help me with debugging here.

Separately, since this is used in non-assert builds, I would like to
ensure it is efficient. Do you know if a switch or if statements will
be compiled to the exact same thing as this at useful optimization
levels?

>
> +                * CREATE TEMPORRARY TABLE AS ...
>
> => typo: temporary

Fixed in v38.

>
> +       if (strategy_io_context  && io_op == IOOP_FSYNC)
>
> => Extra space.

Fixed.

>
> pgstat_count_io_op() has a superflous newline before "}".

I couldn't find the one you are referencing.
Do you mind pasting in the code?

Thanks,
Melanie

[1] https://www.postgresql.org/message-id/CAAKRu_Zvaj_yFA_eiSRrLZsjhT0J8cJ044QhZfKuXq6WN5bu5g%40mail.gmail.com