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

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 16 Mar 2023 11:32:56 +0100

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

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Mar 2023 20:46:15 +0900

> within find_tabstat_entry()).

Sure, however it copies the pointer of the PgStat_TableXactStatus from
tabentry, isn't it?  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?  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.

> 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.

> 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.  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?  There is no trace of them.
Perhaps the ones exposted through pg_stat_xact_all_tables are fine if
not listed.
--
Michael

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 16 Mar 2023 14:13:45 +0100

On 3/16/23 12:46 PM, Michael Paquier wrote:
> 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
> for out-of-core callers of find_tabstat_entry(), no?

Yeah, maybe.

> Perhaps you are

due to what potential out-of-core callers could do with it?

> a lot of savepoints, for example.

No, I did not measure the impact.

> system catalog that is documented. 

Right.

> Still we have a problem here.  I
> 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.

I'd be tempted to add documentation for all of them, I can look at it.

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Mon, 20 Mar 2023 08:43:27 +0900

> I'd be tempted to add documentation for all of them, I can look at it.

I am not sure that there is any need to completely undo ddfc2d9, later
simplified by 5f2b089, so my opinion would be to just add
documentation for the functions that can be used but are used in none
of the system functions.  

Anyway, double-checking, I only see an inconsistency for these two,
confirming my first impression:
- pg_stat_get_xact_blocks_fetched
- pg_stat_get_xact_blocks_hit

There may be a point in having them in some of the system views, but
the non-xact flavors are only used in the statio views, which don't
really need xact versions AFAIK.  I am not sure that it makes much
sense to add them in pg_stat_xact_all_tables, either.  Another view is
just remove them, though some could rely on them externally.  At the
end, documenting both still sounds like the best move to me.
--
Michael

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 20 Mar 2023 11:57:31 +0100

Hi,

On 3/20/23 12:43 AM, Michael Paquier wrote:
> At the

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:

- For pg_stat_get_xact_blocks_hit(), I think it's better to be closer to say the
pg_statio_all_tables.heap_blks_hit definition.

- For pg_stat_get_xact_blocks_fetched(), I think that using "buffer" is better (than block) as at the
end it's related to pgstat_count_buffer_read().

At the end there is a choice to be made for both for the wording between block and buffer. Indeed their
counters get incremented in "buffer" macros while retrieved in those "blocks" functions.

"Buffer" sounds more appropriate to me, so the attached has been done that way.

Regards,

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 6249bb50d0..fba8e892cf 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5697,6 +5697,32 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
        </para></entry>
       </row>
 
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_stat_get_xact_blocks_fetched</primary>
+        </indexterm>
+        <function>pg_stat_get_xact_blocks_fetched</function> ( <type>oid</type> )
+        <returnvalue>bigint</returnvalue>
+       </para>
+       <para>
+        Returns the number of buffer fetches for table or index, in the current transaction
+       </para></entry>
+      </row>
+
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_stat_get_xact_blocks_hit</primary>
+        </indexterm>
+        <function>pg_stat_get_xact_blocks_hit</function> ( <type>oid</type> )
+        <returnvalue>bigint</returnvalue>
+       </para>
+       <para>
+        Returns the number of buffer hits for table or index, in the current transaction
+       </para></entry>
+      </row>
+
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>

---

From: Michael Paquier <[email-masked]>
Date: Wed, 22 Mar 2023 10:16:12 +0900

> "Buffer" sounds more appropriate to me, so the attached has been done that way.

This choice is OK for me.

> +        Returns the number of buffer fetches for table or index, in the current transaction

This should be "number of buffer fetched", no?

> +        Returns the number of buffer hits for table or index, in the current transaction

This one looks OK to me too.
--
Michael

---

From: Melanie Plageman <[email-masked]>
Date: Wed, 22 Mar 2023 14:21:12 -0400

> "Buffer" sounds more appropriate to me, so the attached has been done that way.

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
pgstat_count_buffer_read() is called before ReadBuffer_common() and we
don't know if it is a hit or miss yet). And, I would normally associate
fetching with fetching a block into a buffer. It seems like this counter
is really reflecting the number of buffers acquired or used.

tuples_fetched makes more sense because a tuple is "fetched" into a
slot.

This isn't really the fault of this patch since that member was already
called blocks_fetched.

- Melanie

---

From: Michael Paquier <[email-masked]>
Date: Thu, 23 Mar 2023 07:42:06 +0900

> is really reflecting the number of buffers acquired or used.

Well, it is the number of times we've requested a block read, though
it may not actually be a read if the block was in the cache already.

