From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: andres@anarazel.de
Cc: ah@cybertec.at
Cc: magnus@hagander.net
Cc: robertmhaas@gmail.com
Cc: tgl@sss.pgh.pa.us
Cc: pgsql-hackers@postgresql.org
Subject: Re: shared-memory based stats collector
Date: Thu, 8 Nov 2018 16:06:49 +0100
Message-ID: <de249c3f-79c9-b75c-79a3-5e2d008548a8@2ndquadrant.com> (raw)
In-Reply-To: <20181108.204648.45949674.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20181002.160651.117284090.horiguchi.kyotaro@lab.ntt.co.jp>
<20181005.173017.151015949.horiguchi.kyotaro@lab.ntt.co.jp>
<5253d750-890b-069b-031f-2a9b73e47832@2ndquadrant.com>
<20181108.204648.45949674.horiguchi.kyotaro@lab.ntt.co.jp>
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.vondra@2ndquadrant.com> wrote in <5253d750-890b-069b-031f-2a9b73e47832@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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, horiguchi.kyotaro@lab.ntt.co.jp, andres@anarazel.de, ah@cybertec.at, magnus@hagander.net, robertmhaas@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: shared-memory based stats collector
In-Reply-To: <de249c3f-79c9-b75c-79a3-5e2d008548a8@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox