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 &rel->pages, &rel->tuples, &rel->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 <math.h>
#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 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count,
&spc_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, &statsData);
- 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, &statsData);
+ index_close(indexRel, NoLock);
+
+ /* 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
@@ -6970,10 +6994,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.
signature.asc
Description: PGP signature
