public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 9/9] Ignore correlation for new BRIN opclasses 3+ messages / 3 participants [nested] [flat]
* [PATCH 9/9] Ignore correlation for new BRIN opclasses @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 3+ 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 15a8258677..d2c4172f48 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -410,6 +410,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 02aa618b49..437efa8d31 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1798,6 +1798,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 47ca4ddbb5..bf40f9ce32 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 --------------4B194FF8F3EA3786FF9EAE1F Content-Type: text/x-patch; charset=UTF-8; name="0008-Define-multi-minmax-oclasses-for-types-with-20210203.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0008-Define-multi-minmax-oclasses-for-types-with-20210203.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: [PATCH v2] use has_privs_for_role for predefined roles @ 2022-02-06 17:24 Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Tom Lane @ 2022-02-06 17:24 UTC (permalink / raw) To: Joe Conway <[email protected]>; +Cc: Joshua Brindle <[email protected]>; Bossart, Nathan <[email protected]>; Stephen Frost <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers Joe Conway <[email protected]> writes: > I'd like to pick this patch up and see it through to commit/push. > Presumably that will include back-patching to all supported pg versions. > Before I go through the effort to back-patch, does anyone want to argue > that this should *not* be back-patched? Hm, I'm -0.5 or so. I think changing security-related behaviors in a stable branch is a hard sell unless you are closing a security hole. This is a fine improvement for HEAD but I'm inclined to leave the back branches alone. regards, tom lane ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: [PATCH v2] use has_privs_for_role for predefined roles @ 2022-02-07 15:35 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Robert Haas @ 2022-02-07 15:35 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Joe Conway <[email protected]>; Joshua Brindle <[email protected]>; Bossart, Nathan <[email protected]>; Stephen Frost <[email protected]>; pgsql-hackers On Sun, Feb 6, 2022 at 12:24 PM Tom Lane <[email protected]> wrote: > Joe Conway <[email protected]> writes: > > I'd like to pick this patch up and see it through to commit/push. > > Presumably that will include back-patching to all supported pg versions. > > Before I go through the effort to back-patch, does anyone want to argue > > that this should *not* be back-patched? > > Hm, I'm -0.5 or so. I think changing security-related behaviors > in a stable branch is a hard sell unless you are closing a security > hole. This is a fine improvement for HEAD but I'm inclined to > leave the back branches alone. I think the threshold to back-patch a clear behavior change is pretty high, so I do not think it should be back-patched. I am also not convinced that a sufficient argument has been made for changing this in master. This isn't the only thread where NOINHERIT has come up lately, and I have to admit that I'm not a fan. Let's suppose that I have two users, let's say sunita and sri. In the real world, Sunita is Sri's manager and needs to be able to perform actions as Sri when Sri is out of the office, but it isn't desirable for Sunita to have Sri's privileges in all situations all the time. So we mark role sunita as NOINHERIT and grant sri to sunita. Then it turns out that Sunita also needs to be COPY TO/FROM PROGRAM, so we give her pg_execute_server_program. Now, if she can't exercise this privilege without setting role to the prefined role, that's bad, isn't it? I mean, we want her to be able to copy between *her* tables and various shell commands, not the tables owned by pg_execute_server_program, of which there are presumably none. It seems to me that the INHERIT role flag isn't very well-considered. Inheritance, or the lack of it, ought to be decided separately for each inherited role. However, that would be a major architectural change. But in the absence of that, it seems clearly better for predefined roles to disregard INHERIT and just always grant the rights they are intended to give. Because if we don't do that, then we end up with people having to SET ROLE to the predefined role and perform actions directly as that role, which seems like it can't be what we want. I almost feel like we ought to be looking for ways of preventing people from doing SET ROLE to a predefined role altogether, not encouraging them to do it. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-02-07 15:35 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 9/9] Ignore correlation for new BRIN opclasses Tomas Vondra <[email protected]> 2022-02-06 17:24 Re: [PATCH v2] use has_privs_for_role for predefined roles Tom Lane <[email protected]> 2022-02-07 15:35 ` Re: [PATCH v2] use has_privs_for_role for predefined roles Robert Haas <[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