Hi,
On 11/4/22 9:51 PM, Melanie Plageman wrote:
Hi Bertrand,
I'm glad you are working on this.
I had a few thoughts/ideas
Thanks for looking at it!
It seems better to have all of the counts in the various stats structs
not be prefixed with n_, i_, t_
typedef struct PgStat_StatDBEntry
{
...
PgStat_Counter n_blocks_fetched;
PgStat_Counter n_blocks_hit;
PgStat_Counter n_tuples_returned;
PgStat_Counter n_tuples_fetched;
...
I've attached a patch (0002) to change this in case you are interested
in making such a change
I did not renamed initially the fields/columns to ease the review.
Indeed, I think we should go further than removing the n_, i_ and t_
prefixes so that the fields actually match the view's columns.
For example, currently pg_stat_all_indexes.idx_tup_read is linked to
"tuples_returned", so that it would make sense to rename
"tuples_returned" to "tuples_read" or even "tup_read" in the indexes
counters.
That's why I had in mind to do this fields/columns renaming into a
separate patch (once this one is committed), so that the current one
focus only on splitting the stats: what do you think?
(I've attached all of my suggestions in patches
along with your original patch so that cfbot still passes).
On Wed, Nov 2, 2022 at 5:00 AM Drouvot, Bertrand
<bertranddrouvot...@gmail.com> wrote:
On 11/1/22 1:30 AM, Andres Freund wrote:
On 2022-10-31 14:14:15 +0100, Drouvot, Bertrand wrote:
@@ -240,9 +293,23 @@ pg_stat_get_blocks_fetched(PG_FUNCTION_ARGS)
PG_RETURN_INT64(result);
}
+Datum
+pg_stat_get_index_blocks_fetched(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int64 result;
+ PgStat_StatIndEntry *indentry;
+
+ if ((indentry = pgstat_fetch_stat_indentry(relid)) == NULL)
+ result = 0;
+ else
+ result = (int64) (indentry->blocks_fetched);
+
+ PG_RETURN_INT64(result);
+}
We have so many copies of this by now - perhaps we first should deduplicate
them somehow? Even if it's just a macro or such.
Yeah good point, a new macro has been defined for the "int64" return
case in V3 attached.
I looked for other opportunities to de-duplicate, but most of the functions
that were added that are identical except the return type and
PgStat_Kind are short enough that it doesn't make sense to make wrappers
or macros.
Yeah, agree.
I do think it makes sense to reorder the members of the two structs
PgStat_IndexCounts and PgStat_TableCounts so that they have a common
header. I've done that in the attached patch (0003).
That's a good idea, thanks! But for that we would need to have the same
fields names, means:
- Remove the prefixes (as you've done in 0002)
- And probably reduce the number of fields in the new
PgStat_RelationCounts that 003 is introducing (for example
tuples_returned should be excluded if we're going to rename it later on
to "tuples_read" for the indexes to map the
pg_stat_all_indexes.idx_tup_read column).
ISTM that we should do it in the "renaming" effort, after this patch is
committed.
In the flush functions, I was also thinking it might be nice to use the
same pattern as is used in [1] and [2] to add the counts together. It
makes the lines a bit easier to read, IMO. If we remove the prefixes
from the count fields, this works for many of the fields. I've attached
a patch (0004) that does something like this, in case you wanted to go
in this direction.
I like it too but same remarks as previously. I think it should be part
of the "renaming" effort.
Since you have made new parallel functions for indexes and tables for
many of the functions in pgstat_relation.c, perhaps it makes sense to
split it into pgstat_table.c and pgstat_index.c?
Good point, thanks, I'll work on it.
One question I had about the original code (not related to your
refactor) is why the pending stats aren't memset in the flush functions
after aggregating them into the shared stats.
Not sure I'm getting your point, do you think something is not right
with the flush functions?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
