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

On 2022-08-22 13:15:18 -0400, Melanie Plageman wrote:

> Notes on the commit which accumulates IO Operation stats in shared
> memory:
>
> - I've extended the usage of the Assert()s that IO Operation stats that
> should be zero are. Previously we only checked the stats validity when
> querying the view. Now we check it when flushing pending stats and
> when reading the stats file into shared memory.

> Note that the three locations with these validity checks (when
> flushing pending stats, when reading stats file into shared memory,
> and when querying the view) have similar looking code to loop through
> and validate the stats. However, the actual action they perform if the
> stats are valid is different for each site (adding counters together,
> doing a read, setting nulls in a tuple column to true). Also, some of
> these instances have other code interspersed in the loops which would
> require additional looping if separated from this logic. So it was
> difficult to see a way of combining these into a single helper
> function.

All of them seem to repeat something like

> +                             if (!pgstat_bktype_io_op_valid(bktype, io_op) ||
> +                                     !pgstat_io_context_io_op_valid(io_context, io_op))

perhaps those could be combined? Afaics nothing uses pgstat_bktype_io_op_valid
separately.

> Subject: [PATCH v28 3/5] Track IO operation statistics locally
>
> Introduce "IOOp", an IO operation done by a backend, and "IOContext",
> the IO location source or target or IO type done by a backend. For
> example, the checkpointer may write a shared buffer out. This would be
> counted as an IOOp "write" on an IOContext IOCONTEXT_SHARED by
> BackendType "checkpointer".
>
> Each IOOp (alloc, extend, fsync, read, write) is counted per IOContext
> (local, shared, or strategy) through a call to pgstat_count_io_op().
>
> The primary concern of these statistics is IO operations on data blocks
> during the course of normal database operations. IO done by, for
> example, the archiver or syslogger is not counted in these statistics.

s/is/are/?

> Stats on IOOps for all IOContexts for a backend are counted in a
> backend's local memory. This commit does not expose any functions for
> aggregating or viewing these stats.

s/This commit does not/A subsequent commit will expose/...

> @@ -823,6 +823,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
>       BufferDesc *bufHdr;
>       Block           bufBlock;
>       bool            found;
> +     IOContext       io_context;
>       bool            isExtend;
>       bool            isLocalBuf = SmgrIsTemp(smgr);
> 
> @@ -986,10 +987,25 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
>        */
>       Assert(!(pg_atomic_read_u32(&bufHdr->state) & BM_VALID));       /* spinlock not needed */
> 
> -     bufBlock = isLocalBuf ? LocalBufHdrGetBlock(bufHdr) : BufHdrGetBlock(bufHdr);
> +     if (isLocalBuf)
> +     {
> +             bufBlock = LocalBufHdrGetBlock(bufHdr);
> +             io_context = IOCONTEXT_LOCAL;
> +     }
> +     else
> +     {
> +             bufBlock = BufHdrGetBlock(bufHdr);
> +
> +             if (strategy != NULL)
> +                     io_context = IOCONTEXT_STRATEGY;
> +             else
> +                     io_context = IOCONTEXT_SHARED;
> +     }

There's a isLocalBuf block earlier on, couldn't we just determine the context
there? I guess there's a branch here already, so it's probably fine as is.

>       if (isExtend)
>       {
> +
> +             pgstat_count_io_op(IOOP_EXTEND, io_context);

Spurious newline.

> @@ -2820,9 +2857,12 @@ BufferGetTag(Buffer buffer, RelFileLocator *rlocator, ForkNumber *forknum,
>   *
>   * If the caller has an smgr reference for the buffer's relation, pass it
>   * as the second parameter.  If not, pass NULL.
> + *
> + * IOContext will always be IOCONTEXT_SHARED except when a buffer access strategy is
> + * used and the buffer being flushed is a buffer from the strategy ring.
>   */
>  static void
> -FlushBuffer(BufferDesc *buf, SMgrRelation reln)
> +FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOContext io_context)

Too long line?

But also, why document the possible values here? Seems likely to get out of
date at some point, and it doesn't seem important to know?

> @@ -3549,6 +3591,8 @@ FlushRelationBuffers(Relation rel)
>                                                 localpage,
>                                                 false);
> 
> +                             pgstat_count_io_op(IOOP_WRITE, IOCONTEXT_LOCAL);
> +
>                               buf_state &= ~(BM_DIRTY | BM_JUST_DIRTIED);
>                               pg_atomic_unlocked_write_u32(&bufHdr->state, buf_state);
> 

