Generate pg_stat_get_* functions with Macros

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

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

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 macros will be used in [1]/messages/by-id/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com for both tables and indexes stats (and so we'd have to replace "TAB" with "REL" in [1]/messages/by-id/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com).
Having "REL" already in place reduces the changes that will be needed in [1]/messages/by-id/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com.

[1]: /messages/by-id/f572abe7-a1bb-e13b-48c7-2ca150546822@gmail.com

Regards,

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

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 macros will 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 macros will 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

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 work for [1]https://commitfest.postgresql.org/41/3984/.
Means, once the current patch is committed, working on [1]https://commitfest.postgresql.org/41/3984/ and "cleaning" the databases one could be done in parallel. Sounds good 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

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

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 macro itself, 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 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.

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

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

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: syntax error: 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: syntax error: 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: syntax error: 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

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