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*() functions with Macros.
Thanks!
Thanks for looking at it!
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 "outside" pgstat,
which seems architecturally not great.
Right, good point.
Indeed, with the reconciliation done in find_tabstat_entry() then all the
pg_stat_get_xact*() functions
(making use of find_tabstat_entry()) now "look the same" (should they take into
account live subtransactions or not).
I'm not bothered by making all of pg_stat_get_xact* functions more expensive,
they're not a hot code path. But if we need to, we could just add a parameter
to find_tabstat_entry() indicating whether we need to reconcile or not.
I think that's a good idea to avoid doing extra work if not needed.
V2 adds such a bool.
/* save stats for this function, later used to compensate for recursion
*/
- fcu->save_f_total_time = pending->f_counts.f_total_time;
+ fcu->save_f_total_time = pending->f_total_time;
/* save current backend-wide total time */
fcu->save_total = total_func_time;
The diff is noisy due to all the mechanical changes like the above. Could that
be split into a separate commit?
Fully agree, the PgStat_BackendFunctionEntry stuff will be done in a separate
patch.
find_tabstat_entry(Oid rel_id)
{
PgStat_EntryRef *entry_ref;
+ 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 entry_ref->pending;
- return NULL;
+ {
+ PgStat_TableStatus *tabentry = (PgStat_TableStatus *)
entry_ref->pending;
I'd add an early return for the !entry_ref case, that way you don't need to
indent the bulk of the function.
Good point, done in V2.
+ PgStat_TableXactStatus *trans;
+
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ memcpy(tablestatus, tabentry, sizeof(PgStat_TableStatus));
For things like this I'd use
*tablestatus = *tabentry;
that way the compiler will warn you about mismatching types, and you don't
need the sizeof().
Good point, done in V2.
+ /* live subtransactions' counts aren't in t_counts yet */
+ 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;
+ }
+ }
Hm, why do we end uup with t_counts still being used here, but removed other
places?
t_counts are not removed, maybe you are confused with the "f_counts" that were
removed
in V1 due to the PgStat_BackendFunctionEntry related changes?
diff --git a/src/backend/utils/adt/pgstatfuncs.c
b/src/backend/utils/adt/pgstatfuncs.c
index 6737493402..40a6fbf871 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1366,7 +1366,10 @@ pg_stat_get_xact_numscans(PG_FUNCTION_ARGS)
if ((tabentry = find_tabstat_entry(relid)) == NULL)
result = 0;
else
+ {
result = (int64) (tabentry->t_counts.t_numscans);
+ pfree(tabentry);
+ }
PG_RETURN_INT64(result);
}
I don't think we need to bother with individual pfrees in this path. The
caller will call the function in a dedicated memory context, that'll be reset
very soon after this.
Oh right, the palloc is done in the ExprContext memory context that is reset
soon after.
Removing the pfrees in V2 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..1d3f068a1c 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -474,17 +474,33 @@ pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid)
* If no entry found, return NULL, don't create a new one
*/
PgStat_TableStatus *
-find_tabstat_entry(Oid rel_id)
+find_tabstat_entry(Oid rel_id, bool add_subtrans)
{
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);
+ return tablestatus;
+ }
+
+ tabentry = (PgStat_TableStatus *) entry_ref->pending;
+ tablestatus = palloc(sizeof(PgStat_TableStatus));
+ *tablestatus = *tabentry;
+
+ /* live subtransactions' counts aren't in t_counts yet */
+ for (trans = tabentry->trans; add_subtrans && 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 924698e6ae..349fbdf967 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1363,7 +1363,7 @@ pg_stat_get_xact_numscans(PG_FUNCTION_ARGS)
int64 result;
PgStat_TableStatus *tabentry;
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
+ if ((tabentry = find_tabstat_entry(relid, false)) == NULL)
result = 0;
else
result = (int64) (tabentry->t_counts.t_numscans);
@@ -1378,7 +1378,7 @@ pg_stat_get_xact_tuples_returned(PG_FUNCTION_ARGS)
int64 result;
PgStat_TableStatus *tabentry;
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
+ if ((tabentry = find_tabstat_entry(relid, false)) == NULL)
result = 0;
else
result = (int64) (tabentry->t_counts.t_tuples_returned);
@@ -1393,7 +1393,7 @@ pg_stat_get_xact_tuples_fetched(PG_FUNCTION_ARGS)
int64 result;
PgStat_TableStatus *tabentry;
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
+ if ((tabentry = find_tabstat_entry(relid, false)) == NULL)
result = 0;
else
result = (int64) (tabentry->t_counts.t_tuples_fetched);
@@ -1407,17 +1407,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)
+ if ((tabentry = find_tabstat_entry(relid, true)) == 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);
}
@@ -1428,17 +1422,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)
+ if ((tabentry = find_tabstat_entry(relid, true)) == 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);
}
@@ -1449,17 +1437,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)
+ if ((tabentry = find_tabstat_entry(relid, true)) == 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);
}
@@ -1471,7 +1453,7 @@ pg_stat_get_xact_tuples_hot_updated(PG_FUNCTION_ARGS)
int64 result;
PgStat_TableStatus *tabentry;
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
+ if ((tabentry = find_tabstat_entry(relid, false)) == NULL)
result = 0;
else
result = (int64) (tabentry->t_counts.t_tuples_hot_updated);
@@ -1486,7 +1468,7 @@ pg_stat_get_xact_blocks_fetched(PG_FUNCTION_ARGS)
int64 result;
PgStat_TableStatus *tabentry;
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
+ if ((tabentry = find_tabstat_entry(relid, false)) == NULL)
result = 0;
else
result = (int64) (tabentry->t_counts.t_blocks_fetched);
@@ -1501,7 +1483,7 @@ pg_stat_get_xact_blocks_hit(PG_FUNCTION_ARGS)
int64 result;
PgStat_TableStatus *tabentry;
- if ((tabentry = find_tabstat_entry(relid)) == NULL)
+ if ((tabentry = find_tabstat_entry(relid, false)) == NULL)
result = 0;
else
result = (int64) (tabentry->t_counts.t_blocks_hit);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index db9675884f..ca29c7f26e 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -641,7 +641,7 @@ extern void pgstat_twophase_postabort(TransactionId xid,
uint16 info,
extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry(Oid relid);
extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry_ext(bool shared,
Oid reloid);
-extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id);
+extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id, bool add_subtrans);
/*
