On Wed, Feb 22, 2012 at 2:11 PM, Peter Geoghegan <peter@2ndquadrant.com> wrote:
> One beef that I have with the variable name m_write_ms is that "ms"
> could equally well refer to microseconds or milliseconds, and these
> mistakes are very common.
I would expect ms to mean milliseconds and us to mean microseconds.
> Details of existing comment bug: The docs show that
> pg_stat_*_functions accumulates time in units of milliseconds.
> However, a PgStat_FunctionEntry payload is commented as sending the
> time/self time to the stats collector in microseconds. So this
> comment, in the existing code, is actually wrong:
>
> PgStat_Counter f_time; /* times in microseconds */
> PgStat_Counter f_time_self;
> } PgStat_StatFuncEntry;
I think that the comment is not wrong. The code that populates it
looks like this:
m_ent->f_time = INSTR_TIME_GET_MICROSEC(entry->f_counts.f_time); m_ent->f_time_self =
INSTR_TIME_GET_MICROSEC(entry->f_counts.f_time_self
If that's not producing microseconds, those macros are badly misnamed.Note that the pg_stat_user_functions view divides
thereturn value of
the function by 1000, which is why the view output is in milliseconds.
> As for the substance of the patch, I am in general agreement that this
> is a good idea. Storing the statistics in pg_stat_bgwriter is a more
> flexible approach than is immediately apparent. Users can usefully
> calculate delta values, as part of the same process through which
> checkpoint tuning is performed by comparing output from "select now(),
> * from pg_stat_bgwriter" at different intervals. This also puts this
> information within easy reach of monitoring tools.
On further thought, I'll revise the position I took upthread and
concede that write_ms and sync_ms are useful. Given values of the
stats counters at time A and time B, you can calculate what percentage
of that time was spent in the write phase of some checkpoint, the sync
phase of some checkpoint, and not in any checkpoint. But I'm still
pretty sketchy on sync_files. I can't see how you can do anything
useful with that.
> So, I'd ask Greg and/or Jaime to produce a revision of the patch with
> those concerns in mind, as well as fixing the md.c usage of
> log_checkpoints.
Seems we still are waiting for an update of this patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
