Hi, Michael, CCing you because of the point about PG_STAT_GET_DBENTRY_FLOAT8 below.
On 2023-01-05 14:48:39 +0100, Drouvot, Bertrand wrote: > While at it, I also took the opportunity to create the macros for > pg_stat_get_xact_function_total_time(), > pg_stat_get_xact_function_self_time() and pg_stat_get_function_total_time(), > pg_stat_get_function_self_time() > (even if the same code pattern is only repeated two 2 times). I'd split that up into a separate commit. > Now that this patch renames some fields I don't mind renaming the fields - the prefixes really don't provide anything useful. But it's not clear why this is related to this patch? You could just include the f_ prefix in the macro, no? > , I think that, for consistency, those ones should be renamed too (aka remove > the f_ and t_ prefixes): > > PgStat_FunctionCounts.f_numcalls > PgStat_StatFuncEntry.f_numcalls > PgStat_TableCounts.t_truncdropped > PgStat_TableCounts.t_delta_live_tuples > PgStat_TableCounts.t_delta_dead_tuples > PgStat_TableCounts.t_changed_tuples Yea, without that the result is profoundly weird. > But I think it would be better to do it in a follow-up patch (once this one > get committed). I don't mind committing it in a separate commit, but I think it should be done at least immediately following the other commit. I.e. developed together. I probably would go the other way, and rename all of them first. That'd make this commit a lot more focused, and the renaming commit purely mechanical. Probably should remove PgStat_BackendFunctionEntry. PgStat_TableStatus has reason to exist, but that's not true for PgStat_BackendFunctionEntry. > @@ -168,19 +168,19 @@ pgstat_end_function_usage(PgStat_FunctionCallUsage > *fcu, bool finalize) > INSTR_TIME_ADD(total_func_time, f_self); > > /* > - * Compute the new f_total_time as the total elapsed time added to the > - * pre-call value of f_total_time. This is necessary to avoid > + * Compute the new total_time as the total elapsed time added to the > + * pre-call value of total_time. This is necessary to avoid > * double-counting any time taken by recursive calls of myself. (We do > * not need any similar kluge for self time, since that already excludes > * any recursive calls.) > */ > - INSTR_TIME_ADD(f_total, fcu->save_f_total_time); > + INSTR_TIME_ADD(f_total, fcu->save_total_time); > > /* update counters in function stats table */ > if (finalize) > fs->f_numcalls++; > - fs->f_total_time = f_total; > - INSTR_TIME_ADD(fs->f_self_time, f_self); > + fs->total_time = f_total; > + INSTR_TIME_ADD(fs->self_time, f_self); > } I'd also rename f_self etc. > @@ -148,29 +148,24 @@ pg_stat_get_function_calls(PG_FUNCTION_ARGS) > PG_RETURN_INT64(funcentry->f_numcalls); > } > > -Datum > -pg_stat_get_function_total_time(PG_FUNCTION_ARGS) > -{ > - Oid funcid = PG_GETARG_OID(0); > - PgStat_StatFuncEntry *funcentry; > - > - if ((funcentry = pgstat_fetch_stat_funcentry(funcid)) == NULL) > - PG_RETURN_NULL(); > - /* convert counter from microsec to millisec for display */ > - PG_RETURN_FLOAT8(((double) funcentry->f_total_time) / 1000.0); > +#define PG_STAT_GET_FUNCENTRY_FLOAT8(stat) > \ > +Datum > \ > +CppConcat(pg_stat_get_function_,stat)(PG_FUNCTION_ARGS) > \ > +{ > \ > + Oid funcid = PG_GETARG_OID(0); > \ > + PgStat_StatFuncEntry *funcentry; > \ > + > \ > + if ((funcentry = pgstat_fetch_stat_funcentry(funcid)) == NULL) \ > + PG_RETURN_NULL(); > \ > + /* convert counter from microsec to millisec for display */ > \ > + PG_RETURN_FLOAT8(((double) funcentry->stat) / 1000.0); > \ > } Hm. Given the conversion with / 1000, is PG_STAT_GET_FUNCENTRY_FLOAT8 an accurate name? Maybe PG_STAT_GET_FUNCENTRY_FLOAT8_MS? I now see that PG_STAT_GET_DBENTRY_FLOAT8 already exists, defined the same way. But the name fields misleading enough that I'd be inclined to rename it? > +#define PG_STAT_GET_XACT_WITH_SUBTRANS_RELENTRY_INT64(stat) > \ How about PG_STAT_GET_XACT_PLUS_SUBTRANS_INT64? Although I suspect this actually hints at an architectural thing that could be fixed better: Perhaps we should replace find_tabstat_entry() with a version returning a fully "reconciled" PgStat_StatTabEntry? It feels quite wrong to have that intimitate knowledge of the subtransaction stuff in pgstatfuncs.c and about how the different counts get combined. I think that'd allow us to move the definition of PgStat_TableStatus to PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which feels a heck of a lot cleaner. Greetings, Andres Freund