Hi, Pavel! Thank you for your review!
On Fri, Mar 6, 2026 at 4:45 PM Pavel Borisov <[email protected]> wrote: > Some thoughts: > > Is it worth/possible in recursive calls of ginScanToDelete() to free > allocated myStackItem->child after processing all children of the > current level, when they are not needed anymore? > Previously to this patch, palloc-ed "me" variable also was't freed at > recursion levels. > > Could limiting the maximum recursion level be useful? > > In the comment to myStackItem before ginScanToDelete(), it might be > worth adding that after processing all pages on the current level, > myStackItem is not needed anymore. Already answered in this thread. > > > Yes, it's not possible to have meDelete set for root, because > > > me->leftBuffer is always InvalidBuffer for the root. So the branch > > > handling meDelete case should better do Assert(!isRoot). > Looks like this additional Assert is not in patch v1. > > In the root call of ginScanToDelete(gvs, &root); we can add Assert > checking that its return result is false: > - ginScanToDelete(gvs, &root); > + deleted = ginScanToDelete(gvs, &root); > +. Assert(!deleted); /* Root page is never deleted */ Done. > Additionally, it could be good to rename all vacuum functions related > to posting tree pages only, to include "Posting" (e.g., ginDeletePage > -> ginDeletePostingPage). The same is for the functions only for the > entry tree. It is already named this way in many places (e.g. > ginVacuumPostingTreeLeaves). It could be good to extend this to all > relevant functions. Renamed as you proposed. > Several small proposals on wording: > "rightmost non-deleted page to its left" -> "closest non-deleted > sibling page to its left" I renamed that to just "left sibling". Deleted pages are not in the tree already. And "the rightmost page to its left" is just left sibling. > "each entry tracks the buffer of the page" -> "each entry tracks the > buffers of the page" (as two buffers are mentioned) I prefer to just use word "buffer" twice to make it more explicit. > "must already be pinned" -> "must already have been pinned" Done. ------ Regards, Alexander Korotkov Supabase
v2-0001-Rework-ginScanToDelete-to-pass-Buffers-instead-of.patch
Description: Binary data
