Re: shared-memory based stats collector - v68

On Tuesday, April 5, 2022, Andres Freund <andres(at)anarazel(dot)de> wrote:

> 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?
>
>
Not that strongly; I’m good with the code as-is. Its not pervasive enough
to be hard to understand (I may ponder some code comments though) and the
system it is modeling has some legacy aspects that are more the root
problem and I don’t want to touch those here for sure.

>
> 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 &.

Thanks.

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

Yeah, I compiled but tried to run the tests and learned I still need to
figure out my setup for make check; then I forgot to make install…

It served its purpose at least.

>
> > @@ -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).
>
>
Thanks.

David J.