Re: Hypothetical indexes using BRIN broken since pg10
On Tue, Nov 19, 2019 at 09:48:59PM +0900, Michael Paquier wrote: > Re-reading the thread. Any design change should IMO just happen on > master so as we don't take any risks with potential ABI breakages. > Even if there is not much field demand for it, that's not worth the > risk. Thinking harder, I don't actually quite see why it would be an > issue to provide default stats for an hypothetical BRIN index based > using the best estimations we can do down to 10 with the infra in > place. Taking the case of hypopg, one finishes with an annoying > "could not open relation with OID %u", which is not that nice from the > user perspective. Let's wait a bit and see if others have more > arguments to offer. Okay. Hearing nothing, committed down to 10. -- Michael signature.asc Description: PGP signature
Re: Hypothetical indexes using BRIN broken since pg10
On Tue, Nov 19, 2019 at 08:37:04AM +0100, Julien Rouhaud wrote: > 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. Re-reading the thread. Any design change should IMO just happen on master so as we don't take any risks with potential ABI breakages. Even if there is not much field demand for it, that's not worth the risk. Thinking harder, I don't actually quite see why it would be an issue to provide default stats for an hypothetical BRIN index based using the best estimations we can do down to 10 with the infra in place. Taking the case of hypopg, one finishes with an annoying "could not open relation with OID %u", which is not that nice from the user perspective. Let's wait a bit and see if others have more arguments to offer. -- Michael signature.asc Description: PGP signature
Re: Hypothetical indexes using BRIN broken since pg10
On Tue, Nov 19, 2019 at 6:40 AM Michael Paquier 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 :(
Re: Hypothetical indexes using BRIN broken since pg10
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? 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. -- Michael diff --git a/hypopg.c b/hypopg.c index 4ab76b7..fe8be62 100644 --- a/hypopg.c +++ b/hypopg.c @@ -140,22 +140,22 @@ hypo_getNewOid(Oid relid) char relpersistence; /* Open the relation on which we want a new OID */ - relation = heap_open(relid, AccessShareLock); + relation = table_open(relid, AccessShareLock); reltablespace = relation->rd_rel->reltablespace; relpersistence = relation->rd_rel->relpersistence; /* Close the relation and release the lock now */ - heap_close(relation, AccessShareLock); + table_close(relation, AccessShareLock); /* Open pg_class to aks a new OID */ - pg_class = heap_open(RelationRelationId, RowExclusiveLock); + pg_class = table_open(RelationRelationId, RowExclusiveLock); /* ask for a new relfilenode */ newoid = GetNewRelFileNode(reltablespace, pg_class, relpersistence); /* Close pg_class and release the lock now */ - heap_close(pg_class, RowExclusiveLock); + table_close(pg_class, RowExclusiveLock); return newoid; } @@ -297,7 +297,7 @@ hypo_get_relation_info_hook(PlannerInfo *root, Relation relation; /* Open the current relation */ - relation = heap_open(relationObjectId, AccessShareLock); + relation = table_open(relationObjectId, AccessShareLock); if (relation->rd_rel->relkind == RELKIND_RELATION #if PG_VERSION_NUM >= 90300 @@ -324,7 +324,7 @@ hypo_get_relation_info_hook(PlannerInfo *root, } /* Close the relation release the lock now */ - heap_close(relation, AccessShareLock); + table_close(relation, AccessShareLock); } if (prev_get_relation_info_hook) diff --git a/hypopg_index.c b/hypopg_index.c index db119e1..7a1d18a 100644 --- a/hypopg_index.c +++ b/hypopg_index.c @@ -1396,7 +1396,7 @@ hypopg_get_indexdef(PG_FUNCTION_ARGS) if (indexpr_item == NULL) elog(ERROR, "too few entries in indexprs list"); indexkey = (Node *) lfirst(indexpr_item); - indexpr_item = lnext(indexpr_item); + indexpr_item = lnext(entry->indexprs, indexpr_item); /* Deparse */ str = deparse_expression(indexkey, context, false, false); @@ -1546,7 +1546,7 @@ hypo_estimate_index_simple(hypoIndex * entry, BlockNumber *pages, double *tuples rel = makeNode(RelOptInfo); /* Open the hypo index' relation */ - relation = heap_open(entry->relid, AccessShareLock); + relation = table_open(entry->relid, AccessShareLock); if (!RelationNeedsWAL(relation) && RecoveryInProgress()) ereport(ERROR, @@ -1567,7 +1567,7 @@ hypo_estimate_index_simple(hypoIndex * entry, BlockNumber *pages, double *tuples >pages, >tuples, >allvisfrac); /* Close the relation and release the lock now */ - heap_close(relation, AccessShareLock); + table_close(relation, AccessShareLock); hypo_estimate_index(entry, rel); *pages = entry->pages; diff --git a/import/hypopg_import_index.c b/import/hypopg_import_index.c index d482f30..e6b6a2c 100644 --- a/import/hypopg_import_index.c +++ b/import/hypopg_import_index.c @@ -91,7 +91,7 @@ build_index_tlist(PlannerInfo *root, IndexOptInfo *index, if (indexpr_item == NULL) elog(ERROR, "wrong number of index expressions"); indexvar = (Expr *) lfirst(indexpr_item); - indexpr_item = lnext(indexpr_item); + indexpr_item = lnext(index->indexprs, indexpr_item); } tlist = lappend(tlist, diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 024f325eb0..abbc7e9e7e 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -102,6 +102,7 @@ #include #include "access/brin.h" +#include "access/brin_page.h" #include "access/gin.h" #include "access/table.h" #include "access/tableam.h" @@ -6865,12 +6866,35 @@
Re: Hypothetical indexes using BRIN broken since pg10
On Wed, Sep 25, 2019 at 07:03:52AM +0200, Julien Rouhaud wrote: > IIUC, if something like Heikki's patch is applied on older branch the > problem will be magically fixed from the extension point of view so > that should be safe (an extension would only need to detect the minor > version to get a more useful error message for users), and all > alternatives are too intrusive to be patckbatched. 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? Tom, Alvaro and Julien, do you have more thoughts to share? [1]: https://www.postgresql.org/message-id/b847493e-d263-3f2e-1802-689e778c9...@iki.fi -- Michael signature.asc Description: PGP signature
Re: Hypothetical indexes using BRIN broken since pg10
On Tue, Sep 24, 2019 at 11:53 PM Alvaro Herrera wrote: > > I think the danger is what happens if a version of your plugin that was > compiled with the older definition runs in a Postgres which has been > recompiled with the new code. This has happened to me with previous > unnoticed ABI breaks, and it has resulted in crashes in production > systems. It's not a nice situation to be in. Indeed. > If the break is benign, i.e. "nothing happens", then it's possibly a > worthwhile change to consider. I suppose the only way to know is to > write patches for both sides and try it out. IIUC, if something like Heikki's patch is applied on older branch the problem will be magically fixed from the extension point of view so that should be safe (an extension would only need to detect the minor version to get a more useful error message for users), and all alternatives are too intrusive to be patckbatched.
Re: Hypothetical indexes using BRIN broken since pg10
On 2019-Sep-24, Julien Rouhaud wrote: > On Mon, Sep 9, 2019 at 5:03 PM Tom Lane wrote: > > Whether we should bother back-patching a less capable stopgap fix > > is unclear to me. Yeah, it's a bug that an index adviser can't > > try a hypothetical BRIN index; but given that nobody noticed till > > now, it doesn't seem like there's much field demand for it. > > And I'm not sure that extension authors would want to deal with > > testing minor-release versions to see if the fix is in, so > > even if there were a back-patch, it might go unused. > > FWIW I maintain such an extension and testing for minor release > version is definitely not a problem. I think the danger is what happens if a version of your plugin that was compiled with the older definition runs in a Postgres which has been recompiled with the new code. This has happened to me with previous unnoticed ABI breaks, and it has resulted in crashes in production systems. It's not a nice situation to be in. If the break is benign, i.e. "nothing happens", then it's possibly a worthwhile change to consider. I suppose the only way to know is to write patches for both sides and try it out. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hypothetical indexes using BRIN broken since pg10
On Mon, Sep 9, 2019 at 5:03 PM Tom Lane wrote: > > Alvaro Herrera from 2ndQuadrant writes: > > On 2019-Sep-02, Tom Lane wrote: > >> The right answer IMO is basically for the brinGetStats call to go > >> away from brincostestimate and instead happen during plancat.c's > >> building of the IndexOptInfo. In the case of a hypothetical index, > >> it'd fall to the get_relation_info_hook to fill in suitable fake > >> data. > > > So I'm not clear on what the suggested strategy is, here. Do we want > > that design change to occur in the bugfix that would be backpatched, or > > do we want the backbranches to use the patch as posted and then we apply > > the above design on master only? > > The API change I'm proposing is surely not back-patchable. > > Whether we should bother back-patching a less capable stopgap fix > is unclear to me. Yeah, it's a bug that an index adviser can't > try a hypothetical BRIN index; but given that nobody noticed till > now, it doesn't seem like there's much field demand for it. > And I'm not sure that extension authors would want to deal with > testing minor-release versions to see if the fix is in, so > even if there were a back-patch, it might go unused. FWIW I maintain such an extension and testing for minor release version is definitely not a problem.
Re: Hypothetical indexes using BRIN broken since pg10
Alvaro Herrera from 2ndQuadrant writes: > On 2019-Sep-02, Tom Lane wrote: >> The right answer IMO is basically for the brinGetStats call to go >> away from brincostestimate and instead happen during plancat.c's >> building of the IndexOptInfo. In the case of a hypothetical index, >> it'd fall to the get_relation_info_hook to fill in suitable fake >> data. > So I'm not clear on what the suggested strategy is, here. Do we want > that design change to occur in the bugfix that would be backpatched, or > do we want the backbranches to use the patch as posted and then we apply > the above design on master only? The API change I'm proposing is surely not back-patchable. Whether we should bother back-patching a less capable stopgap fix is unclear to me. Yeah, it's a bug that an index adviser can't try a hypothetical BRIN index; but given that nobody noticed till now, it doesn't seem like there's much field demand for it. And I'm not sure that extension authors would want to deal with testing minor-release versions to see if the fix is in, so even if there were a back-patch, it might go unused. regards, tom lane
Re: Hypothetical indexes using BRIN broken since pg10
On 2019-Sep-02, Tom Lane wrote: > Julien Rouhaud writes: > > On Fri, Jul 26, 2019 at 1:34 PM Heikki Linnakangas wrote: > >> 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 :( > > I can tell you what I think ought to happen, but making it happen might > be more work than this patch should take on. > > The right answer IMO is basically for the brinGetStats call to go > away from brincostestimate and instead happen during plancat.c's > building of the IndexOptInfo. In the case of a hypothetical index, > it'd fall to the get_relation_info_hook to fill in suitable fake > data. So I'm not clear on what the suggested strategy is, here. Do we want that design change to occur in the bugfix that would be backpatched, or do we want the backbranches to use the patch as posted and then we apply the above design on master only? If the former -- is Julien interested in trying to develop that? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hypothetical indexes using BRIN broken since pg10
Julien Rouhaud writes: > On Fri, Jul 26, 2019 at 1:34 PM Heikki Linnakangas wrote: >> 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 :( I can tell you what I think ought to happen, but making it happen might be more work than this patch should take on. The right answer IMO is basically for the brinGetStats call to go away from brincostestimate and instead happen during plancat.c's building of the IndexOptInfo. In the case of a hypothetical index, it'd fall to the get_relation_info_hook to fill in suitable fake data. Sounds simple, but: 1. We really don't want even more AM-specific knowledge in plancat.c. So I think the right way to do this would be something along the line of adding a "void *amdata" field to IndexOptInfo, and adding an AM callback to be called during get_relation_info that's allowed to fill that in with some AM-specific data (which the AM's costestimate routine would know about). The existing btree-specific hacks in get_relation_info should migrate into btree's version of this callback, and IndexOptInfo.tree_height should probably go away in favor of keeping that in btree's version of the amdata struct. 2. This approach puts a premium on the get_relation_info callback being cheap, because there's no certainty that the data it fills into IndexOptInfo.amdata will ever get used. For btree, the _bt_getrootheight call is cheap enough to not be a problem, because it just looks at the metapage data that btree keeps cached in the index's relcache entry. The same cannot be said for brinGetStats as it stands: it goes off to read the index metapage. There are at least two ways to fix that: 2a. Teach brin to keep the metapage cached like btree does. This seems like it could be a performance win across the board, but you'd need to work out invalidation behavior, and it'd be a bit invasive. 2b. Define IndexOptInfo.amdata as being filled lazily, that is brincostestimate will invoke brinGetStats and fill in the data if the pointer is currently NULL. Then a hypothetical-index plugin could override that by pre-filling the field with the desired fake data. I don't have a problem with allowing brincostestimate to fill in defaults based on BRIN_DEFAULT_PAGES_PER_RANGE if it sees that amdata is null and the index is hypothetical. But there should be a way for the get_relation_info_hook to do better. BTW, the current patch doesn't apply according to the cfbot, but I think it just needs a trivial rebase over 9c703c169 (ie, assume the index is already locked). I didn't bother to do that since what I recommend above would require a lot more change in that area. regards, tom lane
Re: Hypothetical indexes using BRIN broken since pg10
On Fri, Jul 26, 2019 at 1:34 PM Heikki Linnakangas 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, );
> > 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.
Re: Hypothetical indexes using BRIN broken since pg10
On 27/06/2019 23:09, Alvaro Herrera wrote:
On 2019-Jun-27, Tom Lane wrote:
Dunno, I just can't get excited about exposing REVMAP_PAGE_MAXITEMS.
Especially not since we seem to agree on the long-term solution here,
and what you're suggesting to Julien doesn't particularly fit into
that long-term solution.
Well, it was brin_page.h, which is supposed to be internal to BRIN
itself. But since we admit that in its current state selfuncs.c is not
salvageable as a module and we'll redo the whole thing in the short
term, I withdraw my comment.
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, );
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?
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.
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.
- Heikki
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 7ac6d2b339..0a2e7898cc 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -102,6 +102,7 @@
#include
#include "access/brin.h"
+#include "access/brin_page.h"
#include "access/gin.h"
#include "access/htup_details.h"
#include "access/sysattr.h"
@@ -8079,11 +8080,31 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
_seq_page_cost);
/*
- * Obtain some data from the index itself.
+ * Obtain some data from the index itself, if possible. Otherwise invent
+ * some plausible internal statistics based on the relation page count.
*/
- indexRel = index_open(index->indexoid, AccessShareLock);
- brinGetStats(indexRel, );
- index_close(indexRel, AccessShareLock);
+ if (!index->hypothetical)
+ {
+ indexRel = index_open(index->indexoid, AccessShareLock);
+ brinGetStats(indexRel, );
+ index_close(indexRel, AccessShareLock);
+
+ /* work out the actual number of ranges in the index */
+ indexRanges = Max(ceil((double) baserel->pages /
+ statsData.pagesPerRange), 1.0);
+ }
+ 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;
+ }
/*
* Compute index correlation
@@ -8184,10 +8205,6 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
baserel->relid,
JOIN_INNER, NULL);
- /* work out the actual number of ranges in the index */
- indexRanges = Max(ceil((double) baserel->pages / statsData.pagesPerRange),
- 1.0);
-
/*
* Now calculate the minimum possible ranges we could match with if all of
* the rows were in the perfect order in the table's heap.
Re: Hypothetical indexes using BRIN broken since pg10
On Thu, Jun 27, 2019 at 10:09 PM Alvaro Herrera wrote: > > On 2019-Jun-27, Tom Lane wrote: > > > Dunno, I just can't get excited about exposing REVMAP_PAGE_MAXITEMS. > > Especially not since we seem to agree on the long-term solution here, > > and what you're suggesting to Julien doesn't particularly fit into > > that long-term solution. > > Well, it was brin_page.h, which is supposed to be internal to BRIN > itself. But since we admit that in its current state selfuncs.c is not > salvageable as a module and we'll redo the whole thing in the short > term, I withdraw my comment. Thanks. I'll also work soon on a patch to move the [am]costestimate functions in the am-specific files.
Re: Hypothetical indexes using BRIN broken since pg10
On 2019-Jun-27, Tom Lane wrote: > Dunno, I just can't get excited about exposing REVMAP_PAGE_MAXITEMS. > Especially not since we seem to agree on the long-term solution here, > and what you're suggesting to Julien doesn't particularly fit into > that long-term solution. Well, it was brin_page.h, which is supposed to be internal to BRIN itself. But since we admit that in its current state selfuncs.c is not salvageable as a module and we'll redo the whole thing in the short term, I withdraw my comment. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hypothetical indexes using BRIN broken since pg10
Alvaro Herrera writes: > On 2019-Jun-27, Tom Lane wrote: >> Um ... it's accounting for revmap pages already (which is why it needs >> this field set), so hasn't that ship sailed? > Yes, but does it need to know how many items there are in a revmap page? Dunno, I just can't get excited about exposing REVMAP_PAGE_MAXITEMS. Especially not since we seem to agree on the long-term solution here, and what you're suggesting to Julien doesn't particularly fit into that long-term solution. regards, tom lane
Re: Hypothetical indexes using BRIN broken since pg10
On 2019-Jun-27, Tom Lane wrote: > Alvaro Herrera writes: > > On 2019-Jun-27, Tom Lane wrote: > >> FWIW, the proposed patch doesn't seem to me like it adds much more > >> BRIN-specific knowledge to brincostestimate than is there already. > > > It's this calculation that threw me off: > > statsData.revmapNumPages = (indexRanges / REVMAP_PAGE_MAXITEMS) + 1; > > ISTM that selfuncs has no reason to learn about revmap low-level > > details. > > Um ... it's accounting for revmap pages already (which is why it needs > this field set), so hasn't that ship sailed? Yes, but does it need to know how many items there are in a revmap page? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hypothetical indexes using BRIN broken since pg10
Alvaro Herrera writes: > On 2019-Jun-27, Tom Lane wrote: >> FWIW, the proposed patch doesn't seem to me like it adds much more >> BRIN-specific knowledge to brincostestimate than is there already. > It's this calculation that threw me off: > statsData.revmapNumPages = (indexRanges / REVMAP_PAGE_MAXITEMS) + 1; > ISTM that selfuncs has no reason to learn about revmap low-level > details. Um ... it's accounting for revmap pages already (which is why it needs this field set), so hasn't that ship sailed? regards, tom lane
Re: Hypothetical indexes using BRIN broken since pg10
On 2019-Jun-27, Tom Lane wrote: > FWIW, the proposed patch doesn't seem to me like it adds much more > BRIN-specific knowledge to brincostestimate than is there already. It's this calculation that threw me off: statsData.revmapNumPages = (indexRanges / REVMAP_PAGE_MAXITEMS) + 1; ISTM that selfuncs has no reason to learn about revmap low-level details. > I think a more useful response to your modularity concern would be > to move all the [indextype]costestimate functions out of the common > selfuncs.c file and into per-AM files. Yeah, that would be nice, but then I'm not going to push Julien to do that to fix just this one problem; and on the other hand, that's even less of a back-patchable fix. > I fooled around with that while trying to refactor selfuncs.c back in > February, but I didn't come up with something that seemed clearly > better. Still, as we move into a world with external index AMs, I > think we're going to have to make that happen eventually. No disagreement. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hypothetical indexes using BRIN broken since pg10
Alvaro Herrera writes: > On 2019-Jun-27, Julien Rouhaud wrote: >> On Thu, Jun 27, 2019 at 8:14 PM Alvaro Herrera >> wrote: >>> I think it would look nicer to have a routine parallel to brinGetStats() >>> (brinGetStatsHypothetical?), instead of polluting selfuncs.c with these >>> gory details. >> I'm not opposed to it, but I used the same approach as a similar fix >> for gincostestimate() (see 7fb008c5ee5). > How many #define lines did you have to add to selfuncs there? FWIW, the proposed patch doesn't seem to me like it adds much more BRIN-specific knowledge to brincostestimate than is there already. I think a more useful response to your modularity concern would be to move all the [indextype]costestimate functions out of the common selfuncs.c file and into per-AM files. I fooled around with that while trying to refactor selfuncs.c back in February, but I didn't come up with something that seemed clearly better. Still, as we move into a world with external index AMs, I think we're going to have to make that happen eventually. regards, tom lane
Re: Hypothetical indexes using BRIN broken since pg10
On 2019-Jun-27, Julien Rouhaud wrote: > On Thu, Jun 27, 2019 at 8:14 PM Alvaro Herrera > wrote: > > I think it would look nicer to have a routine parallel to brinGetStats() > > (brinGetStatsHypothetical?), instead of polluting selfuncs.c with these > > gory details. > > I'm not opposed to it, but I used the same approach as a similar fix > for gincostestimate() (see 7fb008c5ee5). How many #define lines did you have to add to selfuncs there? > If we add an hypothetical > version of brinGetStats(), we should also do it for ginGetStats(). Dunno, seems pointless. The GIN case is different. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Hypothetical indexes using BRIN broken since pg10
On Thu, Jun 27, 2019 at 8:14 PM Alvaro Herrera wrote: > > Hi, thanks for the patch. Thanks for looking at it! > On 2019-Jun-27, Julien Rouhaud wrote: > > > I just realized that 7e534adcdc7 broke support for hypothetical > > indexes using BRIN am. Attached patch fix the issue. > > > > There's no interface to provide the hypothetical pagesPerRange value, > > so I used the default one, and used simple estimates. > > I think it would look nicer to have a routine parallel to brinGetStats() > (brinGetStatsHypothetical?), instead of polluting selfuncs.c with these > gory details. I'm not opposed to it, but I used the same approach as a similar fix for gincostestimate() (see 7fb008c5ee5). If we add an hypothetical version of brinGetStats(), we should also do it for ginGetStats(). > This seems back-patchable ... I definitely hope so!
Re: Hypothetical indexes using BRIN broken since pg10
Hi, thanks for the patch. On 2019-Jun-27, Julien Rouhaud wrote: > I just realized that 7e534adcdc7 broke support for hypothetical > indexes using BRIN am. Attached patch fix the issue. > > There's no interface to provide the hypothetical pagesPerRange value, > so I used the default one, and used simple estimates. I think it would look nicer to have a routine parallel to brinGetStats() (brinGetStatsHypothetical?), instead of polluting selfuncs.c with these gory details. This seems back-patchable ... -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hypothetical indexes using BRIN broken since pg10
Hello,
I just realized that 7e534adcdc7 broke support for hypothetical
indexes using BRIN am. Attached patch fix the issue.
There's no interface to provide the hypothetical pagesPerRange value,
so I used the default one, and used simple estimates.
I'll add this patch to the next commitfest.
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index d7e3f09f1a..64f27369c0 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -102,6 +102,7 @@
#include
#include "access/brin.h"
+#include "access/brin_page.h"
#include "access/gin.h"
#include "access/htup_details.h"
#include "access/sysattr.h"
@@ -6802,12 +6803,26 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
_seq_page_cost);
/*
- * Obtain some data from the index itself. A lock should have already
- * been obtained on the index in plancat.c.
+ * Obtain some data from the index itself, if possible. Otherwise invent
+ * some plausible internal statistics based on the relation page count.
*/
- indexRel = index_open(index->indexoid, NoLock);
- brinGetStats(indexRel, );
- index_close(indexRel, NoLock);
+ if (!index->hypothetical)
+ {
+ /*
+ * A lock should have already been obtained on the index in plancat.c.
+ */
+ indexRel = index_open(index->indexoid, NoLock);
+ brinGetStats(indexRel, );
+ index_close(indexRel, NoLock);
+ }
+ else
+ {
+ 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;
+ }
/*
* Compute index correlation
