Re: shared-memory based stats collector - v66

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