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

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 15 Feb 2023 09:56:44 +0900 (JST)

At Tue, 14 Feb 2023 15:43:26 +0100, "Drouvot, Bertrand" <[email-masked]> 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
> > that we don't inform the callers if the returned memory can be safely
> > pfreed or not.
> > Thus what I have in mind is the following.
> > 
> >>    	if (!entry_ref)
> >> +	{
> >>    		entry_ref = pgstat_fetch_pending_entry(PGSTAT_KIND_RELATION,
> >>    		InvalidOid, rel_id);
> >> +		if (!entry_ref)
> >> +         return NULL;
> >> +	}
> 
> LGTM, done that way in V4.

That part looks good to me, thanks!

I was going through v4 and it seems to me that the comment for
find_tabstat_entry may not be quite right.

> * find any existing PgStat_TableStatus entry for rel
> *
> * Find any existing PgStat_TableStatus entry for rel_id in the current
> * database. If not found, try finding from shared tables.
> *
> * If no entry found, return NULL, don't create a new one

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 counters for the current
transaction in it. Do you think we should update the comment to
reflect this change?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Wed, 15 Feb 2023 09:21:48 +0100

Hi,

> transaction in it. Do you think we should update the comment to

Good point, thanks! Yeah, definitively, done in V5 attached.
  
Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -471,20 +471,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. There is no need for the
+ * caller to pfree the copy as the MemoryContext will be 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 t_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->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;
 }
 
 /*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 9d707c3521..405d080daa 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1548,17 +1548,11 @@ pg_stat_get_xact_tuples_inserted(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->t_counts.t_tuples_inserted;
-		/* live subtransactions' counts aren't in t_tuples_inserted yet */
-		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
-			result += trans->tuples_inserted;
-	}
+		result = (int64) (tabentry->t_counts.t_tuples_inserted);
 
 	PG_RETURN_INT64(result);
 }
@@ -1569,17 +1563,11 @@ 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->t_counts.t_tuples_updated;
-		/* live subtransactions' counts aren't in t_tuples_updated yet */
-		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
-			result += trans->tuples_updated;
-	}
+		result = (int64) (tabentry->t_counts.t_tuples_updated);
 
 	PG_RETURN_INT64(result);
 }
@@ -1590,17 +1578,11 @@ 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->t_counts.t_tuples_deleted;
-		/* live subtransactions' counts aren't in t_tuples_deleted yet */
-		for (trans = tabentry->trans; trans != NULL; trans = trans->upper)
-			result += trans->tuples_deleted;
-	}
+		result = (int64) (tabentry->t_counts.t_tuples_deleted);
 
 	PG_RETURN_INT64(result);
 }

---

From: Andres Freund <[email-masked]>
Date: Thu, 16 Feb 2023 13:21:21 -0800

Hi,

> + * caller to pfree the copy as the MemoryContext will be reset soon after.

The "There is no need" bit seems a bit off. Yes, that's true for the current
callers, but who says that it has to stay that way?

Otherwise this looks ready, on a casual scan.

Greetings,

Andres Freund

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 6 Mar 2023 08:33:15 +0100

Hi,

> callers, but who says that it has to stay that way?

Good point. Wording has been changed in V6 attached.

> Otherwise this looks ready, on a casual scan.

Thanks for having looked at it!

Regards,

index f793ac1516..c030e1782e 100644
+ * subtransactions counterparts. Then return the copy. The caller may need to
+ * pfree the copy (in case the MemoryContext is not reset soon after).

-	if (entry_ref)
 
 /*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index b61a12382b..b4de57c535 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1546,17 +1546,11 @@ pg_stat_get_xact_tuples_inserted(PG_FUNCTION_ARGS)
 	Oid			relid = PG_GETARG_OID(0);
 	int64		result;
 	PgStat_TableStatus *tabentry;
-	PgStat_TableXactStatus *trans;

 	PG_RETURN_INT64(result);
 }
@@ -1567,17 +1561,11 @@ pg_stat_get_xact_tuples_updated(PG_FUNCTION_ARGS)
 	Oid			relid = PG_GETARG_OID(0);
 	int64		result;
 	PgStat_TableStatus *tabentry;
-	PgStat_TableXactStatus *trans;

 	PG_RETURN_INT64(result);
 }
@@ -1588,17 +1576,11 @@ pg_stat_get_xact_tuples_deleted(PG_FUNCTION_ARGS)
 	Oid			relid = PG_GETARG_OID(0);
 	int64		result;
 	PgStat_TableStatus *tabentry;
-	PgStat_TableXactStatus *trans;

 	PG_RETURN_INT64(result);
 }

---

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

> Thanks for having looked at it!

Looking at that, I have a few comments.

[...]
 
-    if (entry_ref)

From what I get with this change, the number of tuples changed by DMLs
have their computations done a bit earlier, meaning that it would make
all the callers of find_tabstat_entry() pay the computation cost.
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.  There are 9 callers of
find_tabstat_entry, with 7 being used for pg_stat_xact_all_tables.
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()?
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..

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?

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