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
