Re: Generate pg_stat_get_xact*() functions with Macros

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