Re: [Patch] New pg_stat_tablespace view

Hi,

On 2026-10-09 00:49:20 -0400, shihao zhong wrote:
> @@ -113,6 +113,20 @@ pgstat_prepare_io_time(bool track_io_guc)
> void
> pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
> instr_time start_time, uint32 cnt, uint64 bytes)
> +{
> + pgstat_count_io_op_time_ext(io_object, io_context, io_op, start_time,
> + cnt, bytes, InvalidOid);
> +}

If everything uses pgstat_count_io_op_time_ext(), there's no point in keeping
pgstat_count_io_op_time(). I.e. we should not introduce _ext().

> +/*
> + * Like pgstat_count_io_op_time() except the time is also credited to
> + * tablespace "spcoid", for pg_stat_tablespace. The buffer manager uses this
> + * for relation I/O; everything else passes InvalidOid.
> + */
> +void
> +pgstat_count_io_op_time_ext(IOObject io_object, IOContext io_context,
> + IOOp io_op, instr_time start_time,
> + uint32 cnt, uint64 bytes, Oid spcoid)

One thing worth noting is that by adding another parameter to
pgstat_count_io_op_time() you pushed it into passing parameters via stack on
x86-64. That sometimes is noticeable.

> {
> if (!INSTR_TIME_IS_ZERO(start_time))
> {
> @@ -147,6 +161,10 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
> /* Add the per-backend count */
> pgstat_count_backend_io_op_time(io_object, io_context, io_op,
> io_time);
> +
> + /* Add the per-tablespace count */
> + if (OidIsValid(spcoid))
> + pgstat_count_tablespace_io_op_time(spcoid, io_op, io_time);
> }
>
> pgstat_count_io_op(io_object, io_context, io_op, cnt, bytes);
> @@ -162,11 +180,16 @@ pgstat_fetch_stat_io(void)

I'm not on board with adding an external function call to every invocation of
pgstat_count_backend_io_op_time(), particularly not one that then will also
often have to do a hash table lookup.

> +/*
> + * Fill in the counts of a relation or index that are kept per tablespace.
> + *
> + * Rows changed by a transaction that is still open are not in here yet. They
> + * are added when the transaction ends, see AtEOXact_PgStat_Relations().
> + */
> +static void
> +pgstat_relation_tablespace_counts(const PgStat_RelationStatus *ps,
> + PgStat_StatTabspaceEntry *counts)
> +{
> + memset(counts, 0, sizeof(*counts));
> +
> + if (ps->kind == PGSTAT_KIND_INDEX)
> + {
> + counts->tuples_returned = ps->idx.tuples_returned;
> + counts->tuples_fetched = ps->idx.tuples_fetched;
> + counts->blocks_fetched = ps->idx.blocks_fetched;
> + counts->blocks_hit = ps->idx.blocks_hit;
> + }
> + else
> + {
> + counts->tuples_returned = ps->tab.counts.tuples_returned;
> + counts->tuples_fetched = ps->tab.counts.tuples_fetched;
> + counts->tuples_inserted = ps->tab.counts_xact.tuples_inserted;
> + counts->tuples_updated = ps->tab.counts_xact.tuples_updated;
> + counts->tuples_deleted = ps->tab.counts_xact.tuples_deleted;
> + counts->blocks_fetched = ps->tab.counts.blocks_fetched;
> + counts->blocks_hit = ps->tab.counts.blocks_hit;
> + }
> +}

Uh, what is the justification for keeping all of these also for tablespaces?
This could just as well be done on the querying side, no?

Greetings,

Andres Freund