On Thu, Mar 16, 2023 at 11:32:56AM +0100, Drouvot, Bertrand wrote: > On 3/16/23 7:29 AM, Michael Paquier wrote: >> From what I get with this change, the number of tuples changed by DMLs >> have their computations done a bit earlier, > > Thanks for looking at it! > > Right, but note this is in a dedicated new tablestatus (created > within find_tabstat_entry()).
Sure, however it copies the pointer of the PgStat_TableXactStatus from tabentry, isn't it? This means that it keeps a reference of the chain of subtransactions. It does not matter for the functions but it could for out-of-core callers of find_tabstat_entry(), no? Perhaps you are right and that's not worth worrying, still I don't feel particularly confident that this is the best approach we can take. >> How much do we need to care about the remaining two callers >> pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit()? > > Regarding pg_stat_get_xact_blocks_fetched() and pg_stat_get_xact_blocks_hit() > the callers (if any) are outside of the core PG (as from what I can > see they are not used at all). > > I don't think we should pay any particular attention to those 2 ones > as anyway nothing prevent the 7 others to be called outside of the > pg_stat_xact_all_tables view. I am not quite sure, TBH. Did you look at the difference with a long chain of subtrans, like savepoints? The ODBC driver "loves" producing a lot of savepoints, for example. >> It would feel a bit safer to me to document that find_tabstat_entry() >> is currently only used for this xact system view.. The extra >> computation could lead to surprises, actually, if this routine is used >> outside this context? Perhaps that's OK, but it does not give me a >> warm feeling, just to reshape three functions of pgstatfuncs.c with >> macros. > > That's a fair point. On the other hand those 9 functions (which can > all be used outside of the pg_stat_xact_all_tables view) are not > documented, so I'm not sure this is that much of a concern (and if > we think it is we still gave the option to add an extra flag to > indicate whether or not the extra computation is needed.) That's not quite exact, I think. The first 7 functions are used in a system catalog that is documented. Still we have a problem here. I can actually see a few projects relying on these two functions while looking a bit around, so they are used. And the issue comes from ddfc2d9, that has removed these functions from the documentation ignoring that they are used in no system catalogs. I think that we should fix that and re-add the two missing functions with a proper description in the docs, at least? There is no trace of them. Perhaps the ones exposted through pg_stat_xact_all_tables are fine if not listed. -- Michael
signature.asc
Description: PGP signature
