Horton

Thread

Commits

  1. Fix macro definitions in pgstatfuncs.c

  2. Generate pg_stat_get*() functions for databases using macros

  1. Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-11-22T07:09:22Z

    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 choose between 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

  2. Re: Generate pg_stat_get_* functions with Macros

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-03T00:51:25Z

    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

  3. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-03T09:31:19Z

    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

  4. Re: Generate pg_stat_get_* functions with Macros

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-03T20:16:17Z

    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

  5. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-04T05:07:37Z

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

  6. Re: Generate pg_stat_get_* functions with Macros

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-04T17:32:07Z

    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

  7. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-05T07:27:15Z

    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

  8. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-05T07:44:13Z

    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

  9. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-05T08:11:43Z

    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].
Means, once the current patch is committed, working on [1] 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

  10. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-05T08:16:56Z

    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

  11. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-06T02:45:10Z

    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

  12. Re: Generate pg_stat_get_* functions with Macros

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-06T03:54:45Z

    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

  13. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-06T04:28:47Z

    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

  14. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-06T05:04:58Z

    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

  15. Re: Generate pg_stat_get_* functions with Macros

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-12-06T06:53:55Z

    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

  16. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-06T08:01:22Z

    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

  17. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-07T00:16:14Z

    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

  18. Re: Generate pg_stat_get_* functions with Macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-10T02:43:56Z

    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

  19. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-10T03:52:16Z

    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

  20. Re: Generate pg_stat_get_* functions with Macros

    Nathan Bossart <nathandbossart@gmail.com> — 2022-12-10T03:55:45Z

    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

  21. Re: Generate pg_stat_get_* functions with Macros

    Michael Paquier <michael@paquier.xyz> — 2022-12-10T04:07:29Z

    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

  22. Re: Generate pg_stat_get_* functions with Macros

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2022-12-10T08:06:06Z

    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