Обсуждение: Generate pg_stat_get_xact*() functions with Macros

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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