Hi, On 2023-01-12 08:38:57 +0100, Drouvot, Bertrand wrote: > On 1/11/23 11:59 PM, Andres Freund wrote: > > > Now that this patch renames some fields > > > > I don't mind renaming the fields - the prefixes really don't provide > > anything > > useful. But it's not clear why this is related to this patch? You could just > > include the f_ prefix in the macro, no? > > > > > > Right, but the idea is to take the same approach that the one used in > 8018ffbf58 (where placing the prefixes in the macro > would have been possible too).
I'm not super happy about that patch tbh. > > Probably should remove PgStat_BackendFunctionEntry. > > I think that would be a 3rd patch, agree? Yep. > > I now see that PG_STAT_GET_DBENTRY_FLOAT8 already exists, defined the same > > way. But the name fields misleading enough that I'd be inclined to rename > > it? > > > > PG_STAT_GET_FUNCENTRY_FLOAT8_MS looks good by me. Waiting on what we'll > decide for > the existing PG_STAT_GET_DBENTRY_FLOAT8 (so that I can align for the > PG_STAT_GET_FUNCENTRY_FLOAT8). +1 > > Although I suspect this actually hints at an architectural thing that could > > be > > fixed better: Perhaps we should replace find_tabstat_entry() with a version > > returning a fully "reconciled" PgStat_StatTabEntry? It feels quite wrong to > > have that intimitate knowledge of the subtransaction stuff in pgstatfuncs.c > > and about how the different counts get combined. > > > > I think that'd allow us to move the definition of PgStat_TableStatus to > > PgStat_TableXactStatus, PgStat_TableCounts to pgstat_internal.h. Which > > feels a > > heck of a lot cleaner. > > Yeah, I think that would be for a 4th patch, agree? Yea. I am of multiple minds about the ordering. I can see benefits on fixing the architectural issue before reducing duplication in the accessor with a macro. The reason is that if we addressed the architectural issue, the difference between the xact and non-xact version will be very minimal, and could even be generated by the same macro. Greetings, Andres Freund
