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 evaluation the cost of iterating throught
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 :/
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/f572abe7-a1bb-e13b-48c7-2ca150546...@gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From ea025820b30f8421ebe4ea9cf9c19f6fe653118a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
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)
- 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 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
