Re: shared-memory based stats collector

Hello. Thank you for looking this.

At Tue, 30 Oct 2018 01:49:59 +0100, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote in <5253d750-890b-069b-031f-2a9b73e47832(at)2ndquadrant(dot)com>
> Hi,
>
> I've started looking at the patch over the past few days. I don't have
> any deep insights at this point, but there seems to be some sort of
> issue in pgstat_update_stat. When building using gcc, I do get this
> warning:
>
> pgstat.c: In function ‘pgstat_update_stat’:
> pgstat.c:648:18: warning: ‘now’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> oldest_pending = now;
> ~~~~~~~~~~~~~~~^~~~~
> PostgreSQL installation complete.

Uggh! The reason for the code is "last_report = now" comes later
than the code around... Fixed.

> When running this under valgrind, I get a couple of warnings in this
> area of code - see the attached log with a small sample. Judging by
> the locations I assume those are related to the same issue, but I have
> not looked into that.

There was several typos/thinkos related to pointers modifed from
original variables. There was a code like the following in the
original code.

memset(&shared_globalStats, 0, siazeof(shared_globalStats));

It was not fixed despite this patch changes the type of the
variable from PgStat_GlboalStats to (PgStat_GlobalStats *). As
the result major part of the varialbe remaineduninitialized.

I re-ran this version on valgrind and I didn't see such kind of
problem. Thank you for the testing.

I refactored the patch into complete set consists of 8 files.

v7-0001-sequential-scan-for-dshash.patch
- dshash sequential scan feature

v7-0002-Add-conditional-lock-feature-to-dshash.patch
- dshash contitional lock feature

v7-0003-Shared-memory-based-stats-collector.patch
- Shared memory base stats collector.
- Remove stats collector process
- Change stats collector to shared memory base
(This needs 0004 to work, but it is currently separate from
this for readability)

v7-0004-Make-archiver-process-an-auxiliary-process.patch
- Archiver process needs to touch shared memory.
(I didn't check EXEC_BACKEND case)

v7-0005-Let-pg_stat_statements-not-to-use-PG_STAT_TMP_DIR.patch
- I removed pg_stat_tmp directory so move pg_stat_statements
file stored there to pg_stat directory. (This would need fix)

v7-0006-Remove-pg_stat_tmp-exclusion-from-pg_rewind.patch
- For the same reason with 0005.

v7-0007-Documentation-update.patch
- Removes description related to pg_stat_tmp.

v7-0008-Split-out-backend-status-monitor-part-from-pgstat.patch
- Just refactoring. Splits the current postmaster/pgstat.c into
two files statmon/pgstat.c and statmon/bestatus.c.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center