Thread: Generate pg_stat_get_* functions with Macros

Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi hackers,

Please find attached a patch proposal to $SUBJECT.

The idea has been proposed by Andres in [1] and can be seen as preparatory work for [1].

The patch introduces 2 new Macros, PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR and PGSTAT_DEFINE_REL_TSTZ_FIELD_ACCESSOR.

For some functions (namely pg_stat_get_ins_since_vacuum(), pg_stat_get_dead_tuples(), pg_stat_get_mod_since_analyze(),
pg_stat_get_live_tuples(), pg_stat_get_last_autovacuum_time(), pg_stat_get_autovacuum_count(),
pg_stat_get_last_vacuum_time(),
pg_stat_get_last_autoanalyze_time(), pg_stat_get_autoanalyze_count() and pg_stat_get_last_analyze_time()), I had to
choosebetween renaming the function and the counter.
 

I took the later option to avoid changing the linked views, tests....

This patch is also a step forward to "cleaning" the metrics/fields/functions naming (means having them match).

[1]: https://www.postgresql.org/message-id/flat/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com

Looking forward to your feedback,

Regards,

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

Re: Generate pg_stat_get_* functions with Macros

From
Nathan Bossart
Date:
Overall, this change looks straightforward, and it saves a couple hundred
lines.

On Tue, Nov 22, 2022 at 08:09:22AM +0100, Drouvot, Bertrand wrote:
> +/* pg_stat_get_numscans */
> +PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR(pg_stat_get_, numscans);
> +
> +/* pg_stat_get_tuples_returned */
> +PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR(pg_stat_get_, tuples_returned);
> +
> +/* pg_stat_get_tuples_fetched */
> +PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR(pg_stat_get_, tuples_fetched);

Can we hard-code the prefix in the macro?  It looks like all of these use
the same one.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com



Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/3/22 1:51 AM, Nathan Bossart wrote:
> Overall, this change looks straightforward, and it saves a couple hundred
> lines.
> 

Thanks for looking at it!

> On Tue, Nov 22, 2022 at 08:09:22AM +0100, Drouvot, Bertrand wrote:
>> +/* pg_stat_get_numscans */
>> +PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR(pg_stat_get_, numscans);
>> +
>> +/* pg_stat_get_tuples_returned */
>> +PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR(pg_stat_get_, tuples_returned);
>> +
>> +/* pg_stat_get_tuples_fetched */
>> +PGSTAT_DEFINE_REL_INT64_FIELD_ACCESSOR(pg_stat_get_, tuples_fetched);
> 
> Can we hard-code the prefix in the macro?  It looks like all of these use
> the same one.
> 

Good point! Done in V2 attached.

Regards,

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

Re: Generate pg_stat_get_* functions with Macros

From
Nathan Bossart
Date:
On Sat, Dec 03, 2022 at 10:31:19AM +0100, Drouvot, Bertrand wrote:
> On 12/3/22 1:51 AM, Nathan Bossart wrote:
>> Can we hard-code the prefix in the macro?  It looks like all of these use
>> the same one.
> 
> Good point! Done in V2 attached.

Thanks.  I editorialized a bit in the attached v3.  I'm not sure that my
proposed names for the macros are actually an improvement.  WDYT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/3/22 9:16 PM, Nathan Bossart wrote:
> On Sat, Dec 03, 2022 at 10:31:19AM +0100, Drouvot, Bertrand wrote:
>> On 12/3/22 1:51 AM, Nathan Bossart wrote:
>>> Can we hard-code the prefix in the macro?  It looks like all of these use
>>> the same one.
>>
>> Good point! Done in V2 attached.
> 
> Thanks.  I editorialized a bit in the attached v3.  I'm not sure that my
> proposed names for the macros are actually an improvement.  WDYT?
> 

Thanks! I do prefer the macros definition ordering that you're proposing (that makes pgstatfuncs.c "easier" to read).

