Thread: Remove nonmeaningful prefixes in PgStat_* fields
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
Attachment
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/
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
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
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
Attachment
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
Attachment
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
Attachment
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
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
Attachment
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
Attachment
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
Attachment
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