Re: Split index and table statistics into different types of stats - Mailing list pgsql-hackers

Hi,

On 11/1/22 1:30 AM, Andres Freund wrote:
> 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!
> 

Thanks for looking at it!

> 
> 
>> 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.
> 

Agree, replaced by "table" where appropriate in V3 attached.

> 
>>   /*
>> @@ -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?
> 
> 

Agree that it should be split in 
pgstat_create_table()/pgstat_create_index() (also as it was already 
split for the "drop" case): done in V3.

>> +/*
>> + * 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".
> 

Yes I think so as pgstat_fetch_stat_indentry_ext() has its use case in 
pgstat_copy_index_stats() (previously pgstat_copy_relation_stats()).

> 
> 
>> @@ -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.
> 

Yeah good point, a new macro has been defined for the "int64" return 
case in V3 attached.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com