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
