Re: shared-memory based stats collector

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: tomas(dot)vondra(at)2ndquadrant(dot)com
Cc: andres(at)anarazel(dot)de, ah(at)cybertec(dot)at, magnus(at)hagander(dot)net, robertmhaas(at)gmail(dot)com, tgl(at)sss(dot)pgh(dot)pa(dot)us, pgsql-hackers(at)postgresql(dot)org
Subject: Re: shared-memory based stats collector
Date: 2018-11-08 11:46:48
Message-ID: 20181108.204648.45949674.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v7-0001-sequential-scan-for-dshash.patch text/x-patch 10.6 KB
v7-0002-Add-conditional-lock-feature-to-dshash.patch text/x-patch 4.1 KB
v7-0003-Shared-memory-based-stats-collector.patch text/x-patch 212.4 KB
v7-0004-Make-archiver-process-an-auxiliary-process.patch text/x-patch 11.9 KB
v7-0005-Let-pg_stat_statements-not-to-use-PG_STAT_TMP_DIR.patch text/x-patch 1.9 KB
v7-0006-Remove-pg_stat_tmp-exclusion-from-pg_rewind.patch text/x-patch 1.1 KB
v7-0007-Documentation-update.patch text/x-patch 6.1 KB
v7-0008-Split-out-backend-status-monitor-part-from-pgstat.patch text/x-patch 237.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2018-11-08 11:54:51 Re: PostgreSQL Limits and lack of documentation about them.
Previous Message Kyotaro HORIGUCHI 2018-11-08 11:42:22 Re: shared-memory based stats collector