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

On Tue, Nov 29, 2022 at 5:13 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> Thanks for the review, Maciek!
>
> I've attached a new version 39 of the patch which addresses your docs
> feedback from this email as well as docs feedback from Andres in [1] and
> Justin in [2].

This looks great! Just a couple of minor comments.

> You are right: reused is a normal, expected part of strategy
> execution. And you are correct: the idea behind reusing existing
> strategy buffers instead of taking buffers off the freelist is to leave
> those buffers for blocks that we might expect to be accessed more than
> once.
>
> In practice, however, if you happen to not be using many shared buffers,
> and then do a large COPY, for example, you will end up doing a bunch of
> writes (in order to reuse the strategy buffers) that you perhaps didn't
> need to do at that time had you leveraged the freelist. I think the
> decision about which tradeoff to make is quite contentious, though.

Thanks for the explanation--that makes sense.

> On Mon, Nov 7, 2022 at 1:26 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:
> > Alternately, what do you think about pulling equivalencies to existing
> > views out of the main column descriptions, and adding them after the
> > main table as a sort of footnote? Most view docs don't have anything
> > like that, but pg_stat_replication does and it might be a good pattern
> > to follow.
> >
> > Thoughts?
>
> Thanks for including a patch!
> In the attached v39, I've taken your suggestion of flattening some of
> the lists and done some rewording as well. I have also moved the note
> about equivalence with pg_stat_statements columns to the
> pg_stat_statements documentation. The result is quite a bit different
> than what I had before, so I would be interested to hear your thoughts.
>
> My concern with the blue "note" section like you mentioned is that it
> would be harder to read the lists of backend types than it was in the
> tabular format.

Oh, I wasn't thinking of doing a separate "note": just additional
paragraphs of text after the table (like what pg_stat_replication has
before its "note", or the brief comment after the pg_stat_archiver
table). But I think the updated docs work also.

+      <para>
+       The context or location of an IO operation.
+      </para>

maybe "...of an IO operation:" (colon) instead?

+       default. Future values could include those derived from
+       <symbol>XLOG_BLCKSZ</symbol>, once WAL IO is tracked in this view, and
+       constant multipliers once non-block-oriented IO (e.g. temporary file IO)
+       is tracked here.

I know Lukas had commented that we should communicate that the goal is
to eventually provide relatively comprehensive I/O stats in this view
(you do that in the view description and I think that works), and this
is sort of along those lines, but I think speculative documentation
like this is not all that helpful. I'd drop this last sentence. Just
my two cents.

+      <para>
+       <varname>evicted</varname> in <varname>io_context</varname>
+       <literal>buffer pool</literal> and <varname>io_object</varname>
+       <literal>temp relation</literal> counts the number of times a block of
+       data from an existing local buffer was evicted in order to replace it
+       with another block, also in local buffers.
+      </para>

Doesn't this follow from the first sentence of the column description?
I think we could drop this, no?

Otherwise, the docs look good to me.

Thanks,
Maciek