Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry

Hi,

On 2023-02-09 11:38:18 +0100, Drouvot, Bertrand wrote:
> Please find attached a patch proposal for $SUBJECT.
> 
> The idea has been raised in [1] by Andres: it would allow to simplify even more the work done to
> generate pg_stat_get_xact*() functions with Macros.

Thanks!

I think this is useful beyond being able to generate those functions with
macros. The fact that we had to deal with transactional code in pgstatfuncs.c
meant that a lot of the relevant itnernals had to be exposed "outside" pgstat,
which seems architecturally not great.


> Indeed, with the reconciliation done in find_tabstat_entry() then all the pg_stat_get_xact*() functions
> (making use of find_tabstat_entry()) now "look the same" (should they take into account live subtransactions or
not).

I'm not bothered by making all of pg_stat_get_xact* functions more expensive,
they're not a hot code path. But if we need to, we could just add a parameter
to find_tabstat_entry() indicating whether we need to reconcile or not.


>      /* save stats for this function, later used to compensate for recursion */
> -    fcu->save_f_total_time = pending->f_counts.f_total_time;
> +    fcu->save_f_total_time = pending->f_total_time;
>  
>      /* save current backend-wide total time */
>      fcu->save_total = total_func_time;

The diff is noisy due to all the mechanical changes like the above. Could that
be split into a separate commit?


>  find_tabstat_entry(Oid rel_id)
>  {
>      PgStat_EntryRef *entry_ref;
> +    PgStat_TableStatus *tablestatus = NULL;
>  
>      entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
>      if (!entry_ref)
>          entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
>  
>      if (entry_ref)
> -        return entry_ref->pending;
> -    return NULL;
> +    {
> +        PgStat_TableStatus *tabentry = (PgStat_TableStatus *) entry_ref->pending;

I'd add an early return for the !entry_ref case, that way you don't need to
indent the bulk of the function.


> +        PgStat_TableXactStatus *trans;
> +
> +        tablestatus  = palloc(sizeof(PgStat_TableStatus));
> +        memcpy(tablestatus, tabentry, sizeof(PgStat_TableStatus));

For things like this I'd use
  *tablestatus = *tabentry;

that way the compiler will warn you about mismatching types, and you don't
need the sizeof().


> +        /* live subtransactions' counts aren't in t_counts yet */
> +        for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
> +        {
> +            tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted;
> +            tablestatus->t_counts.t_tuples_updated += trans->tuples_updated;
> +            tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted;
> +        }
> +    }

Hm, why do we end uup with t_counts still being used here, but removed other
places?


> diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
> index 6737493402..40a6fbf871 100644
> --- a/src/backend/utils/adt/pgstatfuncs.c
> +++ b/src/backend/utils/adt/pgstatfuncs.c
> @@ -1366,7 +1366,10 @@ pg_stat_get_xact_numscans(PG_FUNCTION_ARGS)
>      if ((tabentry = find_tabstat_entry(relid)) == NULL)
>          result = 0;
>      else
> +    {
>          result = (int64) (tabentry->t_counts.t_numscans);
> +        pfree(tabentry);
> +    }
>  
>      PG_RETURN_INT64(result);
>  }

I don't think we need to bother with individual pfrees in this path. The
caller will call the function in a dedicated memory context, that'll be reset
very soon after this.

Greetings,

Andres Freund