Subject: Re: Make autovacuum sort tables in descending order of xid_age
To: David Fetter <david@fetter.org>
Cc: PostgreSQL Development <pgsql-hackers@postgresql.org>,
 Christophe Pettus <xof@thebuild.com>
References: <20191129180138.GD8731@fetter.org>
 <20191129222152.GE8731@fetter.org>
 <2488acea-328b-1328-d437-8c156bc8f7a4@gmail.com>
 <20191130222314.GF8731@fetter.org>
From: Mark Dilger <hornschnorter@gmail.com>
Message-ID: <877ac074-e95d-bdee-09e7-c51f0ee0fde2@gmail.com>
Date: Thu, 12 Dec 2019 08:02:25 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <20191130222314.GF8731@fetter.org>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 11/30/19 2:23 PM, David Fetter wrote:
> On Sat, Nov 30, 2019 at 10:04:07AM -0800, Mark Dilger wrote:
>> On 11/29/19 2:21 PM, David Fetter wrote:
>>> On Fri, Nov 29, 2019 at 07:01:39PM +0100, David Fetter wrote:
>>>> Folks,
>>>>
>>>> Per a suggestion Christophe made, please find attached a patch to
>>>> $Subject:
>>>>
>>>> Apart from carefully fudging with pg_resetwal, and short of running in
>>>> production for a few weeks, what would be some good ways to test this?
>>>
>>> Per discussion on IRC with Sehrope Sarkuni, please find attached a
>>> patch with one fewer bug, this one in the repalloc() calls.
>>
>> Hello David,
>>
>> Here are my initial thoughts.
>>
>> Although you appear to be tackling the problem of vacuuming tables
>> with older Xids first *per database*,
> 
> Yes, that's what's come up for me in production, but lately,
> production has consisted of a single active DB maxing out hardware. I
> can see how in other situations--multi-tenant, especially--it would
> make more sense to sort the DBs first.

I notice you don't address that in your latest patch.  Do you have
any thoughts on whether that needs to be handled in this patch?
Should tackling that problem be left for later?

>> have you considered changing the logic in building and sorting the
>> database list in get_database_list and rebuild_database_list?  I'm
>> just curious what your thoughts might be on this subject.
> 
> I hadn't, but now that you mention it, it seems like a reasonable
> thing to try.
> 
>> As far as sorting the list of tables in an array and then copying
>> that array into a linked list, I think there is no need.  The
>> copying of table_ages into table_oids is followed immediately by
>>
>>    foreach(cell, table_oids)
>>
>> and then table_oids seems not to serve any further purpose.  Perhaps
>> you can just iterate over table_ages directly and avoid the extra
>> copying.
> 
> I hadn't looked toward any optimizations in this section, given that
> the vacuums in question can take hours or days, but I can see how that
> would make the code cleaner, so please find that change attached.

That looks better, thanks!

>> I have not tested this change, but I may do so later today or perhaps
>> on Monday.

The code compiles cleanly and passes all regression tests, but I don't
think those tests really cover what you are changing.  Have you been
using any test framework for this?

I wonder if you might add information about table size, table changes,
and bloat to your RelFrozenXidAge struct and modify rfxa_comparator to
use a heuristic to cost the (age, size, bloat, changed) grouping and
sort on that cost, such that really large bloated tables with old xids
might get vacuumed before smaller, less bloated tables that have
even older xids. Sorting the tables based purely on xid_age seems to
ignore other factors that are worth considering.  I do not have a
formula for how those four factors should be weighted in the heuristic,
but you are implicitly assigning three of them a weight of zero in
your current patch.

relation_needs_vacanalyze currently checks the reltuples, n_dead_tuples
and changes_since_analyze along with vac_scale_factor and
anl_scale_factor for the relation, but only returns booleans dovacuum,
doanalyze, and wraparound.  If you pass your RelFrozenXidAge struct
(perhaps renamed) into relation_needs_vacanalyze, it could store those
values for the relation so that you don't need to look it up again when
sorting.

-- 
Mark Dilger