As far the names, I think it's better to replace "TAB" with "REL" (like in v4 attached): the reason is that those
macroswill be used in [1] for both tables and indexes stats (and so we'd have to replace "TAB" with "REL" in [1]).
 
Having "REL" already in place reduces the changes that will be needed in [1].

[1]: https://www.postgresql.org/message-id/flat/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com

Regards,

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

Re: Generate pg_stat_get_* functions with Macros

From
Nathan Bossart
Date:
On Sun, Dec 04, 2022 at 06:07:37AM +0100, Drouvot, Bertrand wrote:
> On 12/3/22 9:16 PM, Nathan Bossart wrote:
>> Thanks.  I editorialized a bit in the attached v3.  I'm not sure that my
>> proposed names for the macros are actually an improvement.  WDYT?
> 
> Thanks! I do prefer the macros definition ordering that you're proposing (that makes pgstatfuncs.c "easier" to
read).
> 
> As far the names, I think it's better to replace "TAB" with "REL" (like in v4 attached): the reason is that those
macroswill be used in [1] for both tables and indexes stats (and so we'd have to replace "TAB" with "REL" in [1]).
 
> Having "REL" already in place reduces the changes that will be needed in [1].

Alright.  I marked this as ready-for-committer.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com



Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/4/22 6:32 PM, Nathan Bossart wrote:
> On Sun, Dec 04, 2022 at 06:07:37AM +0100, Drouvot, Bertrand wrote:
>> On 12/3/22 9:16 PM, Nathan Bossart wrote:
>>> Thanks.  I editorialized a bit in the attached v3.  I'm not sure that my
>>> proposed names for the macros are actually an improvement.  WDYT?
>>
>> Thanks! I do prefer the macros definition ordering that you're proposing (that makes pgstatfuncs.c "easier" to
read).
>>
>> As far the names, I think it's better to replace "TAB" with "REL" (like in v4 attached): the reason is that those
macroswill be used in [1] for both tables and indexes stats (and so we'd have to replace "TAB" with "REL" in [1]).
 
>> Having "REL" already in place reduces the changes that will be needed in [1].
> 
> Alright.  I marked this as ready-for-committer.
> 

Thanks!

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



Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Mon, Dec 05, 2022 at 08:27:15AM +0100, Drouvot, Bertrand wrote:
> On 12/4/22 6:32 PM, Nathan Bossart wrote:
>> Alright.  I marked this as ready-for-committer.
>
> Thanks!

Well, that's kind of nice:
 5 files changed, 139 insertions(+), 396 deletions(-)
And I like removing code, so..

In the same area, I am counting a total of 21 (?) pgstat routines for
databases that rely on pgstat_fetch_stat_dbentry() while returning an
int64.  This would lead to more cleanup.
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/5/22 8:44 AM, Michael Paquier wrote:
> On Mon, Dec 05, 2022 at 08:27:15AM +0100, Drouvot, Bertrand wrote:
>> On 12/4/22 6:32 PM, Nathan Bossart wrote:
>>> Alright.  I marked this as ready-for-committer.
>>
>> Thanks!
> 
> Well, that's kind of nice:
>   5 files changed, 139 insertions(+), 396 deletions(-)
> And I like removing code, so..
> 

Thanks for looking at it!

> In the same area, I am counting a total of 21 (?) pgstat routines for
> databases that rely on pgstat_fetch_stat_dbentry() while returning an
> int64.  This would lead to more cleanup.
> --


Yeah, good point, thanks!

I'll look at the "databases" ones but I think in a separate patch. The reason is that the current one is preparatory
workfor [1].
 
Means, once the current patch is committed, working on [1] and "cleaning" the databases one could be done in parallel.
Soundsgood to you?
 


[1]: https://commitfest.postgresql.org/41/3984/

Regards,

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



Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Mon, Dec 05, 2022 at 09:11:43AM +0100, Drouvot, Bertrand wrote:
> Means, once the current patch is committed, working on [1] and
> "cleaning" the databases one could be done in parallel. Sounds good
> to you?

Doing that in a separate patch is fine by me.
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Mon, Dec 05, 2022 at 05:16:56PM +0900, Michael Paquier wrote:
> Doing that in a separate patch is fine by me.

I have applied the patch for the tab entries, then could not resist
poking at the parts for the db entries.  This leads to more reduction
than the other one actually, as of:
 4 files changed, 169 insertions(+), 447 deletions(-)

Like the previous one, the functions have the same names and the field
names are updated to fit in the picture.  Thoughts?
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
Nathan Bossart
Date:
On Tue, Dec 06, 2022 at 11:45:10AM +0900, Michael Paquier wrote:
> I have applied the patch for the tab entries, then could not resist
> poking at the parts for the db entries.  This leads to more reduction
> than the other one actually, as of:
>  4 files changed, 169 insertions(+), 447 deletions(-)
> 
> Like the previous one, the functions have the same names and the field
> names are updated to fit in the picture.  Thoughts?

I might alphabetize the functions, but otherwise it looks good to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com



Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/6/22 3:45 AM, Michael Paquier wrote:
> On Mon, Dec 05, 2022 at 05:16:56PM +0900, Michael Paquier wrote:
>> Doing that in a separate patch is fine by me.
> 
> I have applied the patch for the tab entries, then could not resist
> poking at the parts for the db entries.  This leads to more reduction
> than the other one actually, as of:
>   4 files changed, 169 insertions(+), 447 deletions(-)
> 
> Like the previous one, the functions have the same names and the field
> names are updated to fit in the picture.  Thoughts?

Thanks! For this one (the INT64 case) the fields renaming are not strictly mandatory as we could add the "n_" in the
macroitself, something like:
 

+#define PG_STAT_GET_DBENTRY_INT64(stat)                                                        \
+Datum
\
+CppConcat(pg_stat_get_db_,stat)(PG_FUNCTION_ARGS)                              \
+{
       \
 
+       Oid                     dbid = PG_GETARG_OID(0);                                                \
+       int64           result;                                                                                 \
+       PgStat_StatDBEntry *dbentry;                                                            \
+
       \
 
+       if ((dbentry = pgstat_fetch_stat_dbentry(dbid)) == NULL)        \
+               result = 0;
\
+       else
\
+               result = (int64) (dbentry->CppConcat(n_,stat)); \
+
       \
 
+       PG_RETURN_INT64(result);                                                                        \
+}

Fields renaming was mandatory in the previous ones as there was already a mix of with/without "n_" in the existing
fieldsnames.
 

That said, I think it's better to rename the fields as you did (to be "consistent" on the naming between relation/db
stats),so the patch LGTM.
 

