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

2023-11-07 Thread Andres Freund
On 2023-10-30 08:25:17 +0900, Michael Paquier wrote: > On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote: > > Done in V8 attached (pgindent has been run on pgstatfuncs.c and > > pgstat_relation.c). > > And applied that after editing a bit the comments. Thank you both!

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

2023-10-29 Thread Michael Paquier
On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote: > Done in V8 attached (pgindent has been run on pgstatfuncs.c and > pgstat_relation.c). And applied that after editing a bit the comments. -- Michael signature.asc Description: PGP signature

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

2023-10-27 Thread Michael Paquier
On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote: > Oh I see, yeah I do agree to set tablestatus->trans to NULL to avoid > any undesired interference with tabentry->trans. > > Done in V8 attached (pgindent has been run on pgstatfuncs.c and > pgstat_relation.c). LGTM. -- Michael

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

2023-10-27 Thread Drouvot, Bertrand
Hi, On 10/27/23 8:07 AM, Michael Paquier wrote: The part that I found disturbing is here: + tabentry = (PgStat_TableStatus *) entry_ref->pending; + tablestatus = palloc(sizeof(PgStat_TableStatus)); + *tablestatus = *tabentry; This causes tablestatus->trans to point to the

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

2023-10-27 Thread Michael Paquier
On Thu, Oct 26, 2023 at 10:04:25AM +0200, Drouvot, Bertrand wrote: > By "used in an unexpected way in the future", what do you mean exactly? Do > you mean > the caller forgetting it is working on a copy and then could work with > "stale" counters? (Be careful about the code indentation.) The

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

2023-10-26 Thread Drouvot, Bertrand
Hi, On 3/27/23 8:35 AM, Michael Paquier wrote: On Fri, Mar 24, 2023 at 08:00:44PM -0700, Andres Freund wrote: I don't understand what we're optimizing for here. These functions are very very very far from being a hot path. The xact functions are barely ever used. Compared to the cost of query

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

2023-04-04 Thread Michael Paquier
On Tue, Apr 04, 2023 at 09:04:34PM +0900, Michael Paquier wrote: > This addition looks OK for me. Thanks for the patch! Okay, finally done. One part that was still not complete to me in light of the information ddfc2d9 has removed is that the number of physical reads could be lower than the

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

2023-04-04 Thread Michael Paquier
On Wed, Mar 29, 2023 at 07:44:20AM +0200, Drouvot, Bertrand wrote: > Please find a draft attached. This addition looks OK for me. Thanks for the patch! -- Michael signature.asc Description: PGP signature

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

2023-03-28 Thread Drouvot, Bertrand
Hi, On 3/29/23 2:09 AM, Michael Paquier wrote: On Tue, Mar 28, 2023 at 05:43:26PM +0900, Kyotaro Horiguchi wrote: No. Fine by me, except that "block read requests" seems to suggest kernel read() calls, maybe because it's not clear whether "block" refers to our buffer blocks or file blocks to

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

2023-03-28 Thread Michael Paquier
On Tue, Mar 28, 2023 at 05:43:26PM +0900, Kyotaro Horiguchi wrote: > No. Fine by me, except that "block read requests" seems to suggest > kernel read() calls, maybe because it's not clear whether "block" > refers to our buffer blocks or file blocks to me.. If it is generally > clear, I'm fine with

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

2023-03-28 Thread Kyotaro Horiguchi
At Tue, 28 Mar 2023 15:16:36 +0900, Michael Paquier wrote in > On Tue, Mar 28, 2023 at 07:49:45AM +0200, Drouvot, Bertrand wrote: > > What about something like? > > > > for pg_stat_get_xact_blocks_fetched(): "block read requests for table or > > index, in the current > > transaction. This

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

2023-03-28 Thread Michael Paquier
On Tue, Mar 28, 2023 at 07:49:45AM +0200, Drouvot, Bertrand wrote: > What about something like? > > for pg_stat_get_xact_blocks_fetched(): "block read requests for table or > index, in the current > transaction. This number minus pg_stat_get_xact_blocks_hit() gives the number > of kernel >

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

