Re: Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

On 05/31/2016 07:24 PM, Tom Lane wrote:
> Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
>> On 05/31/2016 06:59 PM, Tom Lane wrote:
>>> I'm confused here --- are you speaking of having removed
>>> if (msg->cutoff_time > req->request_time)
>>> req->request_time = msg->cutoff_time;
>>> ? That is not a check for clock skew, it's intending to be sure that
>>> req->request_time reflects the latest request for this DB when we've
>>> seen more than one request. But since req->request_time isn't
>>> actually being used anywhere, it's useless code.
>
>> Ah, you're right. I've made the mistake of writing the e-mail before
>> drinking any coffee today, and I got distracted by the comment change.
>
>>> I reformatted the actual check for clock skew, but I do not think I
>>> changed its behavior.
>
>> I'm not sure it does not change the behavior, though. request_time only
>> became unused after you removed the two places that set the value (one
>> of them in the clock skew check).
>
> Well, it's unused in the sense that the if-test quoted above is the only
> place in HEAD that examines the value of request_time. And since that
> if-test only controls whether we change the value, and not whether we
> proceed to make the clock skew check, I don't see how it's related
> to clock skew or indeed anything else at all.

I see, in that case it indeed is useless.

I've checked how this worked in 9.2 (before the 9.3 patch that split the
file per db), and back then last_statsrequest (transformed to
request_time) was used to decide whether we need to write something. But
now we do that by simply checking whether the list is empty.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services