Re: shared-memory based stats collector - v66

At Thu, 24 Mar 2022 13:21:33 -0400, Melanie Plageman <melanieplageman@gmail.com> wrote in 
> On Thu, Mar 17, 2022 at 3:36 AM Andres Freund <andres@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.

Thanks!

+        .reset_timestamp_cb = pgstat_shared_reset_timestamp_noop,

(I once misunderstood that the "shared" means shared memory area..)

The reset function is type-specific and it must be set.  So don't we
provide all to-be-required reset functions?


+    if (pgstat_shared_ref_get(kind, dboid, objoid, false, NULL))
+    {
+        Oid msg_oid = (kind == PGSTAT_KIND_DB) ? dboid : objoid;

Explicitly using PGSTAT_KIND_DB here is a kind of annoyance.  Since we
always give InvalidOid correctly as the parameters, and objoid alone
is not specific enough, do we warn using both dboid and objoid without
a special treat?

Concretely, I propose to do the following instead.

+    if (pgstat_shared_ref_get(kind, dboid, objoid, false, NULL))
+    {
+        ereport(WARNING,
+                errmsg("resetting existing stats for type %s, db=%d, oid=%d",
+                      pgstat_kind_info_for(kind)->name, dboid, objoid);                



+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);
+

"kind" is used only in assertion, which requires PG_USED_FOR_ASSERTS_ONLY.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center