Horton

Thread

Commits

  1. Generate a few more functions of pgstatfuncs.c with macros

  2. Generate pg_stat_get_xact*() functions for relations using macros

  1. Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-01-05T13:48:39Z

    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

  2. Re: Generate pg_stat_get_xact*() functions with Macros

    Corey Huinker <corey.huinker@gmail.com> — 2023-01-05T20:19:54Z

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

  3. Re: Generate pg_stat_get_xact*() functions with Macros

    Andres Freund <andres@anarazel.de> — 2023-01-05T23:21:40Z

    Hi,

On 2023-01-05 15:19:54 -0500, Corey Huinker wrote:
> 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:

I think that'd make it substantially enough harder to work with the
datastructures that I don't want to go there.


The "more fundamental" approach would be to switch to using a table-returning
function for accessing these stat values. When just accessing a single counter
or two, the current approach avoids the overhead of having to construct a
tuple. But after that the overhead of having to fetch the stats data (i.e. a
hash table lookup, potentially some locking) multiple times takes over.

Unfortunately there's currently no way to dynamically switch between those
behaviours.

Greetings,

Andres Freund

  4. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-01-06T08:40:37Z

    Hi,

On 1/5/23 9:19 PM, Corey Huinker wrote:
> 
> 
> 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.

Thanks for the review!

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  5. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-01-06T08:44:25Z

    Hi,

On 1/6/23 12:21 AM, Andres Freund wrote:
> Hi,
> 
> On 2023-01-05 15:19:54 -0500, Corey Huinker wrote:
>> 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:
> 
> I think that'd make it substantially enough harder to work with the
> datastructures that I don't want to go there.
> 

Yeah, I think that's a good idea from a "coding style" point of view but harder to work with.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  6. Re: Generate pg_stat_get_xact*() functions with Macros

    Andres Freund <andres@anarazel.de> — 2023-01-11T22:59:07Z

    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

  7. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-01-12T07:38:57Z

    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

  8. Re: Generate pg_stat_get_xact*() functions with Macros

    Andres Freund <andres@anarazel.de> — 2023-01-12T18:24:34Z

    Hi,

On 2023-01-12 08:38:57 +0100, Drouvot, Bertrand wrote:
> On 1/11/23 11:59 PM, Andres Freund wrote:
> > > 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'm not super happy about that patch tbh.


> > Probably should remove PgStat_BackendFunctionEntry.
> 
> I think that would be a 3rd patch, agree?

Yep.



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

+1



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

Yea. I am of multiple minds about the ordering. I can see benefits on fixing
the architectural issue before reducing duplication in the accessor with a
macro. The reason is that if we addressed the architectural issue, the
difference between the xact and non-xact version will be very minimal, and
could even be generated by the same macro.

Greetings,

Andres Freund

  9. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-01-13T09:36:49Z

    Hi,

On 1/12/23 7:24 PM, Andres Freund wrote:
> Hi,
> 
> On 2023-01-12 08:38:57 +0100, Drouvot, Bertrand wrote:
>> On 1/11/23 11:59 PM, Andres Freund wrote:
>>>> 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'm not super happy about that patch tbh.
> 
> 
>>> Probably should remove PgStat_BackendFunctionEntry.
>>
>> I think that would be a 3rd patch, agree?
> 
> Yep.
> 
> 
> 
>>> 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).
> 
> +1
> 
> 
> 
>>> 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?
> 
> Yea. I am of multiple minds about the ordering. I can see benefits on fixing
> the architectural issue before reducing duplication in the accessor with a
> macro. The reason is that if we addressed the architectural issue, the
> difference between the xact and non-xact version will be very minimal, and
> could even be generated by the same macro.
> 

Yeah, I do agree and I'm in favor of this ordering:

1) replace find_tabstat_entry() with a version returning a fully "reconciled" PgStat_StatTabEntry
2) remove prefixes
3) Introduce the new macros

And it looks to me that removing PgStat_BackendFunctionEntry can be done independently

I'll first look at 1).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  10. Re: Generate pg_stat_get_xact*() functions with Macros

    Andres Freund <andres@anarazel.de> — 2023-01-13T18:37:46Z

    Hi,

On 2023-01-13 10:36:49 +0100, Drouvot, Bertrand wrote:
> > > > 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?
> > 
> > Yea. I am of multiple minds about the ordering. I can see benefits on fixing
> > the architectural issue before reducing duplication in the accessor with a
> > macro. The reason is that if we addressed the architectural issue, the
> > difference between the xact and non-xact version will be very minimal, and
> > could even be generated by the same macro.
> > 
> 
> Yeah, I do agree and I'm in favor of this ordering:
> 
> 1) replace find_tabstat_entry() with a version returning a fully "reconciled" PgStat_StatTabEntry
> 2) remove prefixes
> 3) Introduce the new macros

