Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry

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