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

On 05/31/2016 06:59 PM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> On 05/26/2016 10:10 PM, Tom Lane wrote:
>>> I posted a patch at
>>> https://www.postgresql.org/message-id/13023.1464213041@sss.pgh.pa.us
>>> which I think is functionally equivalent to what you have here, but
>>> it goes to some lengths to make the code more readable, whereas this
>>> is just adding another layer of complication to something that's
>>> already a mess (eg, the request_time field is quite useless as-is).
>>> So I'd like to propose pushing that in place of this patch ... do you
>>> care to review it first?
>
>> I've reviewed the patch today, and it seems fine to me - correct and
>> achieving the same goal as the patch posted to this thread (plus fixing
>> the issue with shared catalogs and fixing many comments).
>
> Thanks for reviewing!
>
>> FWIW do you still plan to back-patch this? Minimizing the amount of
>> changes was one of the things I had in mind when writing "my" patch,
>> which is why I ended up with parts that are less readable.
>
> Yeah, I think it's a bug fix and should be back-patched.  I'm not in
> favor of making things more complicated just to reduce the number of
> lines a patch touches.
>
>> The one change I'm not quite sure about is the removal of clock skew
>> detection in pgstat_recv_inquiry(). You've removed the first check on
>> the inquiry, replacing it with this comment:
>>      It seems sufficient to check for clock skew once per write round.
>> But the first check was comparing msg/req, while the second check looks
>> at dbentry/cur_ts. I don't see how those two clock skew check are
>> redundant - if they are, the comment should explain that I guess.
>
> 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).

I'm not sure this is a bad change, though. But there was a dependency 
between the new request and the preceding one.

>
>> Another thing is that if you believe merging requests across databases
>> is a silly idea, maybe we should bite the bullet and replace the list of
>> requests with a single item. I'm not convinced about this, though.
>
> No, I don't want to do that either.  We're not spending much code by
> having pending_write_requests be a list rather than a single entry,
> and we might eventually figure out a reasonable way to time the flushes
> so that we can merge requests.
>

+1

regards

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