Re: [PATCHES] GIN improvements - Mailing list pgsql-hackers

From Teodor Sigaev
Subject Re: [PATCHES] GIN improvements
Date
Msg-id 49708008.8070109@sigaev.ru
Whole thread Raw
In response to Re: [PATCHES] GIN improvements  (Jeff Davis <pgsql@j-davis.com>)
Responses Re: [PATCHES] GIN improvements  (Jeff Davis <pgsql@j-davis.com>)
Re: [PATCHES] GIN improvements  (Alvaro Herrera <alvherre@commandprompt.com>)
List pgsql-hackers
New version. Changes:
  - synced with current CVS
  - added all your changes
  - autovacuum will run if fast update mode is turned on and
    trigger of fresh tuple is fired
  - gincostestimate now tries to calculate cost of scan of pending pages.
    gincostestimate set disable_cost if it believe that tidbitmap will
    become lossy. So, tidbitmap has new method - estimation of
    maximum number of tuples with guaranteed non-lossy mode.


> START_CRIT_SECTION();
> ...
> l = PageAddItem(...);
> if (l == InvalidOffsetNumber)
>     elog(ERROR, "failed to add item to index page in \"%s\"",
>          RelationGetRelationName(index));
>
> It's no use using ERROR, because it will turn into PANIC, which is
I did that similar to other GIN/GiST places. BTW, BTree  directly emits PANIC if
PageAddItem fails


>
> 4. Heikki mentioned:
> http://archives.postgresql.org/pgsql-hackers/2008-11/msg01832.php
>
> "To make things worse, a query will fail if all the matching
> fast-inserted tuples don't fit in the non-lossy tid bitmap."
>
> That issue still remains, correct? Is there a resolution to that?

Now gincostestimate can forbid index scan by disable_cost (see Changes). Of
course, it doesn't prevent failure in case of large update (for example), but it
prevents in most cases. BTW, because of sequential scan of pending list cost of
scan grows up fast and index scan becomes non-optimal.

>
> 5. I attached a newer version merged with HEAD.
Thank you

> 6. You defined:
>
> #define GinPageHasFullRow(page) ( GinPageGetOpaque(page)->flags &
> GIN_LIST_FULLROW )
>
Fixed


> 7.  I don't understand this chunk of code:
>
> How can (!ItemPointerEquals(&pos->item, &item)) ever happen?
>
> And how can (scanGetCandidate(scan, pos) == false) ever happen? Should
> that be an Assert() instead?
>
> If those can happen during normal operation, then we need a better error
> message there.

It should be assert, but assert enabled and disabled code will be different :(.
In both cases, scanGetCandidate() should be called, but in assert enabled code
we need to check return value and pos->item.

--
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/


Attachment

pgsql-hackers by date:

Previous
From: Magnus Hagander
Date:
Subject: Re: FWD: Re: Updated backslash consistency patch
Next
From: Magnus Hagander
Date:
Subject: Re: libpq WSACleanup is not needed