Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry - Mailing list pgsql-hackers
From | Drouvot, Bertrand |
---|---|
Subject | Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry |
Date | |
Msg-id | 621ed53c-4d9a-8f77-ab8c-20235f2d95a1@gmail.com Whole thread Raw |
In response to | Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry (Michael Paquier <michael@paquier.xyz>) |
Responses |
Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
(Michael Paquier <michael@paquier.xyz>)
|
List | pgsql-hackers |
Hi, On 3/16/23 7:29 AM, Michael Paquier wrote: > On Mon, Mar 06, 2023 at 08:33:15AM +0100, Drouvot, Bertrand wrote: >> Thanks for having looked at it! > > Looking at that, I have a few comments. > > + tabentry = (PgStat_TableStatus *) entry_ref->pending; > + tablestatus = palloc(sizeof(PgStat_TableStatus)); > + *tablestatus = *tabentry; > + > [...] > + for (trans = tabentry->trans; trans != NULL; trans = trans->upper) > + { > + tablestatus->t_counts.t_tuples_inserted += trans->tuples_inserted; > + tablestatus->t_counts.t_tuples_updated += trans->tuples_updated; > + tablestatus->t_counts.t_tuples_deleted += trans->tuples_deleted; > + } > > - if (entry_ref) > - return entry_ref->pending; > - return NULL; > + return tablestatus; > > 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()). > meaning that it would make > all the callers of find_tabstat_entry() pay the computation cost. Right. Another suggested approach was to add a flag but then we'd not really hide the internal from pgstatfuncs. > Still it is not really going to matter, because we will just do the > computation once when looking at any pending changes of > pg_stat_xact_all_tables for each entry. Yes. > There are 9 callers of > find_tabstat_entry, with 7 being used for pg_stat_xact_all_tables. Right, those are: pg_stat_get_xact_numscans() pg_stat_get_xact_tuples_returned() pg_stat_get_xact_tuples_fetched() pg_stat_get_xact_tuples_inserted() pg_stat_get_xact_tuples_updated() pg_stat_get_xact_tuples_deleted() pg_stat_get_xact_tuples_hot_updated() > 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. > Could it be a problem if these two also pay the extra computation cost > if a transaction with many subtransactions (aka )needs to look at their > data? These two are used nowhere, they have pg_proc entries and they > are undocumented, so it is hard to say the impact of this change on > them.. > Right, and that's the same for the 7 others as nothing prevent them to be called outside of the pg_stat_xact_all_tables view. Do you think it would be better to add the extra flag then? > Second question: once the data from the subtransactions is copied, > would it be cleaner to set trans to NULL after the data copy is done? > That would not hurt but I'm not sure it's worth it (well, it's currently not done in pg_stat_get_xact_tuples_inserted() 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.) Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
pgsql-hackers by date: