Message-ID: <f6ea424b-fabe-b702-e4e3-b71ebc827d59@gmail.com>
Date: Thu, 16 Mar 2023 14:13:45 +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>
 <621ed53c-4d9a-8f77-ab8c-20235f2d95a1@gmail.com>
 <ZBMBh5ve3tyqH5/m@paquier.xyz>
From: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
In-Reply-To: <ZBMBh5ve3tyqH5/m@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/f6ea424b-fabe-b702-e4e3-b71ebc827d59%40gmail.com>
Precedence: bulk

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