Re: [PATCH] Microvacuum for gist. - Mailing list pgsql-hackers

Hi,

I don't know too much about gist, but did a quick read through. Mostly
spotting some stylistic issues. Please fix those making it easier for
the next reviewer.

> *** a/src/backend/access/gist/gist.c
> --- b/src/backend/access/gist/gist.c
> ***************
> *** 36,42 **** static bool gistinserttuples(GISTInsertState *state, GISTInsertStack *stack,
>                    bool unlockbuf, bool unlockleftchild);
>   static void gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
>                   GISTSTATE *giststate, List *splitinfo, bool releasebuf);
> ! 

>   #define ROTATEDIST(d) do { \
>       SplitedPageLayout *tmp=(SplitedPageLayout*)palloc(sizeof(SplitedPageLayout)); \
> --- 36,42 ----
>                    bool unlockbuf, bool unlockleftchild);
>   static void gistfinishsplit(GISTInsertState *state, GISTInsertStack *stack,
>                   GISTSTATE *giststate, List *splitinfo, bool releasebuf);
> ! static void gistvacuumpage(Relation rel, Page page, Buffer buffer);
>   
>   #define ROTATEDIST(d) do { \
>       SplitedPageLayout
>       *tmp=(SplitedPageLayout*)palloc(sizeof(SplitedPageLayout)); \

Newline removed.

> +     /*
> +      * If leaf page is full, try at first to delete dead tuples. And then
> +      * check again.
> +      */
> +     if ((is_split) && GistPageIsLeaf(page) && GistPageHasGarbage(page))