> I'll first look at 1).

Makes sense.


> And it looks to me that removing PgStat_BackendFunctionEntry can be done independently

It's imo the function version of 1), just a bit simpler to implement due to
the much simpler reconciliation. It could be done together with it, or
separately.

Greetings,

Andres Freund

  11. Re: Generate pg_stat_get_xact*() functions with Macros

    Gregory Stark (as CFM) <stark.cfm@gmail.com> — 2023-03-01T19:54:16Z

    Looks like you have a path forward on this and it's not ready to
commit yet? In which case I'll mark it Waiting on Author?

On Fri, 13 Jan 2023 at 13:38, Andres Freund <andres@anarazel.de> wrote:
>
> On 2023-01-13 10:36:49 +0100, Drouvot, Bertrand wrote:
>
> > I'll first look at 1).
>
> Makes sense.
>
> > And it looks to me that removing PgStat_BackendFunctionEntry can be done independently
>
> It's imo the function version of 1), just a bit simpler to implement due to
> the much simpler reconciliation. It could be done together with it, or
> separately.


-- 
Gregory Stark
As Commitfest Manager

  12. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-02T07:39:14Z

    Hi,

On 3/1/23 8:54 PM, Gregory Stark (as CFM) wrote:
> Looks like you have a path forward on this and it's not ready to
> commit yet? In which case I'll mark it Waiting on Author?
> 

Yeah, there is some dependencies around this one.

[1]: depends on it
Current one depends of [2], [3] and [4]

Waiting on Author is then the right state, thanks for having moved it to that state.

[1]: https://www.postgresql.org/message-id/flat/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com
[2]: https://www.postgresql.org/message-id/flat/b9e1f543-ee93-8168-d530-d961708ad9d3@gmail.com
[3]: https://www.postgresql.org/message-id/flat/11d531fe-52fc-c6ea-7e8e-62f1b6ec626e@gmail.com
[4]: https://www.postgresql.org/message-id/flat/9142f62a-a422-145c-bde0-b5bc498a4ada@gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  13. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-24T00:04:31Z

    On Thu, Mar 02, 2023 at 08:39:14AM +0100, Drouvot, Bertrand wrote:
> Yeah, there is some dependencies around this one.
> 
> [1]: depends on it
> Current one depends of [2], [3] and [4]
> 
> Waiting on Author is then the right state, thanks for having moved it to that state.
> 
> [1]: https://www.postgresql.org/message-id/flat/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com
> [2]: https://www.postgresql.org/message-id/flat/b9e1f543-ee93-8168-d530-d961708ad9d3@gmail.com
> [3]: https://www.postgresql.org/message-id/flat/11d531fe-52fc-c6ea-7e8e-62f1b6ec626e@gmail.com
> [4]: https://www.postgresql.org/message-id/flat/9142f62a-a422-145c-bde0-b5bc498a4ada@gmail.com

[3] and [4] have been applied.  [2] is more sensitive than it looks,
and [1] for the split of index and table stats can feed on the one of
this thread.

Roma wasn't built in one day, and from what I can see you can still do
some progress with the refactoring of pgstatfuncs.c with what's
already on HEAD.  So how about handling doing that first as much as we
can based on the state of HEAD?  That looks like 50~60% (?) of the
original goal to switch pgstatfuncs.c to use more macros to generate
the definition of all these SQL functions.
--
Michael

  14. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-24T05:58:30Z

    Hi,

On 3/24/23 1:04 AM, Michael Paquier wrote:
> On Thu, Mar 02, 2023 at 08:39:14AM +0100, Drouvot, Bertrand wrote:
>> Yeah, there is some dependencies around this one.
>>
>> [1]: depends on it
>> Current one depends of [2], [3] and [4]
>>
>> Waiting on Author is then the right state, thanks for having moved it to that state.
>>
>> [1]: https://www.postgresql.org/message-id/flat/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com
>> [2]: https://www.postgresql.org/message-id/flat/b9e1f543-ee93-8168-d530-d961708ad9d3@gmail.com
>> [3]: https://www.postgresql.org/message-id/flat/11d531fe-52fc-c6ea-7e8e-62f1b6ec626e@gmail.com
>> [4]: https://www.postgresql.org/message-id/flat/9142f62a-a422-145c-bde0-b5bc498a4ada@gmail.com
> 
> [3] and [4] have been applied.

Thanks for your help on this!

