On Thu, Jan 5, 2023 at 8:50 AM Drouvot, Bertrand < bertranddrouvot...@gmail.com> wrote:
> Hi hackers, > > Please find attached a patch proposal to $SUBJECT. > > This is the same kind of work that has been done in 83a1a1b566 and > 8018ffbf58 but this time for the > pg_stat_get_xact*() functions (as suggested by Andres in [1]). > > The function names remain the same, but some fields have to be renamed. > > 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). > > Now that this patch renames some fields, 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 > > But I think it would be better to do it in a follow-up patch (once this > one get committed). > > [1]: > https://www.postgresql.org/message-id/20230105002733.ealhzubjaiqis6ua%40awork3.anarazel.de > > Looking forward to your feedback, > > Regards, > > -- > Bertrand Drouvot > PostgreSQL Contributors Team > RDS Open Source Databases > Amazon Web Services: https://aws.amazon.com I like code cleanups like this. It makes sense, it results in less code, and anyone doing a `git grep pg_stat_get_live_tuples` will quickly find the macro definition. Unsurprisingly, it passes `make check-world`. So I think it's good to go as-is. It does get me wondering, however, if we reordered the three typedefs to group like-typed registers together, we could make them an array with the names becoming defined constant index values (or keeping them via a union), then the typedefs effectively become: typedef struct PgStat_FunctionCallUsage { PgStat_FunctionCounts *fs; instr_time time_counters[3]; } PgStat_FunctionCallUsage; typedef struct PgStat_BackendSubEntry { PgStat_Counter counters[2]; } PgStat_BackendSubEntry; typedef struct PgStat_TableCounts { bool t_truncdropped; PgStat_Counter counters[12]; } PgStat_TableCounts; Then we'd only have 3 actual C functions: pg_stat_get_xact_counter(oid, int) pg_stat_get_xact_subtrans_counter(oid, int) pg_stat_get_xact_function_time_counter(oid, int) and then the existing functions become SQL standard function body calls, something like this: CREATE OR REPLACE FUNCTION pg_stat_get_xact_numscans(oid) RETURNS bigint LANGUAGE sql STABLE PARALLEL RESTRICTED COST 1 RETURN pg_stat_get_xact_counter($1, 0); CREATE OR REPLACE FUNCTION pg_stat_get_xact_tuples_returned(oid) RETURNS bigint LANGUAGE sql STABLE PARALLEL RESTRICTED COST 1 RETURN pg_stat_get_xact_counter($1, 1); The most obvious drawback to this approach is that the C functions would need to do runtime bounds checking on the index parameter, and the amount of memory footprint saved by going from 17 short functions to 3 is not enough to make any real difference. So I think your approach is better, but I wanted to throw this idea out there.
