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

Hi Bertrand,

I'm glad you are working on this.

I had a few thoughts/ideas

It seems better to have all of the counts in the various stats structs
not be prefixed with n_, i_, t_

typedef struct PgStat_StatDBEntry
{
...
    PgStat_Counter n_blocks_fetched;
    PgStat_Counter n_blocks_hit;
    PgStat_Counter n_tuples_returned;
    PgStat_Counter n_tuples_fetched;
...

I've attached a patch (0002) to change this in case you are interested
in making such a change (I've attached all of my suggestions in patches
along with your original patch so that cfbot still passes).

On Wed, Nov 2, 2022 at 5:00 AM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
> On 11/1/22 1:30 AM, Andres Freund wrote:
> > On 2022-10-31 14:14:15 +0100, Drouvot, Bertrand wrote:
> >> @@ -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.

I looked for other opportunities to de-duplicate, but most of the functions
that were added that are identical except the return type and
PgStat_Kind are short enough that it doesn't make sense to make wrappers
or macros.

I do think it makes sense to reorder the members of the two structs
PgStat_IndexCounts and PgStat_TableCounts so that they have a common
header. I've done that in the attached patch (0003).

In the flush functions, I was also thinking it might be nice to use the
same pattern as is used in [1] and [2] to add the counts together. It
makes the lines a bit easier to read, IMO. If we remove the prefixes
from the count fields, this works for many of the fields. I've attached
a patch (0004) that does something like this, in case you wanted to go
in this direction.

Since you have made new parallel functions for indexes and tables for
many of the functions in pgstat_relation.c, perhaps it makes sense to
split it into pgstat_table.c and pgstat_index.c?

One question I had about the original code (not related to your
refactor) is why the pending stats aren't memset in the flush functions
after aggregating them into the shared stats.

- Melanie

[1] https://github.com/postgres/postgres/blob/master/src/backend/utils/activity/pgstat_checkpointer.c#L49
[2] https://github.com/postgres/postgres/blob/master/src/backend/utils/activity/pgstat_database.c#L370