On 11/8/18 12:46 PM, Kyotaro HORIGUCHI wrote:
Hello. Thank you for looking this.
At Tue, 30 Oct 2018 01:49:59 +0100, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote
in <5253d750-890b-069b-031f-2a9b73e47...@2ndquadrant.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.
OK, regression tests now seem to pass without any valgrind issues.
However a quite a few extensions in contrib seem are broken now. It
seems fixing it is as simple as including the new bestatus.h next to
pgstat.h.
I'm not sure splitting the headers like this is needed, actually. It's
true we're replacing pgstat.c with something else, but it's still
related to stats, backing pg_stat_* system views etc. So I'd keep as
much of the definitions in pgstat.h, so that it's enough to include that
one header file. That would "unbreak" the extensions.
Renaming pgstat_report_* functions to bestatus_report_* seems
unnecessary to me too. The original names seem quite fine to me.
BTW the updated patches no longer apply cleanly. Apparently it got
broken since Tuesday, most likely by the pread/pwrite patch.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