2023-03-27 Thread Drouvot, Bertrand
Hi, On 3/28/23 7:23 AM, Michael Paquier wrote: On Tue, Mar 28, 2023 at 12:36:15PM +0900, Kyotaro Horiguchi wrote: I found that commit ddfc2d9a37 removed the descriptions for pg_stat_get_blocks_fetched and pg_stat_get_blocks_hit. Right before that commit, monitoring.sgml had these lines: -

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

2023-03-27 Thread Michael Paquier
On Tue, Mar 28, 2023 at 12:36:15PM +0900, Kyotaro Horiguchi wrote: > I found that commit ddfc2d9a37 removed the descriptions for > pg_stat_get_blocks_fetched and pg_stat_get_blocks_hit. Right before > that commit, monitoring.sgml had these lines: > > - pg_stat_get_blocks_fetched minus > -

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

2023-03-27 Thread Kyotaro Horiguchi
At Tue, 28 Mar 2023 07:38:25 +0900, Michael Paquier wrote in > On Mon, Mar 27, 2023 at 10:24:46AM -0400, Melanie Plageman wrote: > > I do like/prefer "block read requests" and > > "blocks requested found in cache" > > Though, now I fear my initial complaint may have been a bit pedantic. > >

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

2023-03-27 Thread Michael Paquier
On Mon, Mar 27, 2023 at 10:24:46AM -0400, Melanie Plageman wrote: > I do like/prefer "block read requests" and > "blocks requested found in cache" > Though, now I fear my initial complaint may have been a bit pedantic. That's fine. Let's ask for extra opinions, then. So, have others an opinion

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

2023-03-27 Thread Melanie Plageman
On Wed, Mar 22, 2023 at 6:42 PM Michael Paquier wrote: > > On Wed, Mar 22, 2023 at 02:21:12PM -0400, Melanie Plageman wrote: > > Apologies as I know this docs update has already been committed, but > > buffers fetched and blocks fetched both feel weird to me. If you have a > > cache hit, you

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

2023-03-27 Thread Michael Paquier
On Fri, Mar 24, 2023 at 08:00:44PM -0700, Andres Freund wrote: > I don't understand what we're optimizing for here. These functions are very > very very far from being a hot path. The xact functions are barely ever > used. Compared to the cost of query evaluation the cost of iterating throught >

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

2023-03-24 Thread Andres Freund
Hi, On 2023-03-25 11:56:22 +0900, Michael Paquier wrote: > On Thu, Mar 16, 2023 at 02:13:45PM +0100, Drouvot, Bertrand wrote: > > On 3/16/23 12:46 PM, Michael Paquier wrote: > >>> I don't think we should pay any particular attention to those 2 ones > >>> as anyway nothing prevent the 7 others to

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

2023-03-24 Thread Michael Paquier
On Thu, Mar 16, 2023 at 02:13:45PM +0100, Drouvot, Bertrand wrote: > On 3/16/23 12:46 PM, Michael Paquier wrote: >>> 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. >> >>

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

2023-03-22 Thread Michael Paquier
On Wed, Mar 22, 2023 at 02:21:12PM -0400, Melanie Plageman wrote: > Apologies as I know this docs update has already been committed, but > buffers fetched and blocks fetched both feel weird to me. If you have a > cache hit, you don't end up really "fetching" anything at all (since >

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

2023-03-22 Thread Melanie Plageman
On Mon, Mar 20, 2023 at 6:58 AM Drouvot, Bertrand wrote: > > Hi, > > On 3/20/23 12:43 AM, Michael Paquier wrote: > > At the > > end, documenting both still sounds like the best move to me. > > Agree. > > Please find attached > v1-0001-pg_stat_get_xact_blocks_fetched-and_hit-doc.patch doing so. >

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

2023-03-22 Thread Michael Paquier
On Wed, Mar 22, 2023 at 09:20:25AM +0100, Drouvot, Bertrand wrote: > That said, please find enclosed V2 with "buffers fetched" suggested > above (and no changes to "buffer hits" to keep consistency with the > other part of the documentation mentioned up-thread). Thanks. Applied and backpatched

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

