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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
