public inbox for [email protected]
help / color / mirror / Atom feedFrom: Drouvot, Bertrand <[email protected]>
To: Andres Freund <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Subject: Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
Date: Mon, 6 Mar 2023 08:33:15 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
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
>> --- 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.
>> + *
>
> 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?
>
Good point. Wording has been changed in V6 attached.
> Otherwise this looks ready, on a casual scan.
>
Thanks for having looked at it!
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..c030e1782e 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. 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 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 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;
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);
}
@@ -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;
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);
}
@@ -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;
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);
}
Attachments:
[text/plain] v6-0001-find_tabstat_entry_recon.patch (3.9K, ../[email protected]/2-v6-0001-find_tabstat_entry_recon.patch)
download | inline diff:
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index f793ac1516..c030e1782e 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. 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 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 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;
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);
}
@@ -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;
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);
}
@@ -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;
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);
}
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox