Re: Odd code around ginScanToDelete

Hi, Pavel!

Thank you for your review!

On Fri, Mar 6, 2026 at 4:45 PM Pavel Borisov <pashkin.elfe@gmail.com> 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