Horton

Thread

Commits

  1. Rename fields in pgstat structures for functions and relations

  1. Remove nonmeaningful prefixes in PgStat_* fields

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-01-12T09:47:11Z

    Hi hackers,

Please find attached a patch to $SUBJECT.

It is a preliminary patch for [1].

The main ideas are: 1) to have consistent naming between the pg_stat_get*() functions
and their associated counters and 2) to define the new macros in [1] the same way as it
has been done in 8018ffbf58 (aka not using the prefixes in the macros).

Looking forward to your feedback,

Regards,

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

[1]: https://www.postgresql.org/message-id/flat/89606d96-cd94-af74-18f3-c7ab2b684ba2@gmail.com

  2. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-01-12T17:12:52Z

    On 2023-Jan-12, Drouvot, Bertrand wrote:

> Please find attached a patch to $SUBJECT.
> 
> It is a preliminary patch for [1].
> 
> The main ideas are: 1) to have consistent naming between the pg_stat_get*() functions
> and their associated counters and 2) to define the new macros in [1] the same way as it
> has been done in 8018ffbf58 (aka not using the prefixes in the macros).

I don't like this at all.  With these prefixes in place, it's much more
likely that you'll be able to grep the whole source tree and not run
into tons of false positives.  If you remove them, that tends to be not
very workable.  If we use these commits as precedent for expanding this
sort of renaming across the tree, we'll soon end up with a very
grep-unfriendly code base.

The PGSTAT_ACCUM_DBCOUNT and PG_STAT_GET_DBENTRY macros are just one
argument away from being able to generate the variable name including
the prefix, anyway.  I don't know why we had to rename everything in
order to do 8018ffbf5895, and if I had my druthers, we'd undo that.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

  3. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-12T17:23:29Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I don't like this at all.  With these prefixes in place, it's much more
> likely that you'll be able to grep the whole source tree and not run
> into tons of false positives.  If you remove them, that tends to be not
> very workable.  If we use these commits as precedent for expanding this
> sort of renaming across the tree, we'll soon end up with a very
> grep-unfriendly code base.

+1, that was my immediate fear as well.

			regards, tom lane

  4. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Andres Freund <andres@anarazel.de> — 2023-01-12T18:07:33Z

    Hi,

On 2023-01-12 18:12:52 +0100, Alvaro Herrera wrote:
> On 2023-Jan-12, Drouvot, Bertrand wrote:
>
> > Please find attached a patch to $SUBJECT.
> >
> > It is a preliminary patch for [1].
> >
> > The main ideas are: 1) to have consistent naming between the pg_stat_get*() functions
> > and their associated counters and 2) to define the new macros in [1] the same way as it
> > has been done in 8018ffbf58 (aka not using the prefixes in the macros).
>
> I don't like this at all.  With these prefixes in place, it's much more
> likely that you'll be able to grep the whole source tree and not run
> into tons of false positives.  If you remove them, that tends to be not
> very workable.  If we use these commits as precedent for expanding this
> sort of renaming across the tree, we'll soon end up with a very
> grep-unfriendly code base.

The problem with that is that the prefixes are used completely inconsistently
- and have been for a long time. And not just between the different type of
stats. Compare e.g. PgStat_TableCounts with PgStat_TableXactStatus and
PgStat_StatTabEntry. Whereas PgStat_FunctionCounts and PgStat_StatFuncEntry
both use it.  Right now there's no way to remember where to add the t_ prefix,
and where not.

Imo the reason to rename here isn't to abolish prefixes, it's to be halfway
consistent within closeby code. And the code overwhelmingly doesn't use the
prefixes.

Greetings,

Andres Freund

  5. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Michael Paquier <michael@paquier.xyz> — 2023-03-20T07:32:14Z

    On Thu, Jan 12, 2023 at 10:07:33AM -0800, Andres Freund wrote:
> The problem with that is that the prefixes are used completely inconsistently
> - and have been for a long time. And not just between the different type of
> stats. Compare e.g. PgStat_TableCounts with PgStat_TableXactStatus and
> PgStat_StatTabEntry. Whereas PgStat_FunctionCounts and PgStat_StatFuncEntry
> both use it.  Right now there's no way to remember where to add the t_ prefix,
> and where not.
>
> Imo the reason to rename here isn't to abolish prefixes, it's to be halfway
> consistent within closeby code. And the code overwhelmingly doesn't use the
> prefixes.