Probably not worth doing, but these made me wonder whether there should be a
function for counting N operations at once.

> @@ -212,8 +215,23 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state)
>       if (strategy != NULL)
>       {
>               buf = GetBufferFromRing(strategy, buf_state);
> -             if (buf != NULL)
> +             *from_ring = buf != NULL;
> +             if (*from_ring)
> +             {

Don't really like the if (*from_ring) - why not keep it as buf != NULL? Seems
a bit confusing this way, making it less obvious what's being changed.

> diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
> index 014f644bf9..a3d76599bf 100644
> --- a/src/backend/storage/buffer/localbuf.c
> +++ b/src/backend/storage/buffer/localbuf.c
> @@ -15,6 +15,7 @@
>   */
>  #include "postgres.h"
> 
> +#include "pgstat.h"
>  #include "access/parallel.h"
>  #include "catalog/catalog.h"
>  #include "executor/instrument.h"

Do most other places not put pgstat.h in the alphabetical order of headers?

> @@ -432,6 +432,15 @@ ProcessSyncRequests(void)
>                                       total_elapsed += elapsed;
>                                       processed++;
> 
> +                                     /*
> +                                      * Note that if a backend using a BufferAccessStrategy is
> +                                      * forced to do its own fsync (as opposed to the
> +                                      * checkpointer doing it), it will not be counted as an
> +                                      * IOCONTEXT_STRATEGY IOOP_FSYNC and instead will be
> +                                      * counted as an IOCONTEXT_SHARED IOOP_FSYNC.
> +                                      */
> +                                     pgstat_count_io_op(IOOP_FSYNC, IOCONTEXT_SHARED);

Why is this noted here? Perhaps just point to the place where that happens
instead? I think it's also documented in ForwardSyncRequest()? Or just only
mention it there...

> @@ -0,0 +1,191 @@
> +/* -------------------------------------------------------------------------
> + *
> + * pgstat_io_ops.c
> + *     Implementation of IO operation statistics.
> + *
> + * This file contains the implementation of IO operation statistics. It is kept
> + * separate from pgstat.c to enforce the line between the statistics access /
> + * storage implementation and the details about individual types of
> + * statistics.
> + *
> + * Copyright (c) 2001-2022, PostgreSQL Global Development Group

Arguably this would just be 2021-2022

> +void
> +pgstat_count_io_op(IOOp io_op, IOContext io_context)
> +{
> +     PgStat_IOOpCounters *pending_counters = &pending_IOOpStats.data[io_context];
> +
> +     Assert(pgstat_expect_io_op(MyBackendType, io_context, io_op));
> +
> +     switch (io_op)
> +     {
> +             case IOOP_ALLOC:
> +                     pending_counters->allocs++;
> +                     break;
> +             case IOOP_EXTEND:
> +                     pending_counters->extends++;
> +                     break;
> +             case IOOP_FSYNC:
> +                     pending_counters->fsyncs++;
> +                     break;
> +             case IOOP_READ:
> +                     pending_counters->reads++;
> +                     break;
> +             case IOOP_WRITE:
> +                     pending_counters->writes++;
> +                     break;
> +     }
> +
> +}

How about replacing the breaks with a return and then erroring out if we reach
the end of the function? You did that below, and I think it makes sense.

> +bool
> +pgstat_bktype_io_context_valid(BackendType bktype, IOContext io_context)
> +{

Maybe add a tiny comment about what 'valid' means here? Something like
'return whether the backend type counts io in io_context'.

> +     /*
> +      * Only regular backends and WAL Sender processes executing queries should
> +      * use local buffers.
> +      */
> +     no_local = bktype == B_AUTOVAC_LAUNCHER || bktype ==
> +             B_BG_WRITER || bktype == B_CHECKPOINTER || bktype ==
> +             B_AUTOVAC_WORKER || bktype == B_BG_WORKER || bktype ==
> +             B_STANDALONE_BACKEND || bktype == B_STARTUP;

I think BG_WORKERS could end up using local buffers, extensions can do just
about everything in them.

> +bool
> +pgstat_bktype_io_op_valid(BackendType bktype, IOOp io_op)
> +{
> +     if ((bktype == B_BG_WRITER || bktype == B_CHECKPOINTER) && io_op ==
> +             IOOP_READ)
> +             return false;

Perhaps we should add an assertion about the backend type making sense here?
I.e. that it's not archiver, walwriter etc?

> +bool
> +pgstat_io_context_io_op_valid(IOContext io_context, IOOp io_op)
> +{
> +     /*
> +      * Temporary tables using local buffers are not logged and thus do not
> +      * require fsync'ing. Set this cell to NULL to differentiate between an
> +      * invalid combination and 0 observed IO Operations.

This comment feels a bit out of place?

> +bool
> +pgstat_expect_io_op(BackendType bktype, IOContext io_context, IOOp io_op)
> +{
> +     if (!pgstat_io_op_stats_collected(bktype))
> +             return false;
> +
> +     if (!pgstat_bktype_io_context_valid(bktype, io_context))
> +             return false;
> +
> +     if (!pgstat_bktype_io_op_valid(bktype, io_op))
> +             return false;
> +
> +     if (!pgstat_io_context_io_op_valid(io_context, io_op))
> +             return false;
> +
> +     /*
> +      * There are currently no cases of a BackendType, IOContext, IOOp
> +      * combination that are specifically invalid.
> +      */

"specifically"?

> From 0f141fa7f97a57b8628b1b6fd6029bd3782f16a1 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Mon, 22 Aug 2022 11:35:20 -0400
> Subject: [PATCH v28 4/5] Aggregate IO operation stats per BackendType
>
> Stats on IOOps for all IOContexts for a backend are tracked locally. Add
> functionality for backends to flush these stats to shared memory and
> accumulate them with those from all other backends, exited and live.
> Also add reset and snapshot functions used by cumulative stats system
> for management of these statistics.
>
> The aggregated stats in shared memory could be extended in the future
> with per-backend stats -- useful for per connection IO statistics and
> monitoring.
>
> Some BackendTypes will not flush their pending statistics at regular
> intervals and explicitly call pgstat_flush_io_ops() during the course of
> normal operations to flush their backend-local IO Operation statistics
> to shared memory in a timely manner.

> Because not all BackendType, IOOp, IOContext combinations are valid, the
> validity of the stats are checked before flushing pending stats and
> before reading in the existing stats file to shared memory.

s/are checked/is checked/?

> @@ -1486,6 +1507,42 @@ pgstat_read_statsfile(void)
>       if (!read_chunk_s(fpin, &shmem->checkpointer.stats))
>               goto error;
> 
> +     /*
> +      * Read IO Operations stats struct
> +      */
> +     if (!read_chunk_s(fpin, &shmem->io_ops.stat_reset_timestamp))
> +             goto error;
> +
> +     for (int backend_type = 0; backend_type < BACKEND_NUM_TYPES; backend_type++)
> +     {
> +             PgStatShared_IOContextOps *backend_io_context_ops = &shmem->io_ops.stats[backend_type];
> +             bool            expect_backend_stats = true;
> +
> +             if (!pgstat_io_op_stats_collected(backend_type))
> +                     expect_backend_stats = false;
> +
> +             for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
> +             {
> +                     if (!expect_backend_stats ||
> +                             !pgstat_bktype_io_context_valid(backend_type, io_context))
> +                     {
> +                             pgstat_io_context_ops_assert_zero(&backend_io_context_ops->data[io_context]);
> +                             continue;
> +                     }
> +
> +                     for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
> +                     {
> +                             if (!pgstat_bktype_io_op_valid(backend_type, io_op) ||
> +                                     !pgstat_io_context_io_op_valid(io_context, io_op))
> +                                     pgstat_io_op_assert_zero(&backend_io_context_ops->data[io_context],
> +                                                                                      io_op);
> +                     }
> +             }
> +
> +             if (!read_chunk_s(fpin, &backend_io_context_ops->data))
> +                     goto error;
> +     }

Could we put the validation out of line? That's a lot of io stats specific
code to be in pgstat_read_statsfile().

> +/*
> + * Helper function to accumulate PgStat_IOOpCounters. If either of the
> + * passed-in PgStat_IOOpCounters are members of PgStatShared_IOContextOps, the
> + * caller is responsible for ensuring that the appropriate lock is held. This
> + * is not asserted because this function could plausibly be used to accumulate
> + * two local/pending PgStat_IOOpCounters.

What's "this" here?

> @@ -496,6 +503,8 @@ extern PgStat_CheckpointerStats *pgstat_fetch_stat_checkpointer(void);
>   */
> 
>  extern void pgstat_count_io_op(IOOp io_op, IOContext io_context);
> +extern PgStat_BackendIOContextOps *pgstat_fetch_backend_io_context_ops(void);
> +extern bool pgstat_flush_io_ops(bool nowait);
>  extern const char *pgstat_io_context_desc(IOContext io_context);
>  extern const char *pgstat_io_op_desc(IOOp io_op);
> 

Is there any call to pgstat_flush_io_ops() from outside pgstat*.c? So possibly
it could be in pgstat_internal.h? Not that it's particularly important...

> @@ -506,6 +515,43 @@ extern bool pgstat_bktype_io_op_valid(BackendType bktype, IOOp io_op);
>  extern bool pgstat_io_context_io_op_valid(IOContext io_context, IOOp io_op);
>  extern bool pgstat_expect_io_op(BackendType bktype, IOContext io_context, IOOp io_op);
> 
> +/*
> + * Functions to assert that invalid IO Operation counters are zero. Used with
> + * the validation functions in pgstat_io_ops.c
> + */
> +static inline void
> +pgstat_io_context_ops_assert_zero(PgStat_IOOpCounters *counters)
> +{
> +     Assert(counters->allocs == 0 && counters->extends == 0 &&
> +                counters->fsyncs == 0 && counters->reads == 0 &&
> +                counters->writes == 0);
> +}
> +
> +static inline void
> +pgstat_io_op_assert_zero(PgStat_IOOpCounters *counters, IOOp io_op)
> +{
> +     switch (io_op)
> +     {
> +             case IOOP_ALLOC:
> +                     Assert(counters->allocs == 0);
> +                     return;
> +             case IOOP_EXTEND:
> +                     Assert(counters->extends == 0);
> +                     return;
> +             case IOOP_FSYNC:
> +                     Assert(counters->fsyncs == 0);
> +                     return;
> +             case IOOP_READ:
> +                     Assert(counters->reads == 0);
> +                     return;
> +             case IOOP_WRITE:
> +                     Assert(counters->writes == 0);
> +                     return;
> +     }
> +
> +     elog(ERROR, "unrecognized IOOp value: %d", io_op);

Hm. This means it'll emit code even in non-assertion builds - this should
probably just be an Assert(false) or pg_unreachable().

> Subject: [PATCH v28 5/5] Add system view tracking IO ops per backend type

> View stats are fetched from statistics incremented when a backend
> performs an IO Operation and maintained by the cumulative statistics
> subsystem.

"fetched from statistics incremented"?

> Each row of the view is stats for a particular BackendType for a
> particular IOContext (e.g. shared buffer accesses by checkpointer) and
> each column in the view is the total number of IO Operations done (e.g.
> writes).

s/is/shows/?

s/for a particular BackendType for a particular IOContext/for a particularl
BackendType and IOContext/? Somehow the repetition is weird.

> diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
> index 9440b41770..9949011ba3 100644
> --- a/doc/src/sgml/monitoring.sgml
> +++ b/doc/src/sgml/monitoring.sgml
> @@ -448,6 +448,15 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
>       </entry>
>       </row>
> 
> +     <row>
> +      <entry><structname>pg_stat_io</structname><indexterm><primary>pg_stat_io</primary></indexterm></entry>
> +      <entry>A row for each IO Context for each backend type showing
> +      statistics about backend IO operations. See
> +       <link linkend="monitoring-pg-stat-io-view">
> +       <structname>pg_stat_io</structname></link> for details.
> +     </entry>
> +     </row>

The "for each for each" thing again :)

> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>io_context</structfield> <type>text</type>
> +      </para>
> +      <para>
> +       IO Context used (e.g. shared buffers, direct).
> +      </para></entry>
> +     </row>

Wrong list of contexts.

> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>alloc</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of buffers allocated.
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>extend</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of blocks extended.
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>fsync</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of blocks fsynced.
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>read</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of blocks read.
> +      </para></entry>
> +     </row>
> +
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>write</structfield> <type>bigint</type>
> +      </para>
> +      <para>
> +       Number of blocks written.
> +      </para></entry>
> +     </row>

> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>stats_reset</structfield> <type>timestamp with time zone</type>
> +      </para>
> +      <para>
> +       Time at which these statistics were last reset.
>        </para></entry>
>       </row>
>      </tbody>

Part of me thinks it'd be nicer if it were "allocated, read, written, extended,
fsynced, stats_reset", instead of alphabetical order. The order already isn't
alphabetical.

> +     /*
> +      * When adding a new column to the pg_stat_io view, add a new enum value
> +      * here above IO_NUM_COLUMNS.
> +      */
> +     enum
> +     {
> +             IO_COLUMN_BACKEND_TYPE,
> +             IO_COLUMN_IO_CONTEXT,
> +             IO_COLUMN_ALLOCS,
> +             IO_COLUMN_EXTENDS,
> +             IO_COLUMN_FSYNCS,
> +             IO_COLUMN_READS,
> +             IO_COLUMN_WRITES,
> +             IO_COLUMN_RESET_TIME,
> +             IO_NUM_COLUMNS,
> +     };

Given it's local and some of the lines are long, maybe just use COL?

> +#define IO_COLUMN_IOOP_OFFSET (IO_COLUMN_IO_CONTEXT + 1)

Undef'ing it probably worth doing.

> +     SetSingleFuncCall(fcinfo, 0);
> +     rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
> +
> +     backends_io_stats = pgstat_fetch_backend_io_context_ops();
> +
> +     reset_time = TimestampTzGetDatum(backends_io_stats->stat_reset_timestamp);
> +
> +     for (int bktype = 0; bktype < BACKEND_NUM_TYPES; bktype++)
> +     {
> +             Datum           bktype_desc = CStringGetTextDatum(GetBackendTypeDesc(bktype));
> +             bool            expect_backend_stats = true;
> +             PgStat_IOContextOps *io_context_ops = &backends_io_stats->stats[bktype];
> +
> +             /*
> +              * For those BackendTypes without IO Operation stats, skip
> +              * representing them in the view altogether.
> +              */
> +             if (!pgstat_io_op_stats_collected(bktype))
> +                     expect_backend_stats = false;

Why not just expect_backend_stats = pgstat_io_op_stats_collected()?

> +             for (int io_context = 0; io_context < IOCONTEXT_NUM_TYPES; io_context++)
> +             {
> +                     PgStat_IOOpCounters *counters = &io_context_ops->data[io_context];
> +                     Datum           values[IO_NUM_COLUMNS];
> +                     bool            nulls[IO_NUM_COLUMNS];
> +
> +                     /*
> +                      * Some combinations of IOCONTEXT and BackendType are not valid
> +                      * for any type of IO Operation. In such cases, omit the entire
> +                      * row from the view.
> +                      */
> +                     if (!expect_backend_stats ||
> +                             !pgstat_bktype_io_context_valid(bktype, io_context))
> +                     {
> +                             pgstat_io_context_ops_assert_zero(counters);
> +                             continue;
> +                     }
> +
> +                     memset(values, 0, sizeof(values));
> +                     memset(nulls, 0, sizeof(nulls));

I'd replace the memset with values[...] = {0} etc.

> +                     values[IO_COLUMN_BACKEND_TYPE] = bktype_desc;
> +                     values[IO_COLUMN_IO_CONTEXT] = CStringGetTextDatum(
> +                                                                                                                        pgstat_io_context_desc(io_context));

Pgindent, I hate you.

Perhaps put it the context desc in a local var, so it doesn't look quite this
ugly?

> +-- Test that allocs, extends, reads, and writes to Shared Buffers and fsyncs
> +-- done to ensure durability of Shared Buffers are tracked in pg_stat_io.
> +SELECT sum(alloc) AS io_sum_shared_allocs_before FROM pg_stat_io WHERE io_context = 'Shared' \gset
> +SELECT sum(extend) AS io_sum_shared_extends_before FROM pg_stat_io WHERE io_context = 'Shared' \gset
> +SELECT sum(fsync) AS io_sum_shared_fsyncs_before FROM pg_stat_io WHERE io_context = 'Shared' \gset
> +SELECT sum(read) AS io_sum_shared_reads_before FROM pg_stat_io WHERE io_context = 'Shared' \gset
> +SELECT sum(write) AS io_sum_shared_writes_before FROM pg_stat_io WHERE io_context = 'Shared' \gset
> +-- Create a regular table and insert some data to generate IOCONTEXT_SHARED allocs and extends.
> +CREATE TABLE test_io_shared(a int);
> +INSERT INTO test_io_shared SELECT i FROM generate_series(1,100)i;
> +SELECT pg_stat_force_next_flush();
> + pg_stat_force_next_flush
> +--------------------------
> +
> +(1 row)
> +
> +-- After a checkpoint, there should be some additional IOCONTEXT_SHARED writes and fsyncs.
> +CHECKPOINT;

Does that work reliably? A checkpoint could have started just before the
CREATE TABLE, I think? Then it'd not have flushed those writes yet. I think
doing two checkpoints would protect against that.

> +DROP TABLE test_io_shared;
> +DROP TABLESPACE test_io_shared_stats_tblspc;

Tablespace creation is somewhat expensive, do we really need that? There
should be one set up in setup.sql or such.

> +-- Test that allocs, extends, reads, and writes of temporary tables are tracked
> +-- in pg_stat_io.
> +CREATE TEMPORARY TABLE test_io_local(a int, b TEXT);
> +SELECT sum(alloc) AS io_sum_local_allocs_before FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT sum(extend) AS io_sum_local_extends_before FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT sum(read) AS io_sum_local_reads_before FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT sum(write) AS io_sum_local_writes_before FROM pg_stat_io WHERE io_context = 'Local' \gset
> +-- Insert enough values that we need to reuse and write out dirty local
> +-- buffers.
> +INSERT INTO test_io_local SELECT generate_series(1, 80000) as id,
> +'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';

Could be abbreviated with repeat('a', some-number) :P

Can the table be smaller than this? That might show up on a slow machine.

> +SELECT sum(alloc) AS io_sum_local_allocs_after FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT sum(extend) AS io_sum_local_extends_after FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT sum(read) AS io_sum_local_reads_after FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT sum(write) AS io_sum_local_writes_after FROM pg_stat_io WHERE io_context = 'Local' \gset
> +SELECT :io_sum_local_allocs_after > :io_sum_local_allocs_before;

Random q: Why are we uppercasing  the first letter of the context?

> +CREATE TABLE test_io_strategy(a INT, b INT);
> +ALTER TABLE test_io_strategy SET (autovacuum_enabled = 'false');

I think you can specify that as part of the CREATE TABLE. Not sure if
otherwise there's not a race where autovac coul start before you do the ALTER.

> +INSERT INTO test_io_strategy SELECT i, i from generate_series(1, 8000)i;
> +-- Ensure that the next VACUUM will need to perform IO by rewriting the table
> +-- first with VACUUM (FULL).

... because VACUUM FULL currently doesn't set all-visible etc on the pages,
which the subsequent vacuum will then do.

> +-- Hope that the previous value of wal_skip_threshold was the default. We
> +-- can't use BEGIN...SET LOCAL since VACUUM can't be run inside a transaction
> +-- block.
> +RESET wal_skip_threshold;

Nothing in this file set it before, so that's a pretty sure-to-be-fulfilled
hope.

> +-- Test that, when using a Strategy, if creating a relation, Strategy extends

s/if/when/?