Re: [PATCH] Microvacuum for gist.

From: Andres Freund <andres(at)anarazel(dot)de>
To: Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>
Cc: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Microvacuum for gist.
Date: 2015-09-02 16:44:40
Message-ID: 20150902164440.GE5286@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christopher Browne 2015-09-02 16:45:26 Re: Proposal: Implement failover on libpq connect level.
Previous Message Robert Haas 2015-09-02 16:41:55 Re: Hooking at standard_join_search (Was: Re: Foreign join pushdown vs EvalPlanQual)