>  [2] is more sensitive than it looks,
> and [1] for the split of index and table stats can feed on the one of
> this thread.
> 
> Roma wasn't built in one day, and from what I can see you can still do
> some progress with the refactoring of pgstatfuncs.c with what's
> already on HEAD.  So how about handling doing that first as much as we
> can based on the state of HEAD?  That looks like 50~60% (?) of the
> original goal to switch pgstatfuncs.c to use more macros to generate
> the definition of all these SQL functions.

I think that's a good idea, so please find enclosed V2 which as compare to V1:

- Does not include the refactoring for pg_stat_get_xact_tuples_inserted(), pg_stat_get_xact_tuples_updated()
and pg_stat_get_xact_tuples_deleted() (as they depend of [2] mentioned above)

- Does not include the refactoring for pg_stat_get_xact_function_total_time(), pg_stat_get_xact_function_self_time(),
pg_stat_get_function_total_time() and pg_stat_get_function_self_time(). I think they can be done in a dedicated commit once
we agree on the renaming for PG_STAT_GET_DBENTRY_FLOAT8 (see Andres's comment up-thread) so that the new macros can match the future agreement.

- Does include the refactoring of the new pg_stat_get_xact_tuples_newpage_updated() function (added in ae4fdde135)

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  15. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-25T02:50:50Z

    On Fri, Mar 24, 2023 at 06:58:30AM +0100, Drouvot, Bertrand wrote:
> - Does not include the refactoring for pg_stat_get_xact_tuples_inserted(),
> pg_stat_get_xact_tuples_updated() and pg_stat_get_xact_tuples_deleted() (as
> they depend of [2] mentioned above) 
> 
> - Does not include the refactoring for
> pg_stat_get_xact_function_total_time(),
> pg_stat_get_xact_function_self_time(), 
> pg_stat_get_function_total_time() and
> pg_stat_get_function_self_time(). I think they can be done in a
> dedicated commit once we agree on the renaming for
> PG_STAT_GET_DBENTRY_FLOAT8 (see Andres's comment up-thread) so that
> the new macros can match the future agreement.
> 
> - Does include the refactoring of the new
> - pg_stat_get_xact_tuples_newpage_updated() function (added in
> - ae4fdde135) 

Fine by me.  One step is better than no steps, and this helps:
 1 file changed, 29 insertions(+), 97 deletions(-)

I'll go apply that if there are no objections.
--
Michael

  16. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-27T01:20:34Z

    On Sat, Mar 25, 2023 at 11:50:50AM +0900, Michael Paquier wrote:
> On Fri, Mar 24, 2023 at 06:58:30AM +0100, Drouvot, Bertrand wrote:
>> - Does not include the refactoring for
>> pg_stat_get_xact_function_total_time(),
>> pg_stat_get_xact_function_self_time(), 
>> pg_stat_get_function_total_time() and
>> pg_stat_get_function_self_time(). I think they can be done in a
>> dedicated commit once we agree on the renaming for
>> PG_STAT_GET_DBENTRY_FLOAT8 (see Andres's comment up-thread) so that
>> the new macros can match the future agreement.

Thanks for the reminder.  I have completely missed that this is
mentioned in [1], and that it is all about 8018ffb.  The suggestion to
prefix the macro names with a "_MS" to outline the conversion sounds
like a good one seen from here.  So please find attached a patch to do
this adjustment, completed with a similar switch for the two counters
of the function entries.

>> - Does include the refactoring of the new
>> - pg_stat_get_xact_tuples_newpage_updated() function (added in
>> - ae4fdde135) 
> 
> Fine by me.  One step is better than no steps, and this helps:
>  1 file changed, 29 insertions(+), 97 deletions(-)
> 
> I'll go apply that if there are no objections.

Just did this part to shave a bit more code.

[1]: https://www.postgresql.org/message-id/20230111225907.6el6c5j3hukizqxc@awork3.anarazel.de
--
Michael

  17. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-27T05:45:26Z

    Hi,

On 3/27/23 3:20 AM, Michael Paquier wrote:
> On Sat, Mar 25, 2023 at 11:50:50AM +0900, Michael Paquier wrote:
>> On Fri, Mar 24, 2023 at 06:58:30AM +0100, Drouvot, Bertrand wrote:
>>> - Does not include the refactoring for
>>> pg_stat_get_xact_function_total_time(),
>>> pg_stat_get_xact_function_self_time(),
>>> pg_stat_get_function_total_time() and
>>> pg_stat_get_function_self_time(). I think they can be done in a
>>> dedicated commit once we agree on the renaming for
>>> PG_STAT_GET_DBENTRY_FLOAT8 (see Andres's comment up-thread) so that
>>> the new macros can match the future agreement.
> 
> Thanks for the reminder.  I have completely missed that this is
> mentioned in [1], and that it is all about 8018ffb.  The suggestion to
> prefix the macro names with a "_MS" to outline the conversion sounds
> like a good one seen from here.  So please find attached a patch to do
> this adjustment, completed with a similar switch for the two counters
> of the function entries.
> 

Thanks! LGTM, but what about also taking care of pg_stat_get_xact_function_total_time()
and pg_stat_get_xact_function_self_time() while at it?

>>> - Does include the refactoring of the new
>>> - pg_stat_get_xact_tuples_newpage_updated() function (added in
>>> - ae4fdde135)
>>
>> Fine by me.  One step is better than no steps, and this helps:
>>   1 file changed, 29 insertions(+), 97 deletions(-)
>>
>> I'll go apply that if there are no objections.
> 
> Just did this part to shave a bit more code.

Thanks!

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  18. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-27T06:40:31Z

    On Mon, Mar 27, 2023 at 07:45:26AM +0200, Drouvot, Bertrand wrote:
> Thanks! LGTM, but what about also taking care of pg_stat_get_xact_function_total_time()
> and pg_stat_get_xact_function_self_time() while at it?

With a macro that uses INSTR_TIME_GET_MILLISEC() to cope with
instr_time?  Why not, that's one duplication less.
--
Michael

  19. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-27T06:54:13Z

    On 3/27/23 8:40 AM, Michael Paquier wrote:
> On Mon, Mar 27, 2023 at 07:45:26AM +0200, Drouvot, Bertrand wrote:
>> Thanks! LGTM, but what about also taking care of pg_stat_get_xact_function_total_time()
>> and pg_stat_get_xact_function_self_time() while at it?
> 
> With a macro that uses INSTR_TIME_GET_MILLISEC() to cope with
> instr_time?  Why not, that's one duplication less.

Yes, something like V1 up-thread was doing. I think it can be added with your current proposal.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  20. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-27T07:23:38Z

    On Mon, Mar 27, 2023 at 08:54:13AM +0200, Drouvot, Bertrand wrote:
> Yes, something like V1 up-thread was doing. I think it can be added with your current proposal.

Sure, I can write that.  Or perhaps you'd prefer write something
yourself?
--
Michael

  21. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-27T08:11:21Z

    
On 3/27/23 9:23 AM, Michael Paquier wrote:
> On Mon, Mar 27, 2023 at 08:54:13AM +0200, Drouvot, Bertrand wrote:
>> Yes, something like V1 up-thread was doing. I think it can be added with your current proposal.
> 
> Sure, I can write that.  Or perhaps you'd prefer write something
> yourself?

Please find attached V2 adding pg_stat_get_xact_function_total_time()
and pg_stat_get_xact_function_self_time() to the party.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  22. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-27T10:08:51Z

    On Mon, Mar 27, 2023 at 10:11:21AM +0200, Drouvot, Bertrand wrote:
> Please find attached V2 adding pg_stat_get_xact_function_total_time()
> and pg_stat_get_xact_function_self_time() to the party.

The patch has one mistake: PG_STAT_GET_XACT_FUNCENTRY_FLOAT8_MS does
not need a slash on its last line or it would include the next, empty
line.  This could lead to mistakes (no need to send a new patch just
for that).
--
Michael

  23. Re: Generate pg_stat_get_xact*() functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2023-03-27T22:41:48Z

    On Mon, Mar 27, 2023 at 07:08:51PM +0900, Michael Paquier wrote:
> The patch has one mistake: PG_STAT_GET_XACT_FUNCENTRY_FLOAT8_MS does
> not need a slash on its last line or it would include the next, empty
> line.  This could lead to mistakes (no need to send a new patch just
> for that).

Adjusted that, and the rest was fine after a second look, so applied.
It looks like we are done for now with this thread, so I have marked
it as committed in the CF app.
--
Michael

  24. Re: Generate pg_stat_get_xact*() functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-28T05:19:53Z

    
On 3/28/23 12:41 AM, Michael Paquier wrote:
> On Mon, Mar 27, 2023 at 07:08:51PM +0900, Michael Paquier wrote:
>> The patch has one mistake: PG_STAT_GET_XACT_FUNCENTRY_FLOAT8_MS does
>> not need a slash on its last line or it would include the next, empty
>> line.  This could lead to mistakes (no need to send a new patch just
>> for that).
> 
> Adjusted that, and the rest was fine after a second look, so applied.
> It looks like we are done for now with this thread, so I have marked
> it as committed in the CF app.

Thanks for having corrected the mistake and applied the patch!

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com