Re: [Patch] New pg_stat_tablespace view - Mailing list pgsql-hackers

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