Re: pg_stat_io not tracking smgrwriteback() is confusing

v5 attached.

On Thu, May 4, 2023 at 12:44 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-04-27 11:36:49 -0400, Melanie Plageman wrote:
> > > > /* and finally tell the kernel to write the data to storage */
> > > > reln = smgropen(currlocator, InvalidBackendId);
> > > > smgrwriteback(reln, BufTagGetForkNum(&tag), tag.blockNum, nblocks);
> >
> > Yes, as it is currently, IssuePendingWritebacks() is only used for shared
> > buffers. My rationale for including IOObject is that localbuf.c calls
> > smgr* functions and there isn't anything stopping it from calling
> > smgrwriteback() or using WritebackContexts (AFAICT).
>
> I think it's extremely unlikely that we'll ever do that, because it's very
> common to have temp tables that are bigger than temp_buffers. We basically
> hope that the kernel can do good caching for us there.
>
>
> > > Or I actually think we might not even need to pass around the io_*
> > > parameters and could just pass immediate values to the
> > > pgstat_count_io_op_time call. If we ever start using shared buffers
> > > for thing other than relation files (for example SLRU?), we'll have to
> > > consider the target individually for each buffer block. That being
> > > said, I'm fine with how it is either.
> >
> > In IssuePendingWritebacks() we don't actually know which IOContext we
> > are issuing writebacks for when we call pgstat_count_io_op_time() (we do
> > issue pending writebacks for other IOContexts than IOCONTEXT_NORMAL). I
> > agree IOObject is not strictly necessary right now. I've kept IOObject a
> > member of WritebackContext for the reasons I mention above, however, I
> > am open to removing it if it adds confusion.
>
> I don't think it's really worth adding struct members for potential future
> safety. We can just add them later if we end up needing them.

I've removed both members of WritebackContext and hard-coded
IOOBJECT_RELATION in the call to pgstat_count_io_op_time().

> > From 7cdd6fc78ed82180a705ab9667714f80d08c5f7d Mon Sep 17 00:00:00 2001
> > From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> > Date: Mon, 24 Apr 2023 18:21:54 -0400
> > Subject: [PATCH v4] Add writeback to pg_stat_io
> >
> > 28e626bde00 added the notion of IOOps but neglected to include
> > writeback. With the addition of IO timing to pg_stat_io in ac8d53dae5,
> > the omission of writeback caused some confusion. Checkpointer write
> > timing in pg_stat_io often differed greatly from the write timing
> > written to the log. To fix this, add IOOp IOOP_WRITEBACK and track
> > writebacks and writeback timing in pg_stat_io.
>
> For the future: It'd be good to note that catversion needs to be increased.

Noted. I've added it to the commit message since I did a new version
anyway.

> > index 99f7f95c39..27b6f1a0a0 100644
> > --- a/doc/src/sgml/monitoring.sgml
> > +++ b/doc/src/sgml/monitoring.sgml
> > @@ -3867,6 +3867,32 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
> > </entry>
> > </row>
> >
> > + <row>
> > + <entry role="catalog_table_entry">
> > + <para role="column_definition">
> > + <structfield>writebacks</structfield> <type>bigint</type>
> > + </para>
> > + <para>
> > + Number of units of size <varname>op_bytes</varname> which the backend
> > + requested the kernel write out to permanent storage.
> > + </para>
> > + </entry>
> > + </row>
>
> I think the reference to "backend" here is somewhat misplaced - it could be
> checkpointer or bgwriter as well. We don't reference the backend in other
> comparable columns of pgsio either...

So, I tried to come up with something that doesn't make reference to
any "requester" of the writeback and the best I could do was:

"Number of units of size op_bytes requested that the kernel write out."

This is awfully awkward sounding.

"backend_type" is the name of the column in pg_stat_io. Client backends
are always referred to as such in the pg_stat_io documentation. Thus, I
think it is reasonable to use the word "backend" and assume people
understand it could be any type of backend.

However, since the existing docs for pg_stat_bgwriter use "backend" to
mean "client backend", and I see a few uses of the word "process" in the
stats docs, I've changed my use of the word "backend" to "process".

> > diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
> > index 0057443f0c..a7182fe95a 100644
> > --- a/src/backend/storage/buffer/buf_init.c
> > +++ b/src/backend/storage/buffer/buf_init.c
> > @@ -145,9 +145,15 @@ InitBufferPool(void)
> > /* Init other shared buffer-management stuff */
> > StrategyInitialize(!foundDescs);
> >
> > - /* Initialize per-backend file flush context */
> > - WritebackContextInit(&BackendWritebackContext,
> > - &backend_flush_after);
> > + /*
> > + * Initialize per-backend file flush context. IOContext is initialized to
> > + * IOCONTEXT_NORMAL because this is the most common context. IOObject is
> > + * initialized to IOOBJECT_RELATION because writeback is currently only
> > + * requested for permanent relations in shared buffers. The backend can
> > + * overwrite these as appropriate.
> > + */
> > + WritebackContextInit(&BackendWritebackContext, IOOBJECT_RELATION,
> > + IOCONTEXT_NORMAL, &backend_flush_after);
> > }
> >
>
> This seems somewhat icky.

I've removed both IOObject and IOContext from WritebackContext.

> > /*
> > diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
> > index 1fa689052e..116910cdfe 100644
> > --- a/src/backend/storage/buffer/bufmgr.c
> > +++ b/src/backend/storage/buffer/bufmgr.c
> > @@ -1685,6 +1685,8 @@ again:
> > FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context);
> > LWLockRelease(content_lock);
> >
> > + BackendWritebackContext.io_object = IOOBJECT_RELATION;
> > + BackendWritebackContext.io_context = io_context;
> > ScheduleBufferTagForWriteback(&BackendWritebackContext,
> > &buf_hdr->tag);
> > }
>
> What about passing the io_context to ScheduleBufferTagForWriteback instead?

I assume we don't want to include the time spent in
sort_pending_writebacks(), so I've added io_context as a parameter to
ScheduleBufferTagForWriteback and threaded it through
IssuePendingWritebacks() as well.

Because WritebackContext was just called "context" as a function
parameter to these functions and that was easy to confuse with
"io_context", I've changed the name of the WritebackContext function
parameter to "wb_context". I separated this rename into its own commit so
that the diff for the commit adding writeback is more clear. I assume
the committer will squash those commits.

> > --- a/src/test/regress/sql/stats.sql
> > +++ b/src/test/regress/sql/stats.sql
>
> Hm. Could we add a test for this? While it's not implemented everywhere, we
> still issue the smgrwriteback() afaics. The default for the _flush_after GUCs
> changes, but backend_flush_after is USERSET, so we could just change it for a
> single command.

I couldn't come up with a way to write a test for this.
GetVictimBuffer() is only called when flushing a dirty buffer. I tried
adding backend_flush_after = 1 and sum(writebacks) to the test for
reuses with vacuum strategy, but there were never more writebacks (in
any context) after doing the full table rewrite with VACUUM. I presume
this is because checkpointer or bgwriter is writing out the dirty
buffers before our client backend gets to reusing them. And, since
bgwriter/checkpointer_flush_after are not USERSET, I don't think we can
guarantee this will cause writeback operations. Just anecdotally, I
increased size of the table to exceed checkpoint_flush_after on my
Postgres instance and I could get the test to cause writeback, but that
doesn't work for a portable test. This was the same reason we couldn't
test writes for VACUUM strategy.

I did notice while working on this that, with the addition of the VACUUM
parameter BUFFER_USAGE_LIMIT, we could decrease the size of the table in
the vacuum strategy reuses test. Not sure if this is legit to commit now
since it isn't required for the writeback patch set, but I included a
patch for it in this patchset.

On Thu, May 4, 2023 at 12:57 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-04-24 21:29:48 -0400, Melanie Plageman wrote:
>
> > 2) I'm a little nervous about not including IOObject in the writeback
> > context. Technically, there is nothing stopping local buffer code from
> > calling IssuePendingWritebacks(). Right now, local buffer code doesn't
> > do ScheduleBufferTagForWriteback(). But it doesn't seem quite right to
> > hardcode in IOOBJECT_RELATION when there is nothing wrong with
> > requesting writeback of local buffers (AFAIK). What do you think?
>
> I think it'd be wrong on performance grounds ;). We could add an assertion to
> ScheduleBufferTagForWriteback(), I guess, to document that fact?

Now that it doesn't have access to IOObject, no need. I've added
comments elsewhere.

> > 3) Should any restrictions be added to pgstat_tracks_io_object() or
> > pgstat_tracks_io_op()? I couldn't think of any backend types or IO
> > contexts which would not do writeback as a rule. Also, though we don't
> > do writeback for temp tables now, it isn't nonsensical to do so. In
> > this version, I didn't add any restrictions.
>
> I think the temp table restriction could be encoded for now, I don't forsee
> that changing anytime soon.

I've done that in the attached v5.

- Melanie