Re: Split index and table statistics into different types of stats

Hi,

On 2022-10-31 14:14:15 +0100, Drouvot, Bertrand wrote:
> Please find attached a patch proposal to split index and table statistics
> into different types of stats.
>
> This idea has been proposed by Andres in a couple of threads, see [1] and
> [2].

Thanks for working on this!

> diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
> index 5b49cc5a09..8a715db82e 100644
> --- a/src/backend/catalog/heap.c
> +++ b/src/backend/catalog/heap.c
> @@ -1853,7 +1853,7 @@ heap_drop_with_catalog(Oid relid)
> RelationDropStorage(rel);
>
> /* ensure that stats are dropped if transaction commits */
> - pgstat_drop_relation(rel);
> + pgstat_drop_heap(rel);

I don't think "heap" is a good name for these, even if there's some historical
reasons for it. Particularly because you used "table" in some bits and pieces
too.

> /*
> @@ -168,39 +210,55 @@ pgstat_unlink_relation(Relation rel)
> void
> pgstat_create_relation(Relation rel)
> {
> - pgstat_create_transactional(PGSTAT_KIND_RELATION,
> - rel->rd_rel->relisshared ? InvalidOid : MyDatabaseId,
> - RelationGetRelid(rel));
> + if (rel->rd_rel->relkind == RELKIND_INDEX)
> + pgstat_create_transactional(PGSTAT_KIND_INDEX,
> + rel->rd_rel->relisshared ? InvalidOid : MyDatabaseId,
> + RelationGetRelid(rel));
> + else
> + pgstat_create_transactional(PGSTAT_KIND_TABLE,
> + rel->rd_rel->relisshared ? InvalidOid : MyDatabaseId,
> + RelationGetRelid(rel));
> +}

Hm - why is this best handled on this level, rather than at the caller?

> +/*
> + * Support function for the SQL-callable pgstat* functions. Returns
> + * the collected statistics for one index or NULL. NULL doesn't mean
> + * that the index doesn't exist, just that there are no statistics, so the
> + * caller is better off to report ZERO instead.
> + */
> +PgStat_StatIndEntry *
> +pgstat_fetch_stat_indentry(Oid relid)
> +{
> + PgStat_StatIndEntry *indentry;
> +
> + indentry = pgstat_fetch_stat_indentry_ext(false, relid);
> + if (indentry != NULL)
> + return indentry;
> +
> + /*
> + * If we didn't find it, maybe it's a shared index.
> + */
> + indentry = pgstat_fetch_stat_indentry_ext(true, relid);
> + return indentry;
> +}
> +
> +/*
> + * More efficient version of pgstat_fetch_stat_indentry(), allowing to specify
> + * whether the to-be-accessed index is shared or not.
> + */
> +PgStat_StatIndEntry *
> +pgstat_fetch_stat_indentry_ext(bool shared, Oid reloid)
> +{
> + Oid dboid = (shared ? InvalidOid : MyDatabaseId);
> +
> + return (PgStat_StatIndEntry *)
> + pgstat_fetch_entry(PGSTAT_KIND_INDEX, dboid, reloid);
> }

Do we need this split anywhere for now? I suspect not, the table case is
mainly for the autovacuum launcher, which won't look at indexes "in isolation".

> @@ -240,9 +293,23 @@ pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
> PG_RETURN_INT64(result);
> }
>
> +Datum
> +pg_stat_get_index_blocks_fetched(PG_FUNCTION_ARGS)
> +{
> + Oid relid = PG_GETARG_OID(0);
> + int64 result;
> + PgStat_StatIndEntry *indentry;
> +
> + if ((indentry = pgstat_fetch_stat_indentry(relid)) == NULL)
> + result = 0;
> + else
> + result = (int64) (indentry->blocks_fetched);
> +
> + PG_RETURN_INT64(result);
> +}

We have so many copies of this by now - perhaps we first should deduplicate
them somehow? Even if it's just a macro or such.

Greetings,

Andres Freund