Re: shared-memory based stats collector

Hi,

On 2019-02-15 17:29:00 +0900, Kyotaro HORIGUCHI wrote:
> At Thu, 7 Feb 2019 13:10:08 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190207211008(dot)nc3axviivmcoaluq(at)alap3(dot)anarazel(dot)de>
> > Hi,
> >
> > On 2018-11-12 20:10:42 +0900, Kyotaro HORIGUCHI wrote:
> > > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> > > index 7eed5866d2..e52ae54821 100644
> > > --- a/src/backend/access/transam/xlog.c
> > > +++ b/src/backend/access/transam/xlog.c
> > > @@ -8587,9 +8587,9 @@ LogCheckpointEnd(bool restartpoint)
> > > &sync_secs, &sync_usecs);
> > >
> > > /* Accumulate checkpoint timing summary data, in milliseconds. */
> > > - BgWriterStats.m_checkpoint_write_time +=
> > > + BgWriterStats.checkpoint_write_time +=
> > > write_secs * 1000 + write_usecs / 1000;
> > > - BgWriterStats.m_checkpoint_sync_time +=
> > > + BgWriterStats.checkpoint_sync_time +=
> > > sync_secs * 1000 + sync_usecs / 1000;
> >
> > Why does this patch do renames like this in the same entry as actual
> > functional changes?
>
> Just because it is no longer "messages". I'm ok to preserve them
> as historcal names. Reverted.

It's fine to do such renames, just do them as separate patches. It's
hard enough to review changes this big...

> > > /*
> > > * Structures in which backends store per-table info that's waiting to be
> > > @@ -189,18 +189,14 @@ typedef struct TabStatHashEntry
> > > * Hash table for O(1) t_id -> tsa_entry lookup
> > > */
> > > static HTAB *pgStatTabHash = NULL;
> > > +static HTAB *pgStatPendingTabHash = NULL;
> > >
> > > /*
> > > * Backends store per-function info that's waiting to be sent to the collector
> > > * in this hash table (indexed by function OID).
> > > */
> > > static HTAB *pgStatFunctions = NULL;
> > > -
> > > -/*
> > > - * Indicates if backend has some function stats that it hasn't yet
> > > - * sent to the collector.
> > > - */
> > > -static bool have_function_stats = false;
> > > +static HTAB *pgStatPendingFunctions = NULL;
> >
> > So this patch leaves us with a pgStatFunctions that has a comment
> > explaining it's about "waiting to be sent" stats, and then additionally
> > a pgStatPendingFunctions?
>
> Mmm. Thanks . I changed the comment and separated pgSTatPending*
> stuff from there and merged with pgstat_pending_*. And unified
> the naming.

I think my point is larger than that - I don't see why the pending
hashtables are needed at all. They seem purely superflous.

> >
> > > + if (cxt->tabhash)
> > > + dshash_detach(cxt->tabhash);
> >
> > Huh, why do we detach here?
>
> To release the lock on cxt->dbentry. It may be destroyed.

Uh, how?

> - Separte shared database stats from db_stats hash.
>
> - Consider relaxing dbentry locking.
>
> - Try removing pgStatPendingFunctions
>
> - ispell on it.

Additionally:
- consider getting rid of all the pending stuff, not just for functions,
- as far as I can tell it's unnecessary

Thanks,

Andres