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.

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

I think I figured it out. Here is v4.

If the number of requests is less than 1 GB, the algorithm stays the same as before. If we need to process more, we will do it incrementally with slices of 1 GB.

Best regards,

Maxim Orlov.

Hi, 
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. 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. 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?

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

Hi,  

I’ve updated and rebased Maxim's patch version 4 with optimizations like ring buffer and capped max_requests proposed by Heikki.  These are the summary of Changes in v5:

• Replaced the linear array model in AbsorbSyncRequests() with a bounded ring buffer to avoid large memmove() operations when processing sync requests.

• Introduced a tunable batch size (CKPT_REQ_BATCH_SIZE, default: 10,000) to incrementally absorb requests while respecting MaxAllocSize.

• Capped max_requests with a hard upper bound (MAX_CHECKPOINT_REQUESTS = 10,000,000) to avoid pathological memory growth when shared_buffers is very large.

• Updated CompactCheckpointerRequestQueue() to support the ring buffer layout.

• Retained the existing compacting logic but modified it to operate in-place over the ring.

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

I’m not certain what the optimal cap for max_requests should be—whether it’s 10 million(setting it to 10 million would result in a peak temporary allocation of roughly 700 MB within CompactCheckpointerRequestQueue), 1 million, or some other value—but introducing an upper bound seems important. Without a cap, max_requests could theoretically scale with NBuffers, potentially resulting in excessive temporary memory allocations.

As this is my first patch submission, it might be somewhat naive and experimental— I appreciate your patience and feedback.