Reading through the patch, two things are done, basically:
- Remove the prefix "t_" from the fields related to table stats.
- Remove the prefix "f_" from the fields related to function stats.

And FWIW, with my recent lookups at the pgstat code, I'd like to think
that removing the prefixes is actually an improvement in consistency.
It will help in refactoring this code to use more macros, reducing its
size, as well.

So, the code paths where the structures are updated are pretty short
so you know to what they refer to.  And that's even more OK because
now the objects are split into their own files, so you know what you
are dealing with even if the individual variable names are more
common.  That's for pgstat_relation.c and pgstat_function.c, first.
The second part of the changes involves pgstatfuncs.c, where all the
objects are grouped in a single file.  We don't lose any information
here, either, as the code updated deals with a "tabentry" or
"funcentry".  There is a small part in pgstat.h where a few macros
have their fields renamed, where we manipulate a "rel", so that looks
rather clear to me what we are dealing with, IMO.

        /* Total time previously charged to function, as of function start */
-       instr_time      save_f_total_time;
+       instr_time      save_total_time;
I have something to say about this one, though..  I find this change a
bit confusing.  It may be better kept as it is, or I think that we'd
better rename also "save_total" and "start" to reflect in a better way
what they do, because removing "f_" reduces the meaning of the field
with the two others in the same structure.
--
Michael

  6. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-20T09:05:21Z

    Hi,

On 3/20/23 8:32 AM, Michael Paquier wrote:
> 
>          /* Total time previously charged to function, as of function start */
> -       instr_time      save_f_total_time;
> +       instr_time      save_total_time;
> I have something to say about this one, though..  I find this change a
> bit confusing.  It may be better kept as it is, or I think that we'd
> better rename also "save_total" and "start" to reflect in a better way
> what they do, because removing "f_" reduces the meaning of the field
> with the two others in the same structure.

Thanks for looking at it!

Good point and keeping it as it is currently would not
affect the work that is/will be done in [1].

So, please find attached V2 attached taking this comment into account.

[1]: https://www.postgresql.org/message-id/flat/89606d96-cd94-af74-18f3-c7ab2b684ba2%40gmail.com

Regards,

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

  7. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Michael Paquier <michael@paquier.xyz> — 2023-03-22T01:28:19Z

    On Mon, Mar 20, 2023 at 10:05:21AM +0100, Drouvot, Bertrand wrote:
> Good point and keeping it as it is currently would not
> affect the work that is/will be done in [1].

I guess that I'm OK with that to get more of pgstatfuncs.c to use
macros for the function definitions there..  Alvaro, Tom, perhaps you
still think that this is unadapted?  Based on the file split and the
references to funcentry and tabentry, I think that's OK, but that
stands just as one opinion among many..

> So, please find attached V2 attached taking this comment into account. 
> [1]: https://www.postgresql.org/message-id/flat/89606d96-cd94-af74-18f3-c7ab2b684ba2%40gmail.com

Nice.  I am pretty sure that finishing some of that is doable by the
end of this CF to reduce the size of pgstatfuncs.c overall.
--
Michael

  8. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Melanie Plageman <melanieplageman@gmail.com> — 2023-03-22T18:52:23Z

    On Mon, Mar 20, 2023 at 10:05:21AM +0100, Drouvot, Bertrand wrote:
> Hi,
> 
> On 3/20/23 8:32 AM, Michael Paquier wrote:
> > 
> >          /* Total time previously charged to function, as of function start */
> > -       instr_time      save_f_total_time;
> > +       instr_time      save_total_time;
> > I have something to say about this one, though..  I find this change a
> > bit confusing.  It may be better kept as it is, or I think that we'd
> > better rename also "save_total" and "start" to reflect in a better way
> > what they do, because removing "f_" reduces the meaning of the field
> > with the two others in the same structure.
> 
> Thanks for looking at it!
> 
> Good point and keeping it as it is currently would not
> affect the work that is/will be done in [1].
> 
> So, please find attached V2 attached taking this comment into account.

> diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
> index 35c6d46555..4f21fb2dc2 100644
> --- a/src/backend/utils/adt/pgstatfuncs.c
> +++ b/src/backend/utils/adt/pgstatfuncs.c
> @@ -1552,7 +1552,7 @@ pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
>  		result = 0;
>  	else
>  	{
> -		result = tabentry->t_counts.t_tuples_inserted;
> +		result = tabentry->counts.tuples_inserted;

This comment still has the t_ prefix as does the one for tuples_updated
and deleted.

otherwise, LGTM.

>  		/* live subtransactions' counts aren't in t_tuples_inserted yet */
>  		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
>  			result += trans->tuples_inserted;
> @@ -1573,7 +1573,7 @@ pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
>  		result = 0;
>  	else
>  	{
> -		result = tabentry->t_counts.t_tuples_updated;
> +		result = tabentry->counts.tuples_updated;
>  		/* live subtransactions' counts aren't in t_tuples_updated yet */
>  		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
>  			result += trans->tuples_updated;
> @@ -1594,7 +1594,7 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
>  		result = 0;
>  	else
>  	{
> -		result = tabentry->t_counts.t_tuples_deleted;
> +		result = tabentry->counts.tuples_deleted;
>  		/* live subtransactions' counts aren't in t_tuples_deleted yet */
>  		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
>  			result += trans->tuples_deleted;
> @@ -1613,7 +1613,7 @@ pg_stat_get_xact_tuples_hot_updated(PG_FUNCTION_ARGS)
>  	if ((tabentry = find_tabstat_entry(relid)) == NULL)
>  		result = 0;
>  	else
> -		result = (int64) (tabentry->t_counts.t_tuples_hot_updated);
> +		result = (int64) (tabentry->counts.tuples_hot_updated);
>  
>  	PG_RETURN_INT64(result);
>  }

- Melanie

  9. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Michael Paquier <michael@paquier.xyz> — 2023-03-23T00:09:37Z

    On Wed, Mar 22, 2023 at 02:52:23PM -0400, Melanie Plageman wrote:
> This comment still has the t_ prefix as does the one for tuples_updated
> and deleted.
> 
> otherwise, LGTM.

Good catch.  pgstat_count_heap_update() has a t_tuples_hot_updated,
and pgstat_update_heap_dead_tuples() a t_delta_dead_tuples on top of
the three you have just reported.

I have grepped for all the fields renamed, and nothing else stands
out.  However, my eyes don't have a 100% accuracy, either.
--
Michael

  10. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-03-23T06:51:37Z

    Hi,

On 3/23/23 1:09 AM, Michael Paquier wrote:
> On Wed, Mar 22, 2023 at 02:52:23PM -0400, Melanie Plageman wrote:
>> This comment still has the t_ prefix as does the one for tuples_updated
>> and deleted.
>>
>> otherwise, LGTM.
> 
> Good catch.  pgstat_count_heap_update() has a t_tuples_hot_updated,
> and pgstat_update_heap_dead_tuples() a t_delta_dead_tuples on top of
> the three you have just reported.
> 
> I have grepped for all the fields renamed, and nothing else stands
> out.  However, my eyes don't have a 100% accuracy, either.

Thank you both for your keen eye! I just did another check too and did not
find more than the ones you've just reported.

Please find attached V3 getting rid of them.

Regards,

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

  11. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Michael Paquier <michael@paquier.xyz> — 2023-03-23T08:24:22Z

    On Thu, Mar 23, 2023 at 07:51:37AM +0100, Drouvot, Bertrand wrote:
> Thank you both for your keen eye! I just did another check too and did not
> find more than the ones you've just reported.

This matches what I have, thanks!
--
Michael

  12. Re: Remove nonmeaningful prefixes in PgStat_* fields

    Michael Paquier <michael@paquier.xyz> — 2023-03-23T23:56:35Z

    On Thu, Mar 23, 2023 at 05:24:22PM +0900, Michael Paquier wrote:
> On Thu, Mar 23, 2023 at 07:51:37AM +0100, Drouvot, Bertrand wrote:
> > Thank you both for your keen eye! I just did another check too and did not
> > find more than the ones you've just reported.
> 
> This matches what I have, thanks!

Applied that, after handling the new t_tuples_newpage_updated.
--
Michael