public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 8/8] Ignore correlation for new BRIN opclasses 4+ messages / 3 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 87a3d5598a..4e0712cfb2 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -817,6 +817,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 f9a7ae6608..4f6262f241 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1306,6 +1306,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 80bd60f876..e76593043f 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 ee4d0706df..67aea62a02 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 --------------45196B023835614BDFCBD16D Content-Type: text/x-patch; charset=UTF-8; name="0007-BRIN-minmax-multi-indexes-20201220.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0007-BRIN-minmax-multi-indexes-20201220.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* pg_stat_statements: Avoid holding excessive lock @ 2024-11-05 17:37 Karina Litskevich <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Karina Litskevich @ 2024-11-05 17:37 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi hackers, In pg_stat_statements there is entry's mutex spinlock which is used to be able to modify counters without holding pgss->lock exclusively. Here is a comment from the top of pg_stat_statements.c: * Note about locking issues: to create or delete an entry in the shared * hashtable, one must hold pgss->lock exclusively. Modifying any field * in an entry except the counters requires the same. To look up an entry, * one must hold the lock shared. To read or update the counters within * an entry, one must hold the lock shared or exclusive (so the entry doesn't * disappear!) and also take the entry's mutex spinlock. * The shared state variable pgss->extent (the next free spot in the external * query-text file) should be accessed only while holding either the * pgss->mutex spinlock, or exclusive lock on pgss->lock. We use the mutex to * allow reserving file space while holding only shared lock on pgss->lock. * Rewriting the entire external query-text file, eg for garbage collection, * requires holding pgss->lock exclusively; this allows individual entries * in the file to be read or written while holding only shared lock. And a comment from pgssEntry declaration: slock_t mutex; /* protects the counters only */ Both comments say that spinlocking mutex should be used to protect counters only. But in pg_stat_statements_internal we do read entry->stats_since and entry->minmax_stats_since holding the entry's mutex spinlock. This is unnecessary since we only modify stats_since and minmax_stats_since while holding pgss->lock exclusively, and in pg_stat_statements_internal we are holding pgss->lock when reading them. So even without holding the entry's mutex spinlock there should be no race. I suggest eliminating holding the excessive lock. See the attached patch. This would also restore the consistency between the code and the comments about entry's mutex spinlock usage. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/ Attachments: [text/x-patch] v1-0001-pg_stat_statements-Avoid-excessive-lock-holding.patch (1.8K, ../../CACiT8ibhCmzbcOxM0v4pRLH3abk-95LPkt7_uC2JMP+miPjxsg@mail.gmail.com/2-v1-0001-pg_stat_statements-Avoid-excessive-lock-holding.patch) download | inline diff: From c817fc9373ca08488088414a052eaf77dbb55bde Mon Sep 17 00:00:00 2001 From: Karina Litskevich <[email protected]> Date: Tue, 5 Nov 2024 20:27:25 +0300 Subject: [PATCH v1] pg_stat_statements: Avoid excessive lock holding Entry's mutex spinlock is used to be able to modify counters without holding pgss->lock exclusively (though holding it at least shared so the entry doesn't disappear). We never actually modify stats_since and minmax_stats_since without holding pgss->lock exclusively, so there is no need to hold entry's mutex spinlock when reading stats_since and minmax_stats_since. This also restores the consistency between the code and the comments about entry's mutex spinlock usage. --- contrib/pg_stat_statements/pg_stat_statements.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 1798e1d016..a66ad99a37 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1869,9 +1869,16 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, /* copy counters to a local variable to keep locking time short */ SpinLockAcquire(&entry->mutex); tmp = entry->counters; + SpinLockRelease(&entry->mutex); + + /* + * There is no need to hold entry->mutex when reading stats_since and + * minmax_stats_since for (unlike counters) they are always written + * while holding pgss->lock exclusively. We are holding pgss->lock + * shared so there should be no race here. + */ stats_since = entry->stats_since; minmax_stats_since = entry->minmax_stats_since; - SpinLockRelease(&entry->mutex); /* Skip entry if unexecuted (ie, it's a pending "sticky" entry) */ if (IS_STICKY(tmp)) -- 2.34.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pg_stat_statements: Avoid holding excessive lock @ 2024-11-07 08:16 Michael Paquier <[email protected]> parent: Karina Litskevich <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michael Paquier @ 2024-11-07 08:16 UTC (permalink / raw) To: Karina Litskevich <[email protected]>; +Cc: Postgres hackers <[email protected]> On Tue, Nov 05, 2024 at 08:37:08PM +0300, Karina Litskevich wrote: > I suggest eliminating holding the excessive lock. See the attached patch. > This would also restore the consistency between the code and the comments > about entry's mutex spinlock usage. You are right. minmax_stats_since and stats_since are only set when an entry is allocated or reset, so this is not going to matter. > + /* > + * There is no need to hold entry->mutex when reading stats_since and > + * minmax_stats_since for (unlike counters) they are always written > + * while holding pgss->lock exclusively. We are holding pgss->lock > + * shared so there should be no race here. > + */ > stats_since = entry->stats_since; > minmax_stats_since = entry->minmax_stats_since; > - SpinLockRelease(&entry->mutex); The comment could be simpler, say a "The spinlock is not required when reading these two as they are always updated when holding pgss->lock exclusively.". Or something like that. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pg_stat_statements: Avoid holding excessive lock @ 2024-11-07 13:08 Karina Litskevich <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Karina Litskevich @ 2024-11-07 13:08 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]> On Thu, Nov 7, 2024 at 11:17 AM Michael Paquier <[email protected]> wrote: > The comment could be simpler, say a "The spinlock is not required when > reading these two as they are always updated when holding pgss->lock > exclusively.". Or something like that. Thank you for your feedback and the shorter wording of the comment. I used it in the new version of the patch. Best regards, Karina Litskevich Postgres Professional: http://postgrespro.com/ Attachments: [text/x-patch] v2-0001-pg_stat_statements-Avoid-excessive-lock-holding.patch (1.6K, ../../CACiT8ia7ya4DVxZ3w-Jx4M7LwPNdaAXVf99E5LCYHq+cSXZXxg@mail.gmail.com/2-v2-0001-pg_stat_statements-Avoid-excessive-lock-holding.patch) download | inline diff: From 4b7f7ea0c9923fa9d2622d86aa1214f331c600ec Mon Sep 17 00:00:00 2001 From: Karina Litskevich <[email protected]> Date: Thu, 7 Nov 2024 15:54:32 +0300 Subject: [PATCH v2] pg_stat_statements: Avoid excessive lock holding Entry's mutex spinlock is used to be able to modify counters without holding pgss->lock exclusively (though holding it at least shared so the entry doesn't disappear). We never actually modify stats_since and minmax_stats_since without holding pgss->lock exclusively, so there is no need to hold entry's mutex spinlock when reading stats_since and minmax_stats_since. This also restores the consistency between the code and the comments about entry's mutex spinlock usage. --- contrib/pg_stat_statements/pg_stat_statements.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 1798e1d016..49c657b3e0 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -1869,9 +1869,14 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo, /* copy counters to a local variable to keep locking time short */ SpinLockAcquire(&entry->mutex); tmp = entry->counters; + SpinLockRelease(&entry->mutex); + + /* + * The spinlock is not required when reading these two as they are + * always updated when holding pgss->lock exclusively. + */ stats_since = entry->stats_since; minmax_stats_since = entry->minmax_stats_since; - SpinLockRelease(&entry->mutex); /* Skip entry if unexecuted (ie, it's a pending "sticky" entry) */ if (IS_STICKY(tmp)) -- 2.34.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-11-07 13:08 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-11-05 17:37 pg_stat_statements: Avoid holding excessive lock Karina Litskevich <[email protected]> 2024-11-07 08:16 ` Re: pg_stat_statements: Avoid holding excessive lock Michael Paquier <[email protected]> 2024-11-07 13:08 ` Re: pg_stat_statements: Avoid holding excessive lock Karina Litskevich <[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