Re: shared-memory based stats collector

Arthur, Andres and Robert, thank you all for the comment, and
sorry for being late.

At Wed, 6 Mar 2019 14:43:32 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190306224332(dot)374sdf6hsjh27m7t(at)alap3(dot)anarazel(dot)de>
> > DSM is inhibited to be used on postmaster. Shared memory baesd stats
> > collector needs it to work on postmaster and no problem found to do
> > that. Just allow it.
>
> Maybe I'm missing something, but why? postmaster doesn't actually need
> to process stats messages in any way?

Agreed. I should have do the initial work in the startup process.
Thank you Robert for the detailed explanation.

Done in the attached patch. Each backend craete/destroys shared
memory and loads/writers stats file. Startup process firstly
creates shared stats and destroys at its end. The first backend
after that again loads the file and craetes shared memory. It is
saved and destroys at server stop. This is because I didn't pin
the dsm underlying to dsa, which could be a cause of troubles.

> > From 774b1495136db1ad6d174ab261487fdf6cb6a5ed Mon Sep 17 00:00:00 2001
> > From: Kyotaro Horiguchi <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > Date: Thu, 21 Feb 2019 12:44:56 +0900
> > Subject: [PATCH 5/6] Shared-memory based stats collector
> >
> > Previously activity statistics is shared via files on disk. Every
> > backend sends the numbers to the stats collector process via a socket.
> > It makes snapshots as a set of files on disk with a certain interval
> > then every backend reads them as necessary. It worked fine for
> > comparatively small set of statistics but the set is under the
> > pressure to growing up and the file size has reached the order of
> > megabytes. To deal with larger statistics set, this patch let backends
> > directly share the statistics via shared memory.
>
> Btw, you can make the life of a committer easier by collecting the
> reviewers and co-authors of a patch yourself...
>
>
> This desparately needs an introductory comment in pgstat.c or such
> explaining how the new scheme works.

Mmm. I'm not sure what you want exactly but this behaves a bit
complex way than the previous version, I added a description that
explains that in the comment at the beginning of pgstat.c. I'm
very happy if you give me a little more information about what
you want.

> > +LWLock StatsMainLock;
> > +#define StatsLock (&StatsMainLock)
>
> Wait, what? You can't just define a lock this way. That's process local
> memory, locking that doesn't do anything useful.

Oops.. I don't understand why I did that. Moved to StatsShmemStruct.

> > +/* Shared stats bootstrap information */
> > +typedef struct StatsShmemStruct {
>
> Please note that in PG's coding style the { comes in the next line.

Fixed.

> > +/*
> > + * Backends store various database-wide info that's waiting to be flushed out
> > + * to shared memory in these variables.
> > + */
> > +static int n_deadlocks = 0;
...
> > +static int n_conflict_bufferpin = 0;
> > +static int n_conflict_startup_deadlock = 0;
>
> Probably worthwhile to group those into a struct, even just to make
> debugging easier.

Done. Moved to BackendDBStats. But didn't for pgStatXact* and
pgStatBlock* to make the patch smaller.

> > -void
> > -pgstat_init(void)
> > +static void
> > +pgstat_postmaster_shutdown(int code, Datum arg)
>
> You can't have a function like that without explaining why it's there.
>
> > + /* trash the stats on crash */
> > + if (code == 0)
> > + pgstat_write_statsfiles();
> > }
>
> And especially not without documenting what that code is supposed to
> mean.

Added comments to all functions.

> > pgstat_report_stat(bool force)
> > {
...
> > + bool have_other_stats = false;
..
> > + /* Do we have anything to flush? */
> > + if (have_recovery_conflicts || n_deadlocks != 0 || n_tmpfiles != 0)
> > + have_other_stats = true;
..
> "other" seems like a pretty mysterious category. Seems better to either
> name precisely, or just use the underlying variables for checks.

Agreed. It is mainly database-wide stats so I named it
dbstats. To move the expression near to the definitions of the
used variables I defined two macros, HAVE_PENDING_DBSTATS() and
HAVE_PENDING_CONFLICTS().

> > +/* -------
> > + * Subroutines for pgstat_flush_stat.
> > + * -------
> > + */
> > +
> > +/*
> > + * snapshot_statentry() - Find an entry from source dshash with cache.
> > + *
>
> Is snapshot_statentry() really a subroutine for pgstat_flush_stat()?

No. Sorry, it is just misplaced. Fixed.

> > +static void *
> > +snapshot_statentry(pgstat_snapshot_cxt *cxt, Oid key)
..
> > + * Create new hash with arbitrary initial entries since we don't know how
> > + * this hash will grow. The boolean put at the end of the entry is
> > + * negative flag.
> > */
>
> That, uh, seems pretty ugly and hard to understand.

Ok. I introduced common struct PgStat_snapshot_head to hold the
negative flag.

> > +/*
> > + * pgstat_flush_stat: Flushes table stats out to shared statistics.
> > + *
> > + * If nowait is true, returns with false if required lock was not acquired
>
> s/with false/false/

Fixed. Thanks.

> > + * immediately. In the case, infos of some tables may be left alone in TSA to
>
> TSA? I assume TabStatusArray, but I don't think that's a common or
> useful abbreviation. It'd be ok to just refer to the variable name.

Yeah I think so and I though that it's not my original but
actually is.. Replaced them with a complete word.

> > +static bool
> > +pgstat_flush_stat(pgstat_flush_stat_context *cxt, bool nowait)
> > +{
>
> > + /* try to apply the tab stats */
> > + if (!pgstat_flush_tabstat(cxt, nowait, entry))
> > {
> > - pgstat_send_tabstat(this_msg);
> > - this_msg->m_nentries = 0;
> > + /*
> > + * Failed. Leave it alone filling at the beginning in TSA.
> > + */
...

> This seems a lot of work for just leaving an entry around to be
> processed later. Shouldn't code for that already exist elsewhere?

No. TabStatusArray was alweays emptied after sending stats data
to collector. So the packing (entry moving) code is a brand new
stuff.

> > void
> > pgstat_vacuum_stat(void)
...
> > - backend_read_statsfile();
> > + /* If not done for this transaction, take a snapshot of stats */
> > + pgstat_snapshot_global_stats();
>
> Hm, why do we need a snapshot here?

Thank you for pointing out. It used to be needed during
development but no longer needed. Removed.

> > /*
> > * Now repeat the above steps for functions. However, we needn't bother
> > * in the common case where no function stats are being collected.
> > */
>
> Can't we move the act of iterating through these hashes and probing
> against another hash into a helper function and reuse? These
> duplications aren't pretty.

Like snapshot_statentry? The first member of both
PgStat_StatTab/FuncEntry is Oid. So just accessing the hash entry
as Oid makes it possible. There's no point in providing common
header struct as I did for backend snapshot.

pgstat_remove_useless_entries() is that.

At Mon, 25 Feb 2019 14:27:13 +0300, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote in <fb4ca586-c3f0-7c2f-ca2e-2aa49b66d146(at)postgrespro(dot)ru>
> Thank you. Still there are couple TAP-test which don't pass:
> 002_archiving.pl and 010_pg_basebackup.pl. I think the next simple
> patch solves the issue:

I accidentially removed it in 0004. Fixed it. Thanks!

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center