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