On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <and...@anarazel.de> wrote:
>
> The biggest todos are:
> - Address all the remaining AFIXMEs and XXXs
Attached is a patch that addresses three of the existing AFIXMEs.
From 2a975cdb5d10ec365ca2ced39b9f99a9385b6268 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 23 Mar 2022 19:43:12 -0400
Subject: [PATCH] Address 3 AFIXMEs
- reset timestamp callback
- schedule stat internl warn and reset if exists
- pending delete callback
---
src/backend/utils/activity/pgstat.c | 117 +++++++++---------
src/backend/utils/activity/pgstat_database.c | 7 ++
src/backend/utils/activity/pgstat_relation.c | 16 +++
.../utils/activity/pgstat_subscription.c | 7 ++
src/include/utils/pgstat_internal.h | 16 +++
5 files changed, 105 insertions(+), 58 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 7f86bc29ee..9841447a8b 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -154,10 +154,8 @@ static void pgstat_shared_refs_release_all(void);
static void pgstat_perform_drop(xl_xact_stats_item *drop);
static bool pgstat_drop_stats_entry(dshash_seq_status *hstat);
-static void pgstat_shared_reset_timestamp(PgStatKind kind, PgStatShm_StatEntryHeader *header);
static void pgstat_reset_all_stats(TimestampTz ts);
-static void pgstat_pending_delete(PgStatSharedRef *shared_ref);
static bool pgstat_pending_flush_stats(bool nowait);
static PgStatKind pgstat_kind_for(char *kind_str);
@@ -281,6 +279,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_StatDBEntry),
.flush_pending_cb = pgstat_flush_database,
+ .pending_delete_cb = pgstat_pending_delete,
+ .reset_timestamp_cb = pgstat_database_reset_timestamp,
},
[PGSTAT_KIND_TABLE] = {
@@ -294,6 +294,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_TableStatus),
.flush_pending_cb = pgstat_flush_relation,
+ .pending_delete_cb = pgstat_relation_pending_delete,
+ .reset_timestamp_cb = pgstat_shared_reset_timestamp_noop,
},
[PGSTAT_KIND_FUNCTION] = {
@@ -307,6 +309,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_BackendFunctionEntry),
.flush_pending_cb = pgstat_flush_function,
+ .pending_delete_cb = pgstat_pending_delete,
+ .reset_timestamp_cb = pgstat_shared_reset_timestamp_noop,
},
[PGSTAT_KIND_SUBSCRIPTION] = {
@@ -322,6 +326,8 @@ static const PgStatKindInfo pgstat_kind_infos[PGSTAT_KIND_LAST + 1] = {
.pending_size = sizeof(PgStat_BackendSubEntry),
.flush_pending_cb = pgstat_flush_subscription,
+ .pending_delete_cb = pgstat_pending_delete,
+ .reset_timestamp_cb = pgstat_subscription_reset_timestamp,
},
@@ -1040,12 +1046,18 @@ pgstat_schedule_stat_internal(PgStatKind kind, Oid dboid, Oid objoid, bool is_cr
void
pgstat_schedule_stat_create(PgStatKind kind, Oid dboid, Oid objoid)
{
- pgstat_schedule_stat_internal(kind, dboid, objoid, /* create */ true);
+ if (pgstat_shared_ref_get(kind, dboid, objoid, false, NULL))
+ {
+ Oid msg_oid = (kind == PGSTAT_KIND_DB) ? dboid : objoid;
- /*
- * AFIXME: It would be a good idea to check if an object with that key
- * already exists. WARN if so, and reset the stats to 0.
- */
+ ereport(WARNING,
+ errmsg("Resetting existing stats for %s with OID %d.",
+ (pgstat_kind_info_for(kind))->name, msg_oid));
+
+ pgstat_reset_one(kind, dboid, objoid);
+ }
+
+ pgstat_schedule_stat_internal(kind, dboid, objoid, /* create */ true);
}
/*
@@ -2178,6 +2190,28 @@ pgstat_shared_refs_release_all(void)
* ------------------------------------------------------------
*/
+/*
+ * Delete pending stats for variable number stats in the general case. Some
+ * types of stats require additional steps and have dedicated callbacks.
+ */
+void
+pgstat_pending_delete(PgStatSharedRef *shared_ref)
+{
+ void *pending_data = shared_ref->pending;
+ PgStatKind kind = shared_ref->shared_entry->key.kind;
+
+ Assert(pending_data != NULL);
+ Assert(!pgstat_kind_info_for(kind)->fixed_amount);
+
+ /* PGSTAT_KIND_TABLE has its own callback */
+ Assert(kind != PGSTAT_KIND_TABLE);
+
+ pfree(pending_data);
+ shared_ref->pending = NULL;
+
+ dlist_delete(&shared_ref->pending_node);
+}
+
/*
* Returns the appropriate PgStatSharedRef, preparing it to receive pending
* stats if not already done.
@@ -2222,38 +2256,6 @@ pgstat_pending_fetch(PgStatKind kind, Oid dboid, Oid objoid)
return shared_ref;
}
-static void
-pgstat_pending_delete(PgStatSharedRef *shared_ref)
-{
- void *pending_data = shared_ref->pending;
- PgStatKind kind;
-
- Assert(pending_data != NULL);
-
- /* AFIXME: Move into a PgStatKindInfo callback */
- kind = shared_ref->shared_entry->key.kind;
- switch (kind)
- {
- case PGSTAT_KIND_TABLE:
- pgstat_relation_unlink(((PgStat_TableStatus *) pending_data)->relation);
- break;
- case PGSTAT_KIND_DB:
- case PGSTAT_KIND_FUNCTION:
- case PGSTAT_KIND_SUBSCRIPTION:
- break;
- default:
- /* !fixed_amount stats should be handled explicitly */
- Assert(pgstat_kind_info_for(kind)->fixed_amount);
- elog(ERROR, "unexpected");
- break;
- }
-
- pfree(pending_data);
- shared_ref->pending = NULL;
-
- dlist_delete(&shared_ref->pending_node);
-}
-
/*
* Flush out pending stats for database objects (databases, relations,
* functions).
@@ -2288,6 +2290,7 @@ pgstat_pending_flush_stats(bool nowait)
Assert(!kind_info->fixed_amount);
Assert(kind_info->flush_pending_cb != NULL);
+ Assert(kind_info->pending_delete_cb != NULL);
/* flush the stats, if possible */
remove = kind_info->flush_pending_cb(shared_ref, nowait);
@@ -2302,7 +2305,7 @@ pgstat_pending_flush_stats(bool nowait)
/* if successfully flushed, remove entry */
if (remove)
- pgstat_pending_delete(shared_ref);
+ kind_info->pending_delete_cb(shared_ref);
else
have_pending = true;
@@ -2438,11 +2441,16 @@ pgstat_perform_drop(xl_xact_stats_item *drop)
{
PgStatShmHashEntry *shent;
PgStatHashKey key;
+ const PgStatKindInfo *kind_info;
key.kind = drop->kind;
key.dboid = drop->dboid;
key.objoid = drop->objoid;
+ kind_info = pgstat_kind_info_for(drop->kind);
+ Assert(!kind_info->fixed_amount);
+ Assert(kind_info->pending_delete_cb != NULL);
+
if (pgStatSharedRefHash)
{
PgStatSharedRefHashEntry *lohashent;
@@ -2452,7 +2460,7 @@ pgstat_perform_drop(xl_xact_stats_item *drop)
if (lohashent)
{
if (lohashent->shared_ref && lohashent->shared_ref->pending)
- pgstat_pending_delete(lohashent->shared_ref);
+ kind_info->pending_delete_cb(lohashent->shared_ref);
pgstat_shared_ref_release(lohashent->key, lohashent->shared_ref);
}
@@ -2498,9 +2506,12 @@ pgstat_perform_drop(xl_xact_stats_item *drop)
static void
pgstat_shared_stat_reset_one(PgStatKind kind, PgStatShm_StatEntryHeader *header)
{
+ const PgStatKindInfo *kind_info = pgstat_kind_info_for(kind);
+
memset(shared_stat_entry_data(kind, header), 0,
shared_stat_entry_len(kind));
- pgstat_shared_reset_timestamp(kind, header);
+
+ kind_info->reset_timestamp_cb(header);
}
/*
@@ -2555,26 +2566,16 @@ pgstat_reset_matching(bool (*do_reset) (PgStatShmHashEntry *))
dshash_seq_term(&hstat);
}
-static void
-pgstat_shared_reset_timestamp(PgStatKind kind, PgStatShm_StatEntryHeader *header)
+/*
+ * For variable number stats with no stats reset timestamp member
+ */
+void
+pgstat_shared_reset_timestamp_noop(PgStatShm_StatEntryHeader *header)
{
- /* AFIXME: Move into a PgStatKindInfo callback */
- switch (kind)
- {
- case PGSTAT_KIND_DB:
- ((PgStatShm_StatDBEntry *) header)->stats.stat_reset_timestamp =
- GetCurrentTimestamp();
- break;
- case PGSTAT_KIND_SUBSCRIPTION:
- ((PgStatShm_StatSubEntry *) header)->stats.stat_reset_timestamp =
- GetCurrentTimestamp();
- break;
- default:
- return;
- }
}
+
/* ------------------------------------------------------------
* Fetching of stats
* ------------------------------------------------------------
diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c
index 49037d345a..92ef8f1974 100644
--- a/src/backend/utils/activity/pgstat_database.c
+++ b/src/backend/utils/activity/pgstat_database.c
@@ -319,6 +319,13 @@ pgstat_should_report_connstat(void)
return MyBackendType == B_BACKEND;
}
+void
+pgstat_database_reset_timestamp(PgStatShm_StatEntryHeader *header)
+{
+ ((PgStatShm_StatDBEntry *) header)->stats.stat_reset_timestamp =
+ GetCurrentTimestamp();
+}
+
/*
* pgstat_flush_db - flush out a local database stats entry
*
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index a96760f5b1..e3eb960659 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -885,6 +885,22 @@ pgstat_flush_relation(PgStatSharedRef *shared_ref, bool nowait)
return true;
}
+void
+pgstat_relation_pending_delete(PgStatSharedRef *shared_ref)
+{
+ void *pending_data = shared_ref->pending;
+
+ Assert(pending_data != NULL);
+
+ pgstat_relation_unlink(((PgStat_TableStatus *) pending_data)->relation);
+
+ pfree(pending_data);
+ shared_ref->pending = NULL;
+
+ dlist_delete(&shared_ref->pending_node);
+}
+
+
/*
* Find or create a PgStat_TableStatus entry for rel. New entry is created and
* initialized if not exists.
diff --git a/src/backend/utils/activity/pgstat_subscription.c b/src/backend/utils/activity/pgstat_subscription.c
index f7f5698c5d..5882016177 100644
--- a/src/backend/utils/activity/pgstat_subscription.c
+++ b/src/backend/utils/activity/pgstat_subscription.c
@@ -43,6 +43,13 @@ pgstat_reset_subscription_counter(Oid subid)
pgstat_reset_one(PGSTAT_KIND_SUBSCRIPTION, InvalidOid, subid);
}
+void
+pgstat_subscription_reset_timestamp(PgStatShm_StatEntryHeader *header)
+{
+ ((PgStatShm_StatSubEntry *) header)->stats.stat_reset_timestamp =
+ GetCurrentTimestamp();
+}
+
/* ----------
* pgstat_report_subscription_error() -
*
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index b312993d1f..718e848866 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -193,6 +193,16 @@ typedef struct PgStatKindInfo
*/
bool (*flush_pending_cb) (PgStatSharedRef *sr, bool nowait);
+ /*
+ * For variable number stats: delete pending stats.
+ */
+ void (*pending_delete_cb) (PgStatSharedRef *sr);
+
+ /*
+ * For variable number stats: reset the reset timestamp.
+ */
+ void (*reset_timestamp_cb) (PgStatShm_StatEntryHeader *header);
+
/*
* For global statistics: Reset All.
*/
@@ -390,7 +400,9 @@ extern PgStatSharedRef *pgstat_shared_stat_locked(PgStatKind kind,
bool nowait);
extern void pgstat_reset_one(PgStatKind kind, Oid dboid, Oid objoid);
extern void pgstat_reset_matching(bool (*do_reset) (PgStatShmHashEntry *));
+extern void pgstat_shared_reset_timestamp_noop(PgStatShm_StatEntryHeader *header);
+extern void pgstat_pending_delete(PgStatSharedRef *shared_ref);
extern PgStatSharedRef *pgstat_pending_prepare(PgStatKind kind, Oid dboid, Oid objoid, bool *created_shared);
extern PgStatSharedRef *pgstat_pending_fetch(PgStatKind kind, Oid dboid, Oid objoid);
@@ -441,6 +453,7 @@ extern void pgstat_checkpointer_snapshot_cb(void);
extern void pgstat_report_disconnect(Oid dboid);
extern void pgstat_update_dbstats(TimestampTz now);
extern void AtEOXact_PgStat_Database(bool isCommit, bool parallel);
+extern void pgstat_database_reset_timestamp(PgStatShm_StatEntryHeader *header);
extern bool pgstat_flush_database(PgStatSharedRef *shared_ref, bool nowait);
extern PgStat_StatDBEntry *pgstat_pending_db_prepare(Oid dboid);
@@ -460,7 +473,9 @@ extern void AtEOXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isC
extern void AtEOSubXact_PgStat_Relations(PgStat_SubXactStatus *xact_state, bool isCommit, int nestDepth);
extern void AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state);
extern void PostPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state);
+
extern bool pgstat_flush_relation(PgStatSharedRef *shared_ref, bool nowait);
+extern void pgstat_relation_pending_delete(PgStatSharedRef *shared_ref);
/*
@@ -497,6 +512,7 @@ extern void pgstat_wal_snapshot_cb(void);
*/
extern bool pgstat_flush_subscription(PgStatSharedRef *shared_ref, bool nowait);
+extern void pgstat_subscription_reset_timestamp(PgStatShm_StatEntryHeader *header);
/*
--
2.30.2
