Re: shared-memory based stats collector

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(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.
>

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