Re: Better way of dealing with pgstat wait timeout during buildfarm runs?

On 25.12.2014 22:16, Tom Lane wrote:
> Tomas Vondra <tv(at)fuzzy(dot)cz> writes:
>> On 25.12.2014 20:36, Tom Lane wrote:
>>> BTW, I notice that in the current state of pgstat.c, all the logic
>>> for keeping track of request arrival times is dead code, because
>>> nothing is actually looking at DBWriteRequest.request_time.
>
>> Really? Which part of the code is dead? I see pgstat_recv_inquiry() is
>> updating the request_time after receiving the inquiry, and
>> pgstat_db_requested() is looking at it when writing the files.
>
> Where is pgstat_db_requested() looking at request_time?

Oh, right. pgstat_db_requested() is not looking at the timestamp - it
only checks the OID of the database. But pgstat_recv_inquiry() is
checking it, comparing it to cutoff_time etc.

ISTM the main change related to this is that this:

if (last_statwrite < last_statrequest)
pgstat_write_statsfile(false);

got replaced by this:

if (pgstat_write_statsfile_needed())
pgstat_write_statsfiles(false, false);

where pgstat_write_statsfile_needed() only checks if the list is empty
(and ignores the request/write timestamps).

If I understand that correctly, this can would lead to writing the files
more often, and we're dealing with a timeout.

>> If we can simplify the code by keeping just OIDs, let's do that. I think
>> the main reason why we haven't done that in 187492b6 was to keep as much
>> of the existing logic (and maybe change it in a separate patch).
>
> The real point here is that I think that commit *already* changed
> the existing logic, because the time-of-receipt used to matter. In
> particular, there used to be a throttle on how often the stats file
> could get written, which seems to have vanished. I seriously doubt
> that that was a good change, especially on
> write-bandwidth-challenged platforms.

Yes - if that change causes writing the files being written more
frequently, it's not a good change.

But I think the time-of-receipt still matters - pgstat_recv_inquiry
logic remained the same, just applied per database. ISTM the only thing
that disappeared is the (last_statwrite < last_statrequest) check.

I have to think about this a bit more, I haven't seen this code since
the split patch.

Tomas