> This isn't really the fault of this patch since that member was already

The original documentation of these functions added by 46aa77c refers
to "block fetch requests" and "block requests found in cache", so that
would not be right either based on your opinion here.  If you find
"fetch" to be incorrect in this context, here is another idea:
- "block read requests" for blocks_fetched().
- "block read requested but actually found in cache" for blocks_hit().

All the system views care only about the difference between both
counters to count the number of physical reads really done.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Sat, 25 Mar 2023 11:56:22 +0900

> No, I did not measure the impact.

I have been thinking again about this particular point, and I would be
fine with an additional boolean flag to compute the subtrans data in
the global counter when only necessary.  This would not make the
macros for the stat functions that much more complicated, while being
sure that we don't do unnecessary computations when we know that we
don't need them..
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Fri, 24 Mar 2023 20:00:44 -0700

Hi,

> sure that we don't do unnecessary computations when we know that we

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
he subxacts is neglegible.

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Mon, 27 Mar 2023 15:35:48 +0900

> he subxacts is neglegible.

I was wondering about that, and I see why I'm wrong.  I have quickly
gone up to 10k subtransactions, and while I was seeing what looks like
difference of 8~10% in runtime when looking at
pg_stat_xact_all_tables, the overval runtime was still close enough
(5.8ms vs 6.4ms).  At this scale, possible that it was some noise,
these seemed repeatable still not to worry about.

Anyway, I was looking at this patch, and I still feel that it is a bit
incorrect to have the copy of PgStat_TableStatus returned by
find_tabstat_entry() to point to the same list of subtransaction data
as the pending entry found, while the counters are incremented.  This
could lead to mistakes if the copy from find_tabstat_entry() is used
in an unexpected way in the future.  The current callers are OK, but
this does not give me a warm feeling :/
--
Michael

---

From: Melanie Plageman <[email-masked]>
Date: Mon, 27 Mar 2023 10:24:46 -0400

> - "block read requested but actually found in cache" for blocks_hit().

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.

- Melanie

---

From: Michael Paquier <[email-masked]>
Date: Tue, 28 Mar 2023 07:38:25 +0900

> 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 to share here?
--
Michael

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 26 Oct 2023 10:04:25 +0200

Hi,

> this does not give me a warm feeling :/

FWIW, please find attached V7 (mandatory rebase).

It would allow to also define:

- pg_stat_get_xact_tuples_inserted
- pg_stat_get_xact_tuples_updated
- pg_stat_get_xact_tuples_deleted

as macros, joining others pg_stat_get_xact_*() that are already
defined as macros.

The concern you raised above has not been addressed, meaning that
find_tabstat_entry() still return a copy of PgStat_TableStatus.

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?

Trying to understand to see if I should invest time to try to address your concern
or leave those 3 functions as they are currently before moving back to the
"Split index and table statistics into different types of stats" work [1].

[1]: https://www.postgresql.org/message-id/flat/[email-masked]

Regards,

From ea025820b30f8421ebe4ea9cf9c19f6fe653118a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Thu, 26 Oct 2023 06:38:08 +0000
Subject: [PATCH v7] Reconcile stats in find_tabstat_entry()

---
 src/backend/utils/activity/pgstat_relation.c | 32 +++++++++-
 src/backend/utils/adt/pgstatfuncs.c          | 66 ++------------------
 2 files changed, 35 insertions(+), 63 deletions(-)
  38.8% src/backend/utils/activity/
  61.1% src/backend/utils/adt/

diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 9876e0c1e8..8ebef877d0 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -478,20 +478,46 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
  * Find any existing PgStat_TableStatus entry for rel_id in the current
  * database. If not found, try finding from shared tables.
  *
+ * If an entry is found, copy it and increment the copy's counters with their
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).
+ *
  * If no entry found, return NULL, don't create a new one
  */
 PgStat_TableStatus *
 find_tabstat_entry(Oid rel_id)
 {
 	PgStat_EntryRef *entry_ref;
+	PgStat_TableXactStatus *trans;
+	PgStat_TableStatus *tabentry = NULL;
+	PgStat_TableStatus *tablestatus = NULL;
 
 	entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, MyDatabaseId, rel_id);
 	if (!entry_ref)
+	{
 		entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION, InvalidOid, rel_id);
