Re: Generate pg_stat_get_xact*() functions with Macros
От | Drouvot, Bertrand |
---|---|
Тема | Re: Generate pg_stat_get_xact*() functions with Macros |
Дата | |
Msg-id | e6191040-40c4-6849-7b5d-bf3c227f1755@gmail.com обсуждение исходный текст |
Ответ на | Re: Generate pg_stat_get_xact*() functions with Macros (Andres Freund <andres@anarazel.de>) |
Ответы |
Re: Generate pg_stat_get_xact*() functions with Macros
(Andres Freund <andres@anarazel.de>)
|
Список | pgsql-hackers |
Hi, On 1/11/23 11:59 PM, Andres Freund wrote: > 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. > > Thanks for looking at it! Makes sense, will do. >> 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? > > Right, but the idea is to take the same approach that the one used in 8018ffbf58 (where placing the prefixes in the macro would have been possible too). >> , 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. > Yeah, makes sense. Let's proceed that way. I'll provide the "rename" patch. > Probably should remove PgStat_BackendFunctionEntry. I think that would be a 3rd patch, agree? >> @@ -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. > Makes sense, will do. >> @@ -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? > PG_STAT_GET_FUNCENTRY_FLOAT8_MS looks good by me. Waiting on what we'll decide for the existing PG_STAT_GET_DBENTRY_FLOAT8 (so that I can align for the PG_STAT_GET_FUNCENTRY_FLOAT8). >> +#define PG_STAT_GET_XACT_WITH_SUBTRANS_RELENTRY_INT64(stat) \ > > How about PG_STAT_GET_XACT_PLUS_SUBTRANS_INT64? > Sounds, better, thanks! > 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. Yeah, I think that would be for a 4th patch, agree? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
В списке pgsql-hackers по дате отправления:
Следующее
От: Will MortensenДата:
Сообщение: Re: Exposing the lock manager's WaitForLockers() to SQL