On Tue, Nov 19, 2019 at 6:40 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Nov 15, 2019 at 12:07:15PM +0900, Michael Paquier wrote:
> > So, Heikki, are you planning to work more on that and commit a change
> > close to what has been proposed upthread in [1]? It sounds to me that
> > this has the advantage to be non-intrusive and a similar solution has
> > been used for GIN indexes. Moving the redesign out of the discussion,
> > is there actually a downsize with back-patching something like
> > Heikki's version?
>
> So... I have been looking at this patch, and indeed it would be nice
> to pass down a better value than BRIN_DEFAULT_PAGES_PER_RANGE to be
> able to compute the stats in brincostestimate(). Still, it looks also
> to me that this allows the code to be able to compute some stats
> directly. As there is no consensus on a backpatch yet, my take would
> be for now to apply just the attached on HEAD, and consider a
> back-patch later on if there are more arguments in favor of it. If
> you actually test hypopg currently, the code fails when attempting to
> open the relation to get the stats now.
>
> Attached are the patch for HEAD, as well as a patch to apply to hypopg
> on branch REL1_STABLE to make the module compatible with PG13~.
>
> Any objections?
None from me. I'm obviously biased, but I hope that it can get
backpatched. BRIN is probably seldom used, but we shouldn't make it
harder to use it, even if it's that's only for hypothetical usage, and
even if it'll still be quite inexact.
> NB @Julien: perhaps you'd want to apply the second patch to the
> upstream repo of hypopg, and add more tests for other index AMs like
> GIN and BRIN.
Thanks! I didn't noticed that the compatibility macro for heap_open
was removed in f25968c49, I'll commit this patch on hypopg with some
compatibility macros to make sure that it compiles against all
versions. GIN (and some others) are unfortunately explicitly
disallowed with hypopg. Actually, most of the code already handles it
but I have no clear idea on how to estimate the number of tuples and
the size of such indexes. But yes, I should definitely add more tests
for supported AM, although I can't add any for BRIN until a fix is
committed :(