2023-03-22 Thread Drouvot, Bertrand
Hi, On 3/22/23 7:44 AM, Drouvot, Bertrand wrote: Hi, On 3/22/23 5:45 AM, Michael Paquier wrote: On Wed, Mar 22, 2023 at 11:37:03AM +0900, Kyotaro Horiguchi wrote: In the original description, "buffer fetches" appears to be a plural form of a compound noun and correct, similar to "buffer

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

2023-03-22 Thread Drouvot, Bertrand
Hi, On 3/22/23 5:45 AM, Michael Paquier wrote: On Wed, Mar 22, 2023 at 11:37:03AM +0900, Kyotaro Horiguchi wrote: In the original description, "buffer fetches" appears to be a plural form of a compound noun and correct, similar to "buffer hits" mentioned later. If we reword it, I think it

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

2023-03-21 Thread Michael Paquier
On Wed, Mar 22, 2023 at 11:37:03AM +0900, Kyotaro Horiguchi wrote: > In the original description, "buffer fetches" appears to be a plural > form of a compound noun and correct, similar to "buffer hits" > mentioned later. If we reword it, I think it should be "number of > buffers fetched". Using

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

2023-03-21 Thread Kyotaro Horiguchi
At Wed, 22 Mar 2023 10:16:12 +0900, Michael Paquier wrote in > On Mon, Mar 20, 2023 at 11:57:31AM +0100, Drouvot, Bertrand wrote: > > "Buffer" sounds more appropriate to me, so the attached has been done that > > way. > > This choice is OK for me. > > > + > > +

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

