Re: shared-memory based stats collector - v68

Hi,

On 2022-04-05 08:49:36 -0700, David G. Johnston wrote:
> On Mon, Apr 4, 2022 at 7:36 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> >
> > I think all this is going to achieve is to making code more complicated.
> > There
> > is a *single* non-assert use of accessed_across_databases and now a single
> > assertion involving it.
> >
> > What would having PGSTAT_KIND_CLUSTER and PGSTAT_KIND_DATABASE achieve?
> >
>
> So, I decided to see what this would look like; the results are attached,
> portions of it also inlined below.

> I'll admit this does introduce a terminology problem - but IMO these words
> are much more meaningful to the reader and code than the existing
> booleans. I'm hopeful we can bikeshed something agreeable as I'm strongly
> in favor of making this change.

Sorry, I just don't agree. I'm happy to try to make it look better, but this
isn't it.

Do you think it should be your way strongly enough that you'd not want to get
it in the current way?

> The ability to create defines for subsets nicely resolves the problem that
> CLUSTER and DATABASE (now OBJECT to avoid DATABASE conflict in PgStat_Kind)
> are generally related together - they are now grouped under the DYNAMIC
> label (variable, if you want) while all of the fixed entries get associated
> with GLOBAL. Thus the majority of usages, since accessed_across_databases
> is rare, end up being either DYNAMIC or GLOBAL.

FWIW, as-is DYNAMIC isn't correct:

> +typedef enum PgStat_KindGroup
> +{
> + PGSTAT_GLOBAL = 1,
> + PGSTAT_CLUSTER,
> + PGSTAT_OBJECT
> +} PgStat_KindGroup;
> +
> +#define PGSTAT_DYNAMIC (PGSTAT_CLUSTER | PGSTAT_OBJECT)

Oring PGSTAT_CLUSTER = 2 with PGSTAT_OBJECT = 3 yields 3 again. To do this
kind of thing the different values need to have power-of-two values, and then
the tests need to be done with &.

Nicely demonstrated by the fact that with the patch applied initdb doesn't
pass...

> @@ -909,7 +904,7 @@ pgstat_build_snapshot(void)
> */
> if (p->key.dboid != MyDatabaseId &&
> p->key.dboid != InvalidOid &&
> - !kind_info->accessed_across_databases)
> + kind_info->kind_group == PGSTAT_OBJECT)
> continue;
>
> if (p->dropped)

Imo this is far harder to interpret - !kind_info->accessed_across_databases
tells you why we're skipping in clear code. Your alternative doesn't.

> @@ -938,7 +933,7 @@ pgstat_build_snapshot(void)
> {
> const PgStat_KindInfo *kind_info = pgstat_kind_info_for(kind);
>
> - if (!kind_info->fixed_amount)
> + if (kind_info->kind_group == PGSTAT_DYNAMIC)

These all would have to be kind_info->kind_group & PGSTAT_DYNAMIC, or even
(kind_group & PGSTAT_DYNAMIC) != 0, depending on the case.

> @@ -1047,8 +1042,8 @@ pgstat_delete_pending_entry(PgStat_EntryRef *entry_ref)
> void *pending_data = entry_ref->pending;
>
> Assert(pending_data != NULL);
> - /* !fixed_amount stats should be handled explicitly */
> - Assert(!pgstat_kind_info_for(kind)->fixed_amount);
> + /* global stats should be handled explicitly : why?*/
> + Assert(pgstat_kind_info_for(kind)->kind_group == PGSTAT_DYNAMIC);

The pending data infrastructure doesn't provide a way of dealing with fixed
amount stats, and there's no PgStat_EntryRef for them (since they're not in
the hashtable).

Greetings,

Andres Freund