Re: Hypothetical indexes using BRIN broken since pg10 - Mailing list pgsql-hackers

On Fri, Jul 26, 2019 at 1:34 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> There seems to be consensus on the going with the approach from the
> original patch, so I had a closer look at it.
>
> The patch first does this:
>
> >
> >       /*
> >        * Obtain some data from the index itself, if possible.  Otherwise invent
> >        * some plausible internal statistics based on the relation page count.
> >        */
> >       if (!index->hypothetical)
> >       {
> >               indexRel = index_open(index->indexoid, AccessShareLock);
> >               brinGetStats(indexRel, &statsData);
> >               index_close(indexRel, AccessShareLock);
> >       }
> >       else
> >       {
> >               /*
> >                * Assume default number of pages per range, and estimate the number
> >                * of ranges based on that.
> >                */
> >               indexRanges = Max(ceil((double) baserel->pages /
> >                                                          BRIN_DEFAULT_PAGES_PER_RANGE), 1.0);
> >
> >               statsData.pagesPerRange = BRIN_DEFAULT_PAGES_PER_RANGE;
> >               statsData.revmapNumPages = (indexRanges / REVMAP_PAGE_MAXITEMS) + 1;
> >       }
> >       ...
>
> And later in the function, there's this:
>
> >       /* work out the actual number of ranges in the index */
> >       indexRanges = Max(ceil((double) baserel->pages / statsData.pagesPerRange),
> >                                         1.0);
>
> It seems a bit error-prone that essentially the same formula is used
> twice in the function, to compute 'indexRanges', with some distance
> between them. Perhaps some refactoring would help with, although I'm not
> sure what exactly would be better. Maybe move the second computation
> earlier in the function, like in the attached patch?

I had the same thought without a great idea on how to refactor it.
I'm fine with the one in this patch.

> The patch assumes the default pages_per_range setting, but looking at
> the code at https://github.com/HypoPG/hypopg, the extension actually
> takes pages_per_range into account when it estimates the index size. I
> guess there's no easy way to pass the pages_per_range setting down to
> brincostestimate(). I'm not sure what we should do about that, but seems
> that just using BRIN_DEFAULT_PAGES_PER_RANGE here is not very accurate.

Yes, hypopg can use a custom pages_per_range as it intercepts it when
the hypothetical index is created.  I didn't find any way to get that
information in brincostestimate(), especially for something that could
backpatched.  I don't like it, but I don't see how to do better than
just using BRIN_DEFAULT_PAGES_PER_RANGE :(

> The attached patch is based on PG v11, because I tested this with
> https://github.com/HypoPG/hypopg, and it didn't compile with later
> versions. There's a small difference in the locking level used between
> v11 and 12, which makes the patch not apply across versions, but that's
> easy to fix by hand.

FTR I created a REL_1_STABLE branch for hypopg which is compatible
with pg12 (it's already used for debian packages), as master is still
in beta and v12 compatibility worked on.