2023-03-21 Thread Michael Paquier
On Mon, Mar 20, 2023 at 11:57:31AM +0100, Drouvot, Bertrand wrote: > "Buffer" sounds more appropriate to me, so the attached has been done that > way. This choice is OK for me. > + > + pg_stat_get_xact_blocks_fetched > + > +pg_stat_get_xact_blocks_fetched ( >

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

2023-03-20 Thread Drouvot, Bertrand
Hi, On 3/20/23 12:43 AM, Michael Paquier wrote: At the end, documenting both still sounds like the best move to me. Agree. Please find attached v1-0001-pg_stat_get_xact_blocks_fetched-and_hit-doc.patch doing so. I did not put the exact same wording as the one being removed in ddfc2d9, as:

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

2023-03-19 Thread Michael Paquier
On Thu, Mar 16, 2023 at 02:13:45PM +0100, Drouvot, Bertrand wrote: > On 3/16/23 12:46 PM, Michael Paquier wrote: >> 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

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

2023-03-16 Thread Drouvot, Bertrand
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

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

2023-03-16 Thread Michael Paquier
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

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

2023-03-16 Thread Drouvot, Bertrand
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 =

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

2023-03-16 Thread Michael Paquier
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; + [...] +

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

2023-03-05 Thread Drouvot, Bertrand
Hi, On 2/16/23 10:21 PM, Andres Freund wrote: Hi, On 2023-02-15 09:21:48 +0100, Drouvot, Bertrand wrote: diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c index f793ac1516..b26e2a5a7a 100644 ---

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

2023-02-16 Thread Andres Freund
Hi, On 2023-02-15 09:21:48 +0100, Drouvot, Bertrand wrote: > diff --git a/src/backend/utils/activity/pgstat_relation.c > b/src/backend/utils/activity/pgstat_relation.c > index f793ac1516..b26e2a5a7a 100644 > --- a/src/backend/utils/activity/pgstat_relation.c > +++

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

2023-02-15 Thread Drouvot, Bertrand
Hi, On 2/15/23 1:56 AM, Kyotaro Horiguchi wrote: At Tue, 14 Feb 2023 15:43:26 +0100, "Drouvot, Bertrand" wrote in The comment assumed that the function directly returns an entry from shared memory, but now it copies the entry's contents into a palloc'ed memory and stores the sums of some

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

2023-02-14 Thread Kyotaro Horiguchi
At Tue, 14 Feb 2023 15:43:26 +0100, "Drouvot, Bertrand" wrote in > Oh right, my bad (the issue has been introduced in V2). > Fixed in V4. Great! > > I thought that we might be able to return entry_ref->pending since the > > callers don't call pfree on the returned pointer, but it is not great

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

2023-02-14 Thread Drouvot, Bertrand
Hi, On 2/14/23 7:11 AM, Kyotaro Horiguchi wrote: Isn't it ignoring the second call to pgstat_fetch_pending_entry? Oh right, my bad (the issue has been introduced in V2). Fixed in V4. I thought that we might be able to return entry_ref->pending since the callers don't call pfree on the

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

2023-02-13 Thread Kyotaro Horiguchi
Thanks for the new version. At Mon, 13 Feb 2023 09:58:52 +0100, "Drouvot, Bertrand" wrote in > Hi, > > On 2/13/23 8:40 AM, Kyotaro Horiguchi wrote: > > At Mon, 13 Feb 2023 08:09:50 +0100, "Drouvot, Bertrand" > > wrote in > >>> I think this is useful beyond being able to generate those

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

2023-02-13 Thread Drouvot, Bertrand
Hi, On 2/13/23 8:40 AM, Kyotaro Horiguchi wrote: At Mon, 13 Feb 2023 08:09:50 +0100, "Drouvot, Bertrand" wrote in I think this is useful beyond being able to generate those functions with macros. The fact that we had to deal with transactional code in pgstatfuncs.c meant that a lot of the

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

2023-02-12 Thread Kyotaro Horiguchi
At Mon, 13 Feb 2023 08:09:50 +0100, "Drouvot, Bertrand" wrote in >> I think this is useful beyond being able to generate those functions >> with >> macros. The fact that we had to deal with transactional code in >> pgstatfuncs.c >> meant that a lot of the relevant itnernals had to be exposed

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

2023-02-12 Thread Drouvot, Bertrand
Hi, On 2/10/23 10:46 PM, Andres Freund wrote: Hi, On 2023-02-09 11:38:18 +0100, Drouvot, Bertrand wrote: Please find attached a patch proposal for $SUBJECT. The idea has been raised in [1] by Andres: it would allow to simplify even more the work done to generate pg_stat_get_xact*()

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

2023-02-10 Thread Andres Freund
Hi, On 2023-02-09 11:38:18 +0100, Drouvot, Bertrand wrote: > Please find attached a patch proposal for $SUBJECT. > > The idea has been raised in [1] by Andres: it would allow to simplify even > more the work done to > generate pg_stat_get_xact*() functions with Macros. Thanks! I think this is

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

2023-02-10 Thread Andres Freund
: Andres Freund References: <20230210.113242.699878230551547182.horikyota@gmail.com> <5420b28c-d33f-d25d-9f47-b06b8a237...@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5420b28c-d33f-d25d-9f47-b06b8a237...@gmail.com> Hi,

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

2023-02-10 Thread Drouvot, Bertrand
Hi, On 2/10/23 3:32 AM, Kyotaro Horiguchi wrote: At Thu, 9 Feb 2023 11:38:18 +0100, "Drouvot, Bertrand" wrote in Hi hackers, Please find attached a patch proposal for $SUBJECT. The idea has been raised in [1] by Andres: it would allow to simplify even more the work done to generate

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

2023-02-09 Thread Kyotaro Horiguchi
At Thu, 9 Feb 2023 11:38:18 +0100, "Drouvot, Bertrand" wrote in > Hi hackers, > > Please find attached a patch proposal for $SUBJECT. > > The idea has been raised in [1] by Andres: it would allow to simplify > even more the work done to > generate pg_stat_get_xact*() functions with Macros. >

Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry

2023-02-09 Thread Drouvot, Bertrand
Hi hackers, Please find attached a patch proposal for $SUBJECT. The idea has been raised in [1] by Andres: it would allow to simplify even more the work done to generate pg_stat_get_xact*() functions with Macros. Indeed, with the reconciliation done in find_tabstat_entry() then all the