> I was referring to the order of the fields in the structure itself,
> but that's no big deal one way or the other.
I understand your point now. I will group them with the
related counters in the next rev and will use
> This should be one comment for the whole block, or this should use the
> singular for each comment.
I will use a singular "/* time in milliseconds */" comment for each
new field.
This existing write_time field is plural, but I will leave
that one alone for now.
PgStat_Counter write_time; /* times in milliseconds */
> On HEAD in the ANALYZE path, the endtime is calculated after
> index_vacuum_cleanup(). Your patch calculates it before
> index_vacuum_cleanup(), which would result in an incorrect calculation
> if the index cleanup takes a long time with less logs generated, no?
> Sorry for not noticing that earlier on the thread.. Perhaps it would
> be just better to pass the start time to pgstat_report_vacuum() and
> pgstat_report_analyze() then let their internals do the elapsed time
> calculations. Consistency of the arguments for both functions is
> something worth having, IMO, even if it means a bit more
> GetCurrentTimestamp() in this case.
So currently, the report of the last_(autoanalyze|analyze)_time
is set before the index_vacuum_cleanup, but for logging purposes
the elapsed time is calculated afterwards. Most users will not notice
this, but I think that is wrong as well.
I think we should calculate endtime and elapsedtime and call
pgstat_report_analyze after the index_vacuum_cleanup; and
before vac_close_indexes. This is more accurate and will
avoid incurring the extra GetCurrentTimestamp() call.
What do you think?
Regards,
Sami
