Thread: Proposal: Limitations of palloc inside checkpointer

Hi, hackers!

Historically, the checkpointer process use palloc() into 
AbsorbSyncRequests() function. Therefore, the checkpointer does not 
expect to receive a request larger than 1 GB.

We encountered a case where the database went into recovery state, after 
applying all wal, the checkpointer process generated an "invalid memory 
alloc request size" error and entered a loop. But it is quite acceptable 
for the recovery state to receive such a large allocation request.

A simple solution to this problem is to use palloc_extended() instead of 
palloc(). But is it safe to allow the checkpointer to allocate so much 
memory at once?

I have proposal to update this memory allocation but I need your ideas 
and advices on how to do it in appropriate way. As an idea, we can 
replace the array with a list of arrays to allocate memory in chunks. As 
a bad idea, we can process a temporary array without locking.

I would be glad to hear your ideas and suggestions about this topic. 
Have a nice day!

-- 
Ekaterina Sokolova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2025-02-26 11:46:45 +0300, Maxim Orlov wrote:
> On Tue, 25 Feb 2025 at 22:44, Ekaterina Sokolova <e.sokolova@postgrespro.ru>
> wrote:
> 
> > Hi, hackers!
> >
> > Historically, the checkpointer process use palloc() into
> > AbsorbSyncRequests() function. Therefore, the checkpointer does not
> > expect to receive a request larger than 1 GB.
> 
> 
> Yeah. And the most unpleasant thing is it won't simply fail with an error
> or helpful message suggesting a workaround (reduce the amount of shared
> memory). Checkpointer will just "stuck".
> 
> AFAICS, we have a few options:
> 1. Leave it as it is, but fatal on allocation of the chunk more than 1G.
> 2. Use palloc_extended with MCXT_ALLOC_HUGE flag.
> 3. Do not use any allocation and use CheckpointerShmem->requests directly
> in case of > 1G size of the required allocation.

4) Do compaction incrementally, instead of doing it for all requests at once.

That'd probably be better, because

a) it'll take some time to to compact 10s to 100s of million requests, which
   makes it much more likely that backends will have to perform syncs
   themselves and the lock will be held for an extended period of time

b) allocating gigabytes of memory obviously makes it more likely that you'll
   fail with out-of-memory at runtime or evne get OOM killed.

Greetings,

Andres Freund

On 12/03/2025 13:00, Maxim Orlov wrote:
> On Wed, 12 Mar 2025 at 10:27, Xuneng Zhou <xunengzhou@gmail.com 
> <mailto:xunengzhou@gmail.com>> wrote:
> 
>> The patch itself looks ok to me. I'm curious about the trade-offs
>> between this incremental approach and the alternative of
>> using palloc_extended() with the MCXT_ALLOC_HUGE flag. The approach
>> of splitting the requests into fixed-size slices  avoids OOM
>> failures or process termination by the OOM killer, which is good.

+1

>> However, it does add some overhead with additional lock acquisition/
>> release cycles and memory movement operations via memmove(). The
>> natural question is whether the security justify the cost.

Hmm, if you turned the array into a ring buffer, you could absorb part 
of the queue without the memmove().

With that, I'd actually suggest using a much smaller allocation, maybe 
10000 entries or even less. That should be enough to keep the lock 
acquire/release overhead acceptable.

>> Regarding the slice size of 1 GB,  is this derived from
>> MaxAllocSize limit, or was it chosen for other performance
>> reasons? whether a different size might offer better performance
>> under typical workloads?
> 
> I think 1 GB is derived purely from MaxAllocSize. This "palloc" is a 
> relatively old one, and no one expected the number of requests to exceed 
> 1 GB. Now we have the ability to set the shared_buffers to a huge number 
> (without discussing now whether this makes any real sense), thus this 
> limit for palloc becomes a problem.

Yeah. Frankly even 1 GB seems excessive for this. We set max_requests = 
NBuffers, which makes some sense so that you can fit the worst case 
scenario of quickly evicting all pages from the buffer cache. But even 
then I'd expect the checkpointer to be able to absorb the changes before 
the queue fills up. And we have the compaction logic now too.

So I wonder if we should cap max_requests at, say, 10 million requests now?

CompactCheckpointerRequestQueue() makes a pretty large allocation too, 
BTW, although much smaller than the one in AbsorbSyncRequests(). The 
hash table it builds could grow quite large though.

-- 
Heikki Linnakangas
Neon (https://neon.tech)