public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 8/8] Ignore correlation for new BRIN opclasses 4+ messages / 4 participants [nested] [flat]
* [PATCH 8/8] Ignore correlation for new BRIN opclasses @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Tomas Vondra @ 2020-09-12 13:07 UTC (permalink / raw) The new BRIN opclasses (bloom and minmax-multi) are less sensitive to poorly correlated data, so just assume the data is perfectly correlated during costing. Author: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/backend/access/brin/brin_bloom.c | 1 + src/backend/access/brin/brin_minmax_multi.c | 1 + src/backend/utils/adt/selfuncs.c | 19 ++++++++++++++++++- src/include/access/brin_internal.h | 3 +++ 4 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c index 4494484b3b..46ffc068be 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -412,6 +412,7 @@ brin_bloom_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(BloomOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (BloomOpaque *) diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index c90c721cac..a6f79311e2 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1737,6 +1737,7 @@ brin_minmax_multi_opcinfo(PG_FUNCTION_ARGS) result = palloc0(MAXALIGN(SizeofBrinOpcInfo(1)) + sizeof(MinmaxMultiOpaque)); + result->oi_ignore_correlation = true; result->oi_nstored = 1; result->oi_regular_nulls = true; result->oi_opaque = (MinmaxMultiOpaque *) diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 52314d3aa1..0320d128f6 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -98,6 +98,7 @@ #include <math.h> #include "access/brin.h" +#include "access/brin_internal.h" #include "access/brin_page.h" #include "access/gin.h" #include "access/table.h" @@ -7352,7 +7353,8 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, double minimalRanges; double estimatedRanges; double selec; - Relation indexRel; + Relation indexRel = NULL; + TupleDesc tupdesc = NULL; ListCell *l; VariableStatData vardata; @@ -7374,6 +7376,7 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, */ indexRel = index_open(index->indexoid, NoLock); brinGetStats(indexRel, &statsData); + tupdesc = RelationGetDescr(indexRel); index_close(indexRel, NoLock); /* work out the actual number of ranges in the index */ @@ -7407,6 +7410,17 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, { IndexClause *iclause = lfirst_node(IndexClause, l); AttrNumber attnum = index->indexkeys[iclause->indexcol]; + FmgrInfo *opcInfoFn; + BrinOpcInfo *opcInfo; + Form_pg_attribute attr = TupleDescAttr(tupdesc, iclause->indexcol); + bool ignore_correlation; + + opcInfoFn = index_getprocinfo(indexRel, iclause->indexcol + 1, BRIN_PROCNUM_OPCINFO); + + opcInfo = (BrinOpcInfo *) + DatumGetPointer(FunctionCall1(opcInfoFn, attr->atttypid)); + + ignore_correlation = opcInfo->oi_ignore_correlation; /* attempt to lookup stats in relation for this index column */ if (attnum != 0) @@ -7477,6 +7491,9 @@ brincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, if (sslot.nnumbers > 0) varCorrelation = Abs(sslot.numbers[0]); + if (ignore_correlation) + varCorrelation = 1.0; + if (varCorrelation > *indexCorrelation) *indexCorrelation = varCorrelation; diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h index fdaff42722..5fbf8cf9c7 100644 --- a/src/include/access/brin_internal.h +++ b/src/include/access/brin_internal.h @@ -30,6 +30,9 @@ typedef struct BrinOpcInfo /* Regular processing of NULLs in BrinValues? */ bool oi_regular_nulls; + /* Ignore correlation during cost estimation */ + bool oi_ignore_correlation; + /* Opaque pointer for the opclass' private use */ void *oi_opaque; -- 2.26.2 --------------AC3FFB734F56C5F52D422EFC-- ^ permalink raw reply [nested|flat] 4+ messages in thread
* set_cheapest without checking pathlist @ 2024-02-01 02:04 James Coleman <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: James Coleman @ 2024-02-01 02:04 UTC (permalink / raw) To: pgsql-hackers; +Cc: Robert Haas <[email protected]> Hello, Robert: I've taken the liberty of cc'ing you since you worked on most of this code. My apologies if that wasn't appropriate. While working on "Parallelize correlated subqueries that execute within each worker" [1] I noticed that while in the other call to set_cheapest (for partially_grouped_rel) in the same function the call after gather_grouping_paths(root, partially_grouped_rel) is not similarly guarded with a check for a NIL pathlist on partially_grouped_rel. I don't see any inherent reason why we must always assume that gather_grouping_paths will always result in having at least one entry in pathlist. If, for example, we've disabled incremental sort and the cheapest partial path happens to already be sorted, then I don't believe we'll add any paths. And if that happens then set_cheapest will error with the message "could not devise a query plan for the given query". So I propose we add a similar guard to this call point. I could be convinced that this should be simply part of the patch in the other thread, but it seemed to me it'd be worth considering independently because as noted above I don't see any reason why this couldn't happen separately. That being said, on master I don't have a case showing this is necessary. Thanks, James Coleman 1: https://www.postgresql.org/message-id/flat/CAAaqYe-Aq6oNf9NPZnpPE7SgRLomXXWJA1Gz9L9ndi_Nv%3D94Yw%40m... Attachments: [application/octet-stream] v1-0001-Guard-set_cheapest-with-pathlist-NIL-check.patch (1023B, ../../CAAaqYe-jemDqoAr0CtHjXFGw3uq+v7=F2HcGeHhO58_u0xjoqA@mail.gmail.com/2-v1-0001-Guard-set_cheapest-with-pathlist-NIL-check.patch) download | inline diff: From 37633dddd28c6af114a94ec9d045005bcd7fb4ed Mon Sep 17 00:00:00 2001 From: jcoleman <[email protected]> Date: Wed, 31 Jan 2024 20:47:17 -0500 Subject: [PATCH v1] Guard set_cheapest with pathlist NIL check We do this elsewhere, and there's no reason we have to be adding entries to pathlist in the gather_grouping_paths call. --- src/backend/optimizer/plan/planner.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 2e2458b128..3214b6e863 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -3945,7 +3945,8 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, if (partially_grouped_rel && partially_grouped_rel->partial_pathlist) { gather_grouping_paths(root, partially_grouped_rel); - set_cheapest(partially_grouped_rel); + if (partially_grouped_rel->pathlist) + set_cheapest(partially_grouped_rel); } /* -- 2.39.3 (Apple Git-145) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: set_cheapest without checking pathlist @ 2024-02-01 03:28 Richard Guo <[email protected]> parent: James Coleman <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Richard Guo @ 2024-02-01 03:28 UTC (permalink / raw) To: James Coleman <[email protected]>; +Cc: pgsql-hackers; Robert Haas <[email protected]> On Thu, Feb 1, 2024 at 10:04 AM James Coleman <[email protected]> wrote: > I don't see any inherent reason why we must always assume that > gather_grouping_paths will always result in having at least one entry > in pathlist. If, for example, we've disabled incremental sort and the > cheapest partial path happens to already be sorted, then I don't > believe we'll add any paths. And if that happens then set_cheapest > will error with the message "could not devise a query plan for the > given query". So I propose we add a similar guard to this call point. I don't believe that would happen. It seems to me that we should, at the very least, have a path which is Gather on top of the cheapest partial path (see generate_gather_paths), as long as the partially_grouped_rel has partial paths. Thanks Richard ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: set_cheapest without checking pathlist @ 2024-02-01 03:37 David Rowley <[email protected]> parent: Richard Guo <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: David Rowley @ 2024-02-01 03:37 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: James Coleman <[email protected]>; pgsql-hackers; Robert Haas <[email protected]> On Thu, 1 Feb 2024 at 16:29, Richard Guo <[email protected]> wrote: > > > On Thu, Feb 1, 2024 at 10:04 AM James Coleman <[email protected]> wrote: >> >> I don't see any inherent reason why we must always assume that >> gather_grouping_paths will always result in having at least one entry >> in pathlist. If, for example, we've disabled incremental sort and the >> cheapest partial path happens to already be sorted, then I don't >> believe we'll add any paths. And if that happens then set_cheapest >> will error with the message "could not devise a query plan for the >> given query". So I propose we add a similar guard to this call point. > > > I don't believe that would happen. It seems to me that we should, at > the very least, have a path which is Gather on top of the cheapest > partial path (see generate_gather_paths), as long as the > partially_grouped_rel has partial paths. It will have partial paths because it's nested inside "if (partially_grouped_rel && partially_grouped_rel->partial_pathlist)" David ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-02-01 03:37 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 8/8] Ignore correlation for new BRIN opclasses Tomas Vondra <[email protected]> 2024-02-01 02:04 set_cheapest without checking pathlist James Coleman <[email protected]> 2024-02-01 03:28 ` Re: set_cheapest without checking pathlist Richard Guo <[email protected]> 2024-02-01 03:37 ` Re: set_cheapest without checking pathlist David Rowley <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox