Message-ID: <621ed53c-4d9a-8f77-ab8c-20235f2d95a1@gmail.com>
Date: Thu, 16 Mar 2023 11:32:56 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: Reconcile stats in find_tabstat_entry() and get rid of
 PgStat_BackendFunctionEntry
Content-Language: en-US
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <e4fe4710-09ff-2d74-9680-b63f4b46ebf9@gmail.com>
 <20230214.151102.1070436528757437157.horikyota.ntt@gmail.com>
 <9262b2a9-6785-1f83-bf7f-7fcf90c2c113@gmail.com>
 <20230215.095644.822493184688844040.horikyota.ntt@gmail.com>
 <130095b4-7c6d-b6f1-69d4-94f66231f81d@gmail.com>
 <20230216212121.uvav7xgdh2ysqjy5@awork3.anarazel.de>
 <f08fee25-1dac-b68b-19af-f4b55ceb3802@gmail.com>
 <ZBK3S5Na71pjgGCX@paquier.xyz>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <ZBK3S5Na71pjgGCX@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/621ed53c-4d9a-8f77-ab8c-20235f2d95a1%40gmail.com>
Precedence: bulk

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