On Fri, Jul 26, 2019 at 1:34 PM Heikki Linnakangas <hlinn...@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.