superfluous parens around is_split
> + /*
> +  * gistkillitems() -- set LP_DEAD state for items an indexscan caller has
> +  * told us were killed.
> +  *
> +  * We match items by heap TID before mark them. If an item has moved off
> +  * the current page due to a split, we'll fail to find it and do nothing
> +  * (this is not an error case --- we assume the item will eventually get
> +  * marked in a future indexscan).
> +  *
> +  * We re-read page here, so it's significant to check page LSN. If page
> +  * has been modified since the last read (as determined by LSN), we dare not
> +  * flag any antries because it is possible that the old entry was vacuumed
> +  * away and the TID was re-used by a completely different heap tuple.

s/significant/important/?.
s/If page/If the page/
s/dare not/cannot/

> +  */
> + static void
> + gistkillitems(IndexScanDesc scan)
> + {
> +     GISTScanOpaque so = (GISTScanOpaque) scan->opaque;
> +     Buffer        buffer;
> +     Page        page;
> +     OffsetNumber minoff;
> +     OffsetNumber maxoff;
> +     int            i;
> +     bool        killedsomething = false;
> + 
> +     Assert(so->curBlkno != InvalidBlockNumber);
> + 
> +     buffer = ReadBuffer(scan->indexRelation, so->curBlkno);
> +     if (!BufferIsValid(buffer))
> +         return;
> + 
> +     LockBuffer(buffer, GIST_SHARE);
> +     gistcheckpage(scan->indexRelation, buffer);
> +     page = BufferGetPage(buffer);
> + 
> +     /*
> +      * If page LSN differs it means that the page was modified since the last read.
> +      * killedItemes could be not valid so LP_DEAD hints applying is not safe.
> +      */
> +     if(PageGetLSN(page) != so->curPageLSN)
> +     {
> +         UnlockReleaseBuffer(buffer);
> +         so->numKilled = 0; /* reset counter */
> +         return;
> +     }
> + 
> +     minoff = FirstOffsetNumber;
> +     maxoff = PageGetMaxOffsetNumber(page);
> + 
> +     maxoff = PageGetMaxOffsetNumber(page);

duplicated line.

> +     for (i = 0; i < so->numKilled; i++)
> +     {
> +         if (so->killedItems != NULL)
> +         {
> +             OffsetNumber offnum = FirstOffsetNumber;
> + 
> +             while (offnum <= maxoff)
> +             {

This nested loop deserves a comment.

> +                 ItemId        iid = PageGetItemId(page, offnum);
> +                 IndexTuple    ituple = (IndexTuple) PageGetItem(page, iid);
> + 
> +                 if (ItemPointerEquals(&ituple->t_tid, &(so->killedItems[i])))
> +                 {
> +                     /* found the item */
> +                     ItemIdMarkDead(iid);
> +                     killedsomething = true;
> +                     break;        /* out of inner search loop */
> +                 }
> +                 offnum = OffsetNumberNext(offnum);
> +             }
> +         }
> +     }

I know it's the same approach nbtree takes, but if there's a significant
number of deleted items this takes me as a rather suboptimal
approach. The constants are small, but this still essentially is O(n^2).

> ***************
> *** 451,456 **** getNextNearest(IndexScanDesc scan)
> --- 553,575 ----
>   
>       if (scan->xs_itup)
>       {
> +         /*
> +          * If previously returned index tuple is not visible save it into
> +          * so->killedItems. And at the end of the page scan mark all saved
> +          * tuples as dead.
> +          */
> +         if (scan->kill_prior_tuple)
> +         {
> +             if (so->killedItems == NULL)
> +             {
> +                 MemoryContext oldCxt2 = MemoryContextSwitchTo(so->giststate->scanCxt);
> + 
> +                 so->killedItems = (ItemPointerData *) palloc(MaxIndexTuplesPerPage * sizeof(ItemPointerData));
> +                 MemoryContextSwitchTo(oldCxt2);
> +             }

oldCxt2?

> +             if ((so->numKilled < MaxIndexTuplesPerPage))
> +                 so->killedItems[so->numKilled++] = scan->xs_ctup.t_self;
> +         }

superfluous parens.

> +             if ((so->curBlkno != InvalidBlockNumber) && (so->numKilled > 0))
> +                 gistkillitems(scan);

superfluous parens.

> +                 if ((scan->kill_prior_tuple) && (so->curPageData > 0))
> +                 {

superfluous parens.

>   
> +                     if (so->killedItems == NULL)
> +                     {
> +                         MemoryContext oldCxt = MemoryContextSwitchTo(so->giststate->scanCxt);
> + 
> +                         so->killedItems = (ItemPointerData *) palloc(MaxIndexTuplesPerPage *
sizeof(ItemPointerData));
> +                         MemoryContextSwitchTo(oldCxt);
> +                     }
> +                     if (so->numKilled < MaxIndexTuplesPerPage)
> +                         so->killedItems[so->numKilled++] = so->pageData[so->curPageData - 1].heapPtr;
> +                 }
>                   /* continuing to return tuples from a leaf page */
>                   scan->xs_ctup.t_self = so->pageData[so->curPageData].heapPtr;
>                   scan->xs_recheck = so->pageData[so->curPageData].recheck;
> ***************

overlong lines.

> *** 586,594 **** gistgettuple(PG_FUNCTION_ARGS)
> --- 723,751 ----
>                   PG_RETURN_BOOL(true);
>               }
>   
> +             /*
> +              * Check the last returned tuple and add it to killitems if
> +              * necessary
> +              */
> +             if ((scan->kill_prior_tuple) && (so->curPageData > 0) && (so->curPageData == so->nPageData))
> +             {

superfluous parens galore.

> +                 if (so->killedItems == NULL)
> +                 {
> +                     MemoryContext oldCxt = MemoryContextSwitchTo(so->giststate->scanCxt);
> + 
> +                     so->killedItems = (ItemPointerData *) palloc(MaxIndexTuplesPerPage * sizeof(ItemPointerData));
> +                     MemoryContextSwitchTo(oldCxt);
> +                 }
> +                 if ((so->numKilled < MaxIndexTuplesPerPage))
> +                     so->killedItems[so->numKilled++] = so->pageData[so->curPageData - 1].heapPtr;
> +             }
>               /* find and process the next index page */
>               do
>               {
> +                 if ((so->curBlkno != InvalidBlockNumber) && (so->numKilled > 0))
> +                     gistkillitems(scan);
> + 
>                   GISTSearchItem *item = getNextGISTSearchItem(so);
>   
>                   if (!item)

Too long lines.


Greetings,

Andres Freund