Subject: Re: Generate pg_stat_get_xact*() functions with Macros

From: Andres Freund <[email-masked]>
Date: Wed, 11 Jan 2023 14:59:07 -0800

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

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 12 Jan 2023 08:38:57 +0100

Hi,

> I'd split that up into a separate commit.

Thanks for looking at it! Makes sense, will do.

> 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).

> 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?

> I'd also rename f_self etc.

Makes sense, will do.

> 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).

> How about PG_STAT_GET_XACT_PLUS_SUBTRANS_INT64?

Sounds, better, thanks!

> PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which feels a

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