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
