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

Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)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.

I reformatted the actual check for clock skew, but I do not think I
changed its behavior.

> 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.

regards, tom lane