Re: Generate pg_stat_get_xact*() functions with Macros

On Thu, Jan 5, 2023 at 8:50 AM Drouvot, Bertrand <
bertranddrouvot(dot)pg(at)gmail(dot)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.