Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?) - Mailing list pgsql-hackers

On Thu, Nov 3, 2022 at 10:00 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> I decided not to call it pg_statio because all of the other stats views
> have an underscore after stat and I thought it was an opportunity to be
> consistent with them.

Oh, got it. Makes sense.

> > I'm reviewing the rendered docs now, and I noticed sentences like this
> > are a bit hard to scan: they force the reader to parse a big list of
> > backend types before even getting to the meat of what this is talking
> > about. Should we maybe reword this so that the backend list comes at
> > the end of the sentence? Or maybe even use a list (e.g., like in the
> > "state" column description in pg_stat_activity)?
>
> Good idea with the bullet points.
> For the lengthy lists, I've added bullet point lists to the docs for
> several of the columns. It is quite long now but, hopefully, clearer?
> Let me know if you think it improves the readability.

Hmm, I should have tried this before suggesting it. I think the lists
break up the flow of the column description too much. What do you
think about the attached (on top of your patches--attaching it as a
.diff to hopefully not confuse cfbot)? I kept the lists for backend
types but inlined the others as a middle ground. I also added a few
omitted periods and reworded "read plus extended" to avoid starting
the sentence with a (lowercase) varname (I think in general it's fine
to do that, but the more complicated sentence structure here makes it
easier to follow if the sentence starts with a capital).

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?

> > Also, is this (in the middle of the table) the right place for this
> > column? I would have expected to see it before or after all the actual
> > I/O op cells.
>
> I put it after read, write, and extend columns because it applies to
> them. It doesn't apply to files_synced. For reused and evicted, I didn't
> think bytes reused and evicted made sense. Also, when we add non-block
> oriented IO, reused and evicted won't be used but op_bytes will be. So I
> thought it made more sense to place it after the operations it applies
> to.

Got it, makes sense.

> > + <varname>io_context</varname>s. When a <quote>Buffer Access
> > + Strategy</quote> reuses a buffer in the strategy ring, it must evict its
> > + contents, incrementing <varname>reused</varname>. When a <quote>Buffer
> > + Access Strategy</quote> adds a new shared buffer to the strategy ring
> > + and this shared buffer is occupied, the <quote>Buffer Access
> > + Strategy</quote> must evict the contents of the shared buffer,
> > + incrementing <varname>evicted</varname>.
> >
> > I think the parallel phrasing here makes this a little hard to follow.
> > Specifically, I think "must evict its contents" for the strategy case
> > sounds like a bad thing, but in fact this is a totally normal thing
> > that happens as part of strategy access, no? The idea is you probably
> > won't need that buffer again, so it's fine to evict it. I'm not sure
> > how to reword, but I think the current phrasing is misleading.
>
> I had trouble rephrasing this. I changed a few words. I see what you
> mean. It is worth noting that reusing strategy buffers when there are
> buffers on the freelist may not be the best behavior, so I wouldn't
> necessarily consider "reused" a good thing. However, I'm not sure how
> much the user could really do about this. I would at least like this
> phrasing to be clear (evicted is for shared buffers, reused is for
> strategy buffers), so, perhaps this section requires more work.

Oh, I see. I think the updated wording works better. Although I think
we can drop the quotes around "Buffer Access Strategy" here. They're
useful when defining the term originally, but after that I think it's
clearer to use the term unquoted.

Just to understand this better myself, though: can you clarify when
"reused" is not a normal, expected part of the strategy execution? I
was under the impression that a ring buffer is used because each page
is needed only "once" (i.e., for one set of operations) for the
command using the strategy ring buffer. Naively, in that situation, it
seems better to reuse a no-longer-needed buffer than to claim another
buffer from the freelist (where other commands may eventually make
better use of it).

> > + again. A high number of repossessions is a sign of contention for the
> > + blocks operated on by the strategy operation.
> >
> > This (and in general the repossession description) makes sense, but
> > I'm not sure what to do with the information. Maybe Andres is right
> > that we could skip this in the first version?
>
> I've removed repossessed and rejected in attached v37. I am a bit sad
> about this because I don't see a good way forward and I think those
> could be useful for users.

I can see that, but I think as long as we're not doing anything to
preclude adding this in the future, it's better to get something out
there and expand it later. For what it's worth, I don't feel it needs
to be excluded, just that it's not worth getting hung up on.

> I have added the new column Andres recommended in [1] ("io_object") to
> clarify temp and local buffers and pave the way for bypass IO (IO not
> done through a buffer pool), which can be done on temp or permanent
> files for temp or permanent relations, and spill file IO which is done
> on temporary files but isn't related to temporary tables.
>
> IOObject has increased the memory footprint and complexity of the code
> around tracking and accumulating the statistics, though it has not
> increased the number of rows in the view.
>
> One question I still have about this additional dimension is how much
> enumeration we need of the various combinations of IO operations, IO
> objects, IO ops, and backend types which are allowed and not allowed.
> Currently because it is only valid to operate on both IOOBJECT_RELATION
> and IOOBJECT_TEMP_RELATION in IOCONTEXT_BUFFER_POOL, the changes to the
> various functions asserting and validating what is "allowed" in terms of
> combinations of ops, objects, contexts, and backend types aren't much
> different than they were without IO Object. However, once we begin
> adding other objects and contexts, we will need to make this logic more
> comprehensive. I'm not sure whether or not I should do that
> preemptively.

It's definitely something to consider, but I have no useful input here.

Some more notes on the docs patch:

+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>io_context</structfield> <type>text</type>
+ </para>
+ <para>
+ The context or location of an IO operation.
+ </para>
+ <itemizedlist>
+ <listitem>
+ <para>
+ <varname>io_context</varname> <literal>buffer pool</literal> refers to
+ IO operations on data in both the shared buffer pool and process-local
+ buffer pools used for temporary relation data.
+ </para>
+ <para>
+ Operations on temporary relations are tracked in
+ <varname>io_context</varname> <literal>buffer pool</literal> and
+ <varname>io_object</varname> <literal>temp relation</literal>.
+ </para>
+ <para>
+ Operations on permanent relations are tracked in
+ <varname>io_context</varname> <literal>buffer pool</literal> and
+ <varname>io_object</varname> <literal>relation</literal>.
+ </para>
+ </listitem>

For this column, you repeat "io_context" in the list describing the
possible values of the column. Enum-style columns in other tables
don't do that (e.g., the pg_stat_activty "state" column). I think it
might read better to omit "io_context" from the list.

+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>io_object</structfield> <type>text</type>
+ </para>
+ <para>
+ Object operated on in a given <varname>io_context</varname> by a given
+ <varname>backend_type</varname>.
+ </para>

Is this a fixed set of objects we should list, like for io_context?

Thanks,
Maciek