Thread: Generate pg_stat_get_* functions with Macros
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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