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
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.
Case (3) is not an option, in my opinion. So, we following (1) or (2). Personally, I'm for (2), PFA v0 patch.--
Best regards,
Maxim Orlov.
Attachment
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 Wed, 26 Feb 2025 at 11:54, Andres Freund <andres@anarazel.de> wrote:
4) Do compaction incrementally, instead of doing it for all requests at once.
Yeah, good idea! I completely forgot about that. Thanks!
Best regards,
Maxim Orlov.
I tried to implement the idea (4). This is the patch.
But, there is a problem. See, when we release lock and call RememberSyncRequest() and acquire it again, CompactCheckpointerRequestQueue() may be called and the state of the request array may be changed. Of course, we can recheck num_requests after retaking the lock and restart the algo all over again. But this is not a great idea, since we can stuck in this loop if someone is pushing requests in the queue.
As for case (3). In fact, the described problem happens only with high enough values of NBuffers. Thus, user already expected postgres to use huge amount of RAM. Is this really a problem if he will demand some more to process sync request?
Best regards,
Maxim Orlov.
Attachment
After done some testing, I found a bug in the patch. If more requests were pushed while we release the lock, num_requests could not be set to zero.
Here is a fixed version.
--
Best regards,
Maxim Orlov.
Attachment
Here is an alternative solution. We can limit the number of processed requests to fit in a 1Gb memory chunk for each pass. Obviously, we left some requests in the queue to be processed in the next call. We can overcome this by using multi-step processing: estimating the number of steps in the beginning and processing requests again.
--
I'd like to hear your opinion on the subject.
Best regards,
Maxim Orlov.
Attachment
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.
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.
Attachment
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. 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?
It would be helpful to know the reasoning behind these design decisions.
Maxim Orlov <orlovmg@gmail.com> 于2025年3月1日周六 00:54写道：
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.
On Wed, 12 Mar 2025 at 10:27, Xuneng Zhou <xunengzhou@gmail.com> wrote:
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?
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.
Best regards,
Maxim Orlov.
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)
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.
Attachment
Hi,
I've moved this entry to the July CommitFest. The CI reported an unused variable warning in patch v5, so I've addressed it by removing the unused one. Sorry for not catching the issue locally.
Xuneng Zhou <xunengzhou@gmail.com> 于2025年4月10日周四 23:43写道：
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.
Attachment
Hi, Xuneng Zhou! On Tue, Apr 15, 2025 at 7:02 AM Xuneng Zhou <xunengzhou@gmail.com> wrote: > I've moved this entry to the July CommitFest. The CI reported an unused variable warning in patch v5, so I've addressedit by removing the unused one. Sorry for not catching the issue locally. Thank you for your work on this subject! I have few notes about that: 1) Should we make CompactCheckpointerRequestQueue() process the queue of checkpoint requests in smaller parts for the same reason we do this in AbsorbSyncRequests()? That would require significant redesign of the algorithm, but still. 2) That's pretty independent to the changes by the patch, but should CompactCheckpointerRequestQueue() fill the gaps with entries from the tail instead of rewriting the whole queue? That might be a bit faster. 3) For sure, we wouldn't backpatch this. Can we prepare some simple solution for back branches? Perhaps, just introduction of MAX_CHECKPOINT_REQUESTS is enough to save us from allocations larger than 1GB. ------ Regards, Alexander Korotkov Supabase
On Tue, Jun 3, 2025 at 1:16 PM Xuneng Zhou <xunengzhou@gmail.com> wrote: > Thanks a lot for reviewing! > > > I have few notes about that: > > 1) Should we make CompactCheckpointerRequestQueue() process the queue > > of checkpoint requests in smaller parts for the same reason we do this > > in AbsorbSyncRequests()? That would require significant redesign of > > the algorithm, but still. > > In AbsorbSyncRequests, we process requests incrementally in batches to > avoid allocating more than 1 GB of memory, which would lead to > repeated failure. I think this is less concerning in > CompactCheckpointerRequestQueue, because if we caps num_requests at 10 > million, the hash table peaks at ~500 MB and skip_slot[] at ~10 > MB—both under 1 GB. Right, but another point is to avoid lengthy holding of CheckpointerCommLock. What do you think about that? > > 2) That's pretty independent to the changes by the patch, but should > > CompactCheckpointerRequestQueue() fill the gaps with entries from the > > tail instead of rewriting the whole queue? That might be a bit > > faster. > This optimization would be quite helpful for compacting large queues. > For small ones, it may also add extra costs. Can we use a hybrid > approach? If it's independent, should we create a standalone patch for > it? Why do you think this would add extra cost of class queues? Given we're basically rewriting the algorithm of CompactCheckpointerRequestQueue(), I think it's OK to integrate this into once patch. > > 3) For sure, we wouldn't backpatch this. Can we prepare some simple > > solution for back branches? Perhaps, just introduction of > > MAX_CHECKPOINT_REQUESTS is enough to save us from allocations larger > > than 1GB. > > > I think this would work well for back branches. OK. ------ Regards, Alexander Korotkov Supabase
Hi all, Sorry—I forgot to Cc on my previous message. Resending here so they’re on the thread: On Wed, Jun 4, 2025 at 11:07 AM Xuneng Zhou <xunengzhou@gmail.com> wrote: > > Hi Alexander, > > Thanks again for the feedback! > > 1) Batch-processing CompactCheckpointerRequestQueue() and AbsorbSyncRequests()? > > After some thoughts, I realized my previous take was incomplete—sorry > for the confusion. Heikki suggested capping num_requests at 10 million > [1]. With that limit, the largest hash table is ~500 MB and the > skip_slot[] array is ~10 MB in CompactCheckpointerRequestQueue and the > max size of request array in AbsorbSyncRequests is well under 400 MB, > so we never exceed 1 GB. Even without batching, compaction stays under > the cap. Batching in AbsorbSyncRequests may still help by amortizing > memory allocation, but it adds extra lock/unlock overhead. Not sure if > that overhead is worth it under the cap. > > Of course, all of this depends on having a cap in place. Picking the > right cap size can be tricky (see point 2). If we decide not to > enforce a cap now or in future versions, then batching both > CompactCheckpointerRequestQueue(maybe?) and AbsorbSyncRequests become > essential. We also need to consider the batch size—Heikki suggested 10 > k for AbsorbSyncRequests—but I’m not sure whether that suits typical > or extreme workloads. > > > Right, but another point is to avoid lengthy holding of > > CheckpointerCommLock. What do you think about that? > > I am not clear on this. Could you elaborate on it? > > [1] https://www.postgresql.org/message-id/c1993b75-a5bc-42fd-bbf1-6f06a1b37107%40iki.fi > > > 2) Back-branch fixes with MAX_CHECKPOINT_REQUESTS? > > This is simple and effective, but can be hard to get the value right. > I think we should think more of it. For very large-scale use cases, > like hundreds of GB shared_buffers, 10 million seems small if the > checkpointer is not able to absorb the changes before the queue fills > up. In this case, making compaction more efficient like 3) would be > helpful. However, if we do this for back-branch as well, the solution > is not that simple any more. > > > 3) Fill gaps by pulling from the tail instead of rewriting the whole queue? > > I misunderstood at first—this is a generally helpful optimization. > I'll integrate it into the current patch.
Hi, Thanks for the feedback! > I think it would be good start point to use the same batch size of > CompactCheckpointerRequestQueue() and AbsorbSyncRequests() > So we’ll keep both batch processing and the request cap in place for now. > > > Right, but another point is to avoid lengthy holding of > > > CheckpointerCommLock. What do you think about that? > > > > I am not clear on this. Could you elaborate on it? > > See [1] for more detailed description of this. > Links. > 1. https://www.postgresql.org/message-id/flat/db4534f83a22a29ab5ee2566ad86ca92%40postgrespro.ru I read the thread but didn’t find a specific explanation of avoiding long lock holds. My understanding is: when compaction processes a very large queue in one go, it holds CheckpointerCommLock the entire time, blocking all ForwardSyncRequest callers. Batch processing would release the lock after each chunk, allowing other backends to make progress. Is that correct?