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@anarazel.de> wrote in
<20190207211008.nc3axviivmcoaluq@alap3.anarazel.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