Proposal: Limitations of palloc inside checkpointer

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.

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.

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.

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.

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?

It would be helpful to know the reasoning behind these design decisions.

Maxim Orlov <orlovmg@gmail.com> 于2025年3月1日周六 00:54写道：

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.

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, 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 addressed it 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

Hi Alexander,

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.

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?

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.

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:

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. /messages/by-id/db4534f83a22a29ab5ee2566ad86ca92@postgrespro.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?

Hi,

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.

Great, thank you.

I dug deeper into the “fill gaps from the tail” optimization and
implemented a version of it. The tricky part is not the copy itself but
guaranteeing that the queue ends up hole-free and that tail really points
at the slot after the last live request. With a twin-cursor gap-fill we
refuse to move SYNC_FORGET_REQUEST / SYNC_FILTER_REQUEST (they’re
order-sensitive fences).

If the final survivor is one of those barriers, the cursors meet while a
hole still exists immediately before the barrier:

head → A [hole] FILTER(X) …unused…

If we then compute tail = (head + remaining_requests) % max_requests, the
value lands inside the live region (on the barrier itself). The
invariant (head
+ num_requests) % max_requests == tail is broken, so the next enqueue
overwrites live data or the checkpointer under-scans the queue.

Alternatively, we may allow relocating SYNC_FORGET_REQUEST and
SYNC_FILTER_REQUEST entries, but ensuring their ordering semantics remain
correct would be quite challenging. That concern is why the implementation
uses a forward-scan compaction. As the source comment noted:

/*
* The basic idea here is that a request can be skipped if it's followed
* by a later, identical request. It might seem more sensible to work
* backwards from the end of the queue and check whether a request is
* *preceded* by an earlier, identical request, in the hopes of doing less
* copying. But that might change the semantics, if there's an
* intervening SYNC_FORGET_REQUEST or SYNC_FILTER_REQUEST, so we do it
* this way.

Best,
Xuneng

Hi,

Patch v7 modifies CompactCheckpointerRequestQueue() to process requests
incrementally in batches of CKPT_REQ_BATCH_SIZE (10,000), similar to the
approach used in AbsorbSyncRequests(). This limits memory usage from
O(num_requests) to O(batch_size) for both hash tables and skip arrays.

- Hash table memory bounded by batch size regardless of total queue size

- Skip array allocation limited to batch size instead of max_requests

- Prevents potential OOM conditions with very large request queues

Trade-offs

Cross-batch duplicate detection: The incremental approach won't detect
duplicates spanning batch boundaries. This limitation seems acceptable
since:

- The main issue need to be addressed is preventing memory allocation
failures

- Remaining duplicates are still handled by the RememberSyncRequest()
function in the sync subsystem

- The purpose of this function is to make some rooms for new requests
not remove all duplicates.

Lock holding Duration

Andres pointed out[1]https://postgrespro.com/list/id/bjno37ickfafixkqmd2lcyopsajnuig5mm4rg6tn2ackpqyiba@w3sjfo3usuos that compacting a very large queue takes considerable
time, and holding the exclusive lock for an extended period makes it much
more likely that backends will have to perform syncs themselves - which is
exactly what CompactCheckpointerRequestQueue() is trying to avoid in the
first place. However, releasing the lock between batches would introduce
race conditions that would make the design much more complex. Given that
the primary goal of this patch is to avoid large memory allocations, I keep
the lock held for the whole function for simplicity now.

[1]: https://postgrespro.com/list/id/bjno37ickfafixkqmd2lcyopsajnuig5mm4rg6tn2ackpqyiba@w3sjfo3usuos
https://postgrespro.com/list/id/bjno37ickfafixkqmd2lcyopsajnuig5mm4rg6tn2ackpqyiba@w3sjfo3usuos

Best regards,

Xuneng