> 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
