Message-ID: <e6191040-40c4-6849-7b5d-bf3c227f1755@gmail.com>
Date: Thu, 12 Jan 2023 08:38:57 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: Generate pg_stat_get_xact*() functions with Macros
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>,
 Michael Paquier <michael.paquier@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <89606d96-cd94-af74-18f3-c7ab2b684ba2@gmail.com>
 <20230111225907.6el6c5j3hukizqxc@awork3.anarazel.de>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <20230111225907.6el6c5j3hukizqxc@awork3.anarazel.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/e6191040-40c4-6849-7b5d-bf3c227f1755%40gmail.com>
Precedence: bulk

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