public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 6/6] Ignore correlation for new BRIN opclasses 2+ messages / 2 participants [nested] [flat]
* [PATCH 6/6] Ignore correlation for new BRIN opclasses @ 2020-09-12 13:07 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 2+ 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 000c2387ee..0137095660 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -385,6 +385,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 bd17da8cd5..6ef5c5f2bf 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1309,6 +1309,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 d5e61664bc..d0cc145938 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 --------------310A2AE1CC4C2E2E77559E3D-- ^ permalink raw reply [nested|flat] 2+ messages in thread
* pg_stop_backup() v2 incorrectly marked as proretset @ 2022-03-02 07:46 Michael Paquier <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Michael Paquier @ 2022-03-02 07:46 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, In my hunt looking for incorrect SRFs, I have noticed a new case of a system function marked as proretset while it builds and returns only one record. And this is a popular one: pg_stop_backup(), labelled v2. This leads to a lot of unnecessary work, as the function creates a tuplestore it has no need for with the usual set of checks related to SRFs. The logic can be be simplified as of the attached. Thoughts? -- Michael Attachments: [text/x-diff] stopbackup-no-srf.patch (4.0K, ../../[email protected]/2-stopbackup-no-srf.patch) download | inline diff: diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index bf88858171..d8e8715ed1 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6275,9 +6275,9 @@ proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r', prorettype => 'pg_lsn', proargtypes => '', prosrc => 'pg_stop_backup' }, { oid => '2739', descr => 'finish taking an online backup', - proname => 'pg_stop_backup', prorows => '1', proretset => 't', - provolatile => 'v', proparallel => 'r', prorettype => 'record', - proargtypes => 'bool bool', proallargtypes => '{bool,bool,pg_lsn,text,text}', + proname => 'pg_stop_backup', provolatile => 'v', proparallel => 'r', + prorettype => 'record', proargtypes => 'bool bool', + proallargtypes => '{bool,bool,pg_lsn,text,text}', proargmodes => '{i,i,o,o,o}', proargnames => '{exclusive,wait_for_archive,lsn,labelfile,spcmapfile}', prosrc => 'pg_stop_backup_v2' }, diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c index 12e2bf4135..2f292e2bd8 100644 --- a/src/backend/access/transam/xlogfuncs.c +++ b/src/backend/access/transam/xlogfuncs.c @@ -165,11 +165,9 @@ pg_stop_backup(PG_FUNCTION_ARGS) Datum pg_stop_backup_v2(PG_FUNCTION_ARGS) { +#define PG_STOP_BACKUP_V2_COLS 3 ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; TupleDesc tupdesc; - Tuplestorestate *tupstore; - MemoryContext per_query_ctx; - MemoryContext oldcontext; Datum values[3]; bool nulls[3]; @@ -178,29 +176,15 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) XLogRecPtr stoppoint; SessionBackupState status = get_backup_status(); - /* check to see if caller supports us returning a tuplestore */ - if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("set-valued function called in context that cannot accept a set"))); - if (!(rsinfo->allowedModes & SFRM_Materialize)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("materialize mode required, but it is not allowed in this context"))); - - /* Build a tuple descriptor for our result type */ - if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) - elog(ERROR, "return type must be a row type"); - - per_query_ctx = rsinfo->econtext->ecxt_per_query_memory; - oldcontext = MemoryContextSwitchTo(per_query_ctx); - - tupstore = tuplestore_begin_heap(true, false, work_mem); - rsinfo->returnMode = SFRM_Materialize; - rsinfo->setResult = tupstore; - rsinfo->setDesc = tupdesc; - - MemoryContextSwitchTo(oldcontext); + /* Initialise attributes information in the tuple descriptor */ + tupdesc = CreateTemplateTupleDesc(PG_STOP_BACKUP_V2_COLS); + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "lsn", + PG_LSNOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "labelfile", + TEXTOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 3, "spcmapfile", + TEXTOID, -1, 0); + BlessTupleDesc(tupdesc); MemSet(values, 0, sizeof(values)); MemSet(nulls, 0, sizeof(nulls)); @@ -251,9 +235,8 @@ pg_stop_backup_v2(PG_FUNCTION_ARGS) /* Stoppoint is included on both exclusive and nonexclusive backups */ values[0] = LSNGetDatum(stoppoint); - tuplestore_putvalues(tupstore, tupdesc, values, nulls); - - return (Datum) 0; + /* Returns the record as Datum */ + PG_RETURN_DATUM(HeapTupleGetDatum(heap_form_tuple(tupdesc, values, nulls))); } /* diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql index 758ab6e25a..81bac6f581 100644 --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -384,7 +384,7 @@ CREATE OR REPLACE FUNCTION CREATE OR REPLACE FUNCTION pg_stop_backup ( exclusive boolean, wait_for_archive boolean DEFAULT true, OUT lsn pg_lsn, OUT labelfile text, OUT spcmapfile text) - RETURNS SETOF record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2' + RETURNS record STRICT VOLATILE LANGUAGE internal as 'pg_stop_backup_v2' PARALLEL RESTRICTED; CREATE OR REPLACE FUNCTION [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2022-03-02 07:46 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-09-12 13:07 [PATCH 6/6] Ignore correlation for new BRIN opclasses Tomas Vondra <[email protected]> 2022-03-02 07:46 pg_stop_backup() v2 incorrectly marked as proretset Michael Paquier <[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