Message-ID: <0be607a5-20f6-4894-bb22-412e3c784bc2@postgrespro.ru>
Date: Wed, 19 Mar 2025 19:05:13 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Optimize truncation logic to reduce AccessExclusive lock impact
To: David Rowley <dgrowleyml@gmail.com>, Stepan Neretin <slpmcf@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CA+Yyo5Trs9x03-sc4PpmEXPY9K_B_jbEbNd01RbF+vOxs3zVUA@mail.gmail.com>
 <CAApHDvrfngqCpQ09LKdr-BnJEVHW0=wAhxfRBnw=HJ2645cJPg@mail.gmail.com>
Content-Language: en-US
From: Yura Sokolov <y.sokolov@postgrespro.ru>
In-Reply-To: 
 <CAApHDvrfngqCpQ09LKdr-BnJEVHW0=wAhxfRBnw=HJ2645cJPg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/0be607a5-20f6-4894-bb22-412e3c784bc2%40postgrespro.ru>
Precedence: bulk

19.03.2025 05:09, David Rowley wrote:
> On Tue, 18 Mar 2025 at 19:04, Stepan Neretin <slpmcf@gmail.com> wrote:
>> We propose modifying the truncation condition in should_attempt_truncation to avoid unnecessary full buffer scans. The new formula ensures we only attempt truncation when we can free at least 3% of the relation size + 2 pages. This change prevents excessive truncation attempts on small tables while reducing the impact on larger relations. Previously, small tables could theoretically be truncated to a single page, but with this change, they may remain around 3 pages instead. We don't see this as a significant issue.
> 
> I'm just following this code through. Looking at
> DropRelationBuffers(), d6ad34f34 added an optimisation so that we can
> skip scanning all of shared buffers while in recovery (where
> smgrnblocks_cached() can return a non-InvalidBlockNumber value), which
> the following code might decide to lookup the buffers one-by-one
> rather than scanning the entire buffer pool:
> 
> if (BlockNumberIsValid(nBlocksToInvalidate) &&
>     nBlocksToInvalidate < BUF_DROP_FULL_SCAN_THRESHOLD)
>     {
>         for (j = 0; j < nforks; j++)
>             FindAndDropRelationBuffers(rlocator.locator, forkNum[j],
>                 nForkBlock[j], firstDelBlock[j]);
>         return;
> }
> 
> While I don't know this code too well, from a quick look, it seem to
> me that since DropRelationBuffers() is being called from
> smgrtruncate() in this case, smgrtruncate() has access to old_nblocks
> so it knows the old size of the relation. Couldn't we do something
> like have another version of DropRelationBuffers() which accepts a
> parameter for the old number of blocks and uses that instead of
> calling smgrnblocks_cached()?  That would allow the
> FindAndDropRelationBuffers() optimisation to be used in cases where
> the buffers to truncate is small or shared buffers is large.

Yes, it will work for smaller relations.
But it doesn't fix issue with large relations:
- imho, there is no big need to truncate 2TB table to release every 8MB of
free space, but it happens now.

So, probably we should do both:
- improve truncate of smaller relations
- reduce frequency of truncation larger relations.

I believe, suggested patch is acceptable in its form and will not harm
after optimization for small relations will be implemented.
(Disclaimer: I'm the author of the patch.)

> As for the patch being proposed, it looks like you're maybe just
> showing the answer to the question left in the comment below is "Yes".
>
> /*
>  * Space/time tradeoff parameters: do these need to be user-tunable?

Well, the patch changes formula as well. It is harder to implement
user-tunable formula.

-------
regards
Yura Sokolov