Regards,

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



Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/6/22 3:45 AM, Michael Paquier wrote:
> On Mon, Dec 05, 2022 at 05:16:56PM +0900, Michael Paquier wrote:
>> Doing that in a separate patch is fine by me.
> 
> I have applied the patch for the tab entries,

Oops, I missed this part when reading the email the first time and just saw the patch has been committed.

So, thanks for having applied the patch!

Regards,

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



Re: Generate pg_stat_get_* functions with Macros

From
Bharath Rupireddy
Date:
On Tue, Dec 6, 2022 at 8:15 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Dec 05, 2022 at 05:16:56PM +0900, Michael Paquier wrote:
> > Doing that in a separate patch is fine by me.
>
> I have applied the patch for the tab entries, then could not resist
> poking at the parts for the db entries.  This leads to more reduction
> than the other one actually, as of:
>  4 files changed, 169 insertions(+), 447 deletions(-)
>
> Like the previous one, the functions have the same names and the field
> names are updated to fit in the picture.  Thoughts?

Likewise, is there a plan to add function generation macros for
pg_stat_get_bgwriter, pg_stat_get_xact and so on?

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



Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Tue, Dec 06, 2022 at 12:23:55PM +0530, Bharath Rupireddy wrote:
> Likewise, is there a plan to add function generation macros for
> pg_stat_get_bgwriter, pg_stat_get_xact and so on?

Yes, I saw that and we could do it, but I did not get as much
enthusiastic in terms of code reduction.
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Tue, Dec 06, 2022 at 05:28:47AM +0100, Drouvot, Bertrand wrote:
> Fields renaming was mandatory in the previous ones as there was
> already a mix of with/without "n_" in the existing fields names.
>
> That said, I think it's better to rename the fields as you did (to
> be "consistent" on the naming between relation/db stats), so the
> patch LGTM.

Yeah, PgStat_StatDBEntry is the last one using this style, so I have
kept my change with the variables renamed rather than painting more
CppConcat()s.  The functions are still named the same as the original
ones.
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
Tom Lane
Date:
This series of patches has caused buildfarm member wrasse to
start complaining about "empty declarations":

 wrasse        | 2022-12-09 21:08:33 |
