On 3/16/23 12:46 PM, Michael Paquier wrote:
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?
Oh I see what you mean, yeah, the pointer is copied.
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?
Yeah, maybe.
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.
due to what potential out-of-core callers could do with it?
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.
No, I did not measure the impact.
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.
Right.
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?
As they could be/are used outside of the xact view, yes I think the same.
There is no trace of them.
Perhaps the ones exposted through pg_stat_xact_all_tables are fine if
not listed.
I'd be tempted to add documentation for all of them, I can look at it.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