+		if(!entry_ref)
+			return tablestatus;
+	}
+
+	tabentry = (PgStat_TableStatus *) entry_ref->pending;
+	tablestatus = palloc(sizeof(PgStat_TableStatus));
+	*tablestatus = *tabentry;
+
+	/*
+	 * Live subtransactions' counts aren't in counts yet. This is not a hot
+	 * code path so it sounds ok to reconcile for tuples_inserted,
+	 * tuples_updated and tuples_deleted even if this is not what the caller
+	 * is interested in.
+	 */
+	for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
+	{
+		tablestatus->counts.tuples_inserted += trans->tuples_inserted;
+		tablestatus->counts.tuples_updated += trans->tuples_updated;
+		tablestatus->counts.tuples_deleted += trans->tuples_deleted;
+	}
 
-	if (entry_ref)
 }
 
 /*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 3b44af8006..01362ae688 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1600,68 +1600,14 @@ PG_STAT_GET_XACT_RELENTRY_INT64(blocks_fetched)
 /* pg_stat_get_xact_blocks_hit */
 PG_STAT_GET_XACT_RELENTRY_INT64(blocks_hit)
 
-Datum
-pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-	PgStat_TableXactStatus *trans;
+/* pg_stat_get_xact_tuples_inserted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_inserted)
 
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-	{
-		result = tabentry->counts.tuples_inserted;
-		/* live subtransactions' counts aren't in tuples_inserted yet */
-		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
-			result += trans->tuples_inserted;
-	}
+/* pg_stat_get_xact_tuples_updated */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_updated)
 
-	PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-	PgStat_TableXactStatus *trans;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-	{
-		result = tabentry->counts.tuples_updated;
-		/* live subtransactions' counts aren't in tuples_updated yet */
-		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
-			result += trans->tuples_updated;
-	}
-
-	PG_RETURN_INT64(result);
-}
-
-Datum
-pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
-{
-	Oid			relid = PG_GETARG_OID(0);
-	int64		result;
-	PgStat_TableStatus *tabentry;
-	PgStat_TableXactStatus *trans;
-
-	if ((tabentry = find_tabstat_entry(relid)) == NULL)
-		result = 0;
-	else
-	{
-		result = tabentry->counts.tuples_deleted;
-		/* live subtransactions' counts aren't in tuples_deleted yet */
-		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
-			result += trans->tuples_deleted;
-	}
-
-	PG_RETURN_INT64(result);
-}
+/* pg_stat_get_xact_tuples_deleted */
+PG_STAT_GET_XACT_RELENTRY_INT64(tuples_deleted)
 
 Datum
 pg_stat_get_xact_function_calls(PG_FUNCTION_ARGS)
-- 
2.34.1

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Oct 2023 15:07:55 +0900

> the caller forgetting it is working on a copy and then could work with

(Be careful about the code indentation.)

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 same location as
tabentry->trans, but wouldn't it be better to set tablestatus->trans
to NULL instead for the copy returned to the caller?
--
Michael

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Fri, 27 Oct 2023 09:45:52 +0200

Hi,

> to NULL instead for the copy returned to the caller?

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).

Regards,

From 9f69363296058bd473efa50f8d8b995627f0dbdf Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Thu, 26 Oct 2023 06:38:08 +0000
Subject: [PATCH v8] Reconcile stats in find_tabstat_entry()

---
 src/backend/utils/activity/pgstat_relation.c | 38 ++++++++++-
 src/backend/utils/adt/pgstatfuncs.c          | 66 ++------------------
 2 files changed, 41 insertions(+), 63 deletions(-)
  42.3% src/backend/utils/activity/
  57.6% src/backend/utils/adt/

index 9876e0c1e8..4c694d925d 100644
@@ -478,20 +478,52 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
 
+		if (!entry_ref)
+	 * We don't want tablestatus->trans to point to the same location as
+	 * tabentry->trans as that could lead to undesired behavior.
+	tablestatus->trans = NULL;
 
-	if (entry_ref)
 
 /*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 6468b6a805..998c69e241 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1588,68 +1588,14 @@ PG_STAT_GET_XACT_RELENTRY_INT64(blocks_fetched)
 /* pg_stat_get_xact_blocks_hit */
 PG_STAT_GET_XACT_RELENTRY_INT64(blocks_hit)
 
-Datum

 
 Datum

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Oct 2023 16:50:45 +0900

> Done in V8 attached (pgindent has been run on pgstatfuncs.c and

LGTM.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 30 Oct 2023 08:25:17 +0900

> Done in V8 attached (pgindent has been run on pgstatfuncs.c and

And applied that after editing a bit the comments.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Tue, 7 Nov 2023 20:24:45 -0800

> And applied that after editing a bit the comments.

Thank you both!