"/export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/backend/utils/adt/pgstatfuncs.c",line 56: warning:
syntaxerror:  empty declaration 
 wrasse        | 2022-12-09 21:08:33 |
"/export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/backend/utils/adt/pgstatfuncs.c",line 59: warning:
syntaxerror:  empty declaration 
 wrasse        | 2022-12-09 21:08:33 |
"/export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/backend/utils/adt/pgstatfuncs.c",line 62: warning:
syntaxerror:  empty declaration 

[ etc etc ]

Presumably it could be silenced by removing the semicolons after
the new macro calls:

/* pg_stat_get_analyze_count */
PG_STAT_GET_RELENTRY_INT64(analyze_count);

/* pg_stat_get_autoanalyze_count */
PG_STAT_GET_RELENTRY_INT64(autoanalyze_count);

/* pg_stat_get_autovacuum_count */
PG_STAT_GET_RELENTRY_INT64(autovacuum_count);

I wondered if that would confuse pgindent, but a quick check
says no.  (The blank lines in between may be helping.)

While I'm nitpicking, I think that the way you've set up the
macro definitions is a bit dangerous:

#define PG_STAT_GET_RELENTRY_INT64(stat)                        \
Datum                                                           \
CppConcat(pg_stat_get_,stat)(PG_FUNCTION_ARGS)                  \
{                                                               \
...                                                             \
    PG_RETURN_INT64(result);                                    \
}                                                               \

The backslash after the last right brace means that the line
following that is part of the macro body.  This does no harm as
long as said line is blank ... but I think it's a foot-gun
waiting to bite somebody, because visually you'd think the macro
ends with the brace.  So I'd leave off that last backslash.

            regards, tom lane



Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Fri, Dec 09, 2022 at 09:43:56PM -0500, Tom Lane wrote:
> Presumably it could be silenced by removing the semicolons after
> the new macro calls:
>
> /* pg_stat_get_analyze_count */
> PG_STAT_GET_RELENTRY_INT64(analyze_count);
>
> /* pg_stat_get_autoanalyze_count */
> PG_STAT_GET_RELENTRY_INT64(autoanalyze_count);
>
> /* pg_stat_get_autovacuum_count */
> PG_STAT_GET_RELENTRY_INT64(autovacuum_count);
>
> I wondered if that would confuse pgindent, but a quick check
> says no.  (The blank lines in between may be helping.)

Indeed.  Will fix.

> The backslash after the last right brace means that the line
> following that is part of the macro body.  This does no harm as
> long as said line is blank ... but I think it's a foot-gun
> waiting to bite somebody, because visually you'd think the macro
> ends with the brace.  So I'd leave off that last backslash.

Will address this one as well for all the macro definitions.  Thanks!
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
Nathan Bossart
Date:
On Fri, Dec 09, 2022 at 09:43:56PM -0500, Tom Lane wrote:
> Presumably it could be silenced by removing the semicolons after
> the new macro calls:

> The backslash after the last right brace means that the line
> following that is part of the macro body.  This does no harm as
> long as said line is blank ... but I think it's a foot-gun
> waiting to bite somebody, because visually you'd think the macro
> ends with the brace.  So I'd leave off that last backslash.

Indeed.  Patch attached.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
Michael Paquier
Date:
On Fri, Dec 09, 2022 at 07:55:45PM -0800, Nathan Bossart wrote:
> Indeed.  Patch attached.

Yep, thanks.  I have exactly the same thing brewing in one of my
staging branches.
--
Michael

Attachment

Re: Generate pg_stat_get_* functions with Macros

From
"Drouvot, Bertrand"
Date:
Hi,

On 12/10/22 4:55 AM, Nathan Bossart wrote:
> On Fri, Dec 09, 2022 at 09:43:56PM -0500, Tom Lane wrote:
>> Presumably it could be silenced by removing the semicolons after
>> the new macro calls:
> 
>> The backslash after the last right brace means that the line
>> following that is part of the macro body.  This does no harm as
>> long as said line is blank ... but I think it's a foot-gun
>> waiting to bite somebody, because visually you'd think the macro
>> ends with the brace.  So I'd leave off that last backslash.
> 
> Indeed.  Patch attached.
> 

Oh right. Thanks Tom for the explanations and Nathan/Michael for the fix.

Regards,

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