public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v22 2/5] Deprecate ReindexStmt->concurrent.. 5+ messages / 3 participants [nested] [flat]
* [PATCH v22 2/5] Deprecate ReindexStmt->concurrent.. @ 2020-08-11 06:41 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-08-11 06:41 UTC (permalink / raw) ..since parenthesized syntax is the modern syntax, remove the special field for handling nonparenthesized option. --- src/backend/commands/indexcmds.c | 20 +++++++++++--------- src/backend/nodes/copyfuncs.c | 1 - src/backend/nodes/equalfuncs.c | 1 - src/backend/parser/gram.y | 16 ++++++++++++---- src/backend/tcop/utility.c | 6 +++--- src/include/nodes/parsenodes.h | 2 +- 6 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a22b32fc74..097d8720a4 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -97,7 +97,7 @@ static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts); */ struct ReindexIndexCallbackState { - bool concurrent; /* flag from statement */ + bool concurrent; /* flag from statement XXX */ Oid locked_table_oid; /* tracks previously locked table */ }; @@ -2438,10 +2438,10 @@ ReindexIndex(ReindexStmt *stmt) * upgrade the lock, but that's OK, because other sessions can't hold * locks on our temporary table. */ - state.concurrent = stmt->concurrent; + state.concurrent = (stmt->options & REINDEXOPT_CONCURRENT); state.locked_table_oid = InvalidOid; indOid = RangeVarGetRelidExtended(indexRelation, - stmt->concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, + state.concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock, 0, RangeVarCallbackForReindexIndex, &state); @@ -2461,7 +2461,7 @@ ReindexIndex(ReindexStmt *stmt) persistence = irel->rd_rel->relpersistence; index_close(irel, NoLock); - if (stmt->concurrent && persistence != RELPERSISTENCE_TEMP) + if (state.concurrent && persistence != RELPERSISTENCE_TEMP) ReindexRelationConcurrently(indOid, stmt->options); else reindex_index(indOid, false, persistence, @@ -2548,6 +2548,7 @@ ReindexTable(ReindexStmt *stmt) RangeVar *relation = stmt->relation; Oid heapOid; bool result; + bool concurrent = (stmt->options & REINDEXOPT_CONCURRENT); /* * The lock level used here should match reindex_relation(). @@ -2558,11 +2559,11 @@ ReindexTable(ReindexStmt *stmt) * locks on our temporary table. */ heapOid = RangeVarGetRelidExtended(relation, - stmt->concurrent ? ShareUpdateExclusiveLock : ShareLock, + concurrent ? ShareUpdateExclusiveLock : ShareLock, 0, RangeVarCallbackOwnsTable, NULL); - if (stmt->concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) + if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP) { result = ReindexRelationConcurrently(heapOid, stmt->options); @@ -2610,13 +2611,14 @@ ReindexMultipleTables(ReindexStmt *stmt) ListCell *l; int num_keys; bool concurrent_warning = false; + bool concurrent = (stmt->options & REINDEXOPT_CONCURRENT); AssertArg(objectName); Assert(objectKind == REINDEX_OBJECT_SCHEMA || objectKind == REINDEX_OBJECT_SYSTEM || objectKind == REINDEX_OBJECT_DATABASE); - if (objectKind == REINDEX_OBJECT_SYSTEM && stmt->concurrent) + if (objectKind == REINDEX_OBJECT_SYSTEM && concurrent) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot reindex system catalogs concurrently"))); @@ -2727,7 +2729,7 @@ ReindexMultipleTables(ReindexStmt *stmt) * Skip system tables, since index_create() would reject indexing them * concurrently (and it would likely fail if we tried). */ - if (stmt->concurrent && + if (concurrent && IsCatalogRelationOid(relid)) { if (!concurrent_warning) @@ -2769,7 +2771,7 @@ ReindexMultipleTables(ReindexStmt *stmt) /* functions in indexes may want a snapshot set */ PushActiveSnapshot(GetTransactionSnapshot()); - if (stmt->concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) + if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { (void) ReindexRelationConcurrently(relid, stmt->options); /* ReindexRelationConcurrently() does the verbose output */ diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 71548acc0c..a67e637c37 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -4452,7 +4452,6 @@ _copyReindexStmt(const ReindexStmt *from) COPY_STRING_FIELD(name); COPY_SCALAR_FIELD(options); COPY_NODE_FIELD(params); - COPY_SCALAR_FIELD(concurrent); return newnode; } diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index de48a42cdd..efcb23c12e 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -2137,7 +2137,6 @@ _equalReindexStmt(const ReindexStmt *a, const ReindexStmt *b) COMPARE_STRING_FIELD(name); COMPARE_SCALAR_FIELD(options); COMPARE_NODE_FIELD(params); - COMPARE_SCALAR_FIELD(concurrent); return true; } diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index b909b161a6..ac489c03c3 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -8176,40 +8176,48 @@ ReindexStmt: { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $2; - n->concurrent = $3; n->relation = $4; n->name = NULL; n->params = NIL; + if ($3) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @3)); $$ = (Node *)n; } | REINDEX reindex_target_multitable opt_concurrently name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $2; - n->concurrent = $3; n->name = $4; n->relation = NULL; n->params = NIL; + if ($3) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @3)); $$ = (Node *)n; } | REINDEX '(' vac_analyze_option_list ')' reindex_target_type opt_concurrently qualified_name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $5; - n->concurrent = $6; n->relation = $7; n->name = NULL; n->params = $3; + if ($6) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @6)); $$ = (Node *)n; } | REINDEX '(' vac_analyze_option_list ')' reindex_target_multitable opt_concurrently name { ReindexStmt *n = makeNode(ReindexStmt); n->kind = $5; - n->concurrent = $6; n->name = $7; n->relation = NULL; n->params = $3; + if ($6) + n->params = lappend(n->params, + makeDefElem("concurrently", (Node *)makeString("concurrently"), @6)); $$ = (Node *)n; } ; diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index c48983a5de..639b6fce74 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -543,7 +543,7 @@ void parse_reindex_params(ParseState *pstate, ReindexStmt *stmt) stmt->options &= ~REINDEXOPT_VERBOSE; } else if (strcmp(opt->defname, "concurrently") == 0) - stmt->concurrent = true; + stmt->options |= REINDEXOPT_CONCURRENT; else ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -946,11 +946,11 @@ standard_ProcessUtility(PlannedStmt *pstmt, { ReindexStmt *stmt = (ReindexStmt *) parsetree; - if (stmt->concurrent) + parse_reindex_params(pstate, stmt); + if (stmt->options & REINDEXOPT_CONCURRENT) PreventInTransactionBlock(isTopLevel, "REINDEX CONCURRENTLY"); - parse_reindex_params(pstate, stmt); switch (stmt->kind) { case REINDEX_OBJECT_INDEX: diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 295c2802a4..ba685139ef 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3352,6 +3352,7 @@ typedef struct ConstraintsSetStmt /* Reindex options */ #define REINDEXOPT_VERBOSE (1 << 0) /* print progress info */ #define REINDEXOPT_REPORT_PROGRESS (1 << 1) /* report pgstat progress */ +#define REINDEXOPT_CONCURRENT (1 << 2) /* reindex CONCURRENTLY */ typedef enum ReindexObjectType { @@ -3371,7 +3372,6 @@ typedef struct ReindexStmt const char *name; /* name of database to reindex */ int options; /* Reindex options flags */ List *params; /* Params not further parsed by the grammer */ - bool concurrent; /* reindex concurrently? */ } ReindexStmt; /* ---------------------- -- 2.17.0 --g7w8+K/95kPelPD2 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v22-0003-Allow-REINDEX-to-change-tablespace.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Performance issues with parallelism and LIMIT @ 2023-02-01 13:41 David Geier <[email protected]> 2023-02-08 10:42 ` Re: Performance issues with parallelism and LIMIT Tomas Vondra <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David Geier @ 2023-02-01 13:41 UTC (permalink / raw) To: pgsql-hackers; +Cc: Robert Haas <[email protected]>; [email protected] Hi hackers, While migrating from PostgreSQL 14 to 15, we encountered the following performance degradation caused by commit 46846433a03dff: "shm_mq: Update mq_bytes_written less often", discussion in [1]. The batching can make queries with a LIMIT clause run significantly slower compared to PostgreSQL 14, because neither the ring buffer write position is updated, nor the latch to inform the leader that there's data available is set, before a worker's queue is 1/4th full. This can be seen in the number of rows produced by a parallel worker. Worst-case, the data set is large and all rows to answer the query appear early, but are not big enough to fill the queue to 1/4th (e.g. when the LIMIT and the tuple sizes are small). Here is an example to reproduce the problem. CREATE TABLE t(id1 INT, id2 INT, id3 INT, id4 INT, id5 INT); INSERT INTO t(id1, id2, id3, id4, id5) SELECT i%1000, i, i, i, i FROM generate_series(1, 10000000) AS i; ANALYZE t; SET parallel_tuple_cost = 0; SET parallel_setup_cost = 0; SET min_parallel_table_scan_size = 0; SET max_parallel_workers_per_gather = 8; EXPLAIN ANALYZE VERBOSE SELECT id2 FROM t WHERE id1 = 100 LIMIT 100; PostgreSQL 15: Limit (cost=0.00..797.43 rows=100 width=4) (actual time=65.083..69.207 rows=100 loops=1) Output: id2 -> Gather (cost=0.00..79320.18 rows=9947 width=4) (actual time=65.073..68.417 rows=100 loops=1) Output: id2 Workers Planned: 8 Workers Launched: 7 -> Parallel Seq Scan on public.t (cost=0.00..79320.18 rows=1243 width=4) (actual time=0.204..33.049 rows=100 loops=7) Output: id2 Filter: (t.id1 = 100) Rows Removed by Filter: 99345 Worker 0: actual time=0.334..32.284 rows=100 loops=1 Worker 1: actual time=0.060..32.680 rows=100 loops=1 Worker 2: actual time=0.637..33.954 rows=98 loops=1 Worker 3: actual time=0.136..33.301 rows=100 loops=1 Worker 4: actual time=0.140..31.942 rows=100 loops=1 Worker 5: actual time=0.062..33.673 rows=100 loops=1 Worker 6: actual time=0.062..33.512 rows=100 loops=1 Planning Time: 0.113 ms Execution Time: 69.772 ms PostgreSQL 14: Limit (cost=0.00..797.75 rows=100 width=4) (actual time=30.602..38.459 rows=100 loops=1) Output: id2 -> Gather (cost=0.00..79320.18 rows=9943 width=4) (actual time=30.592..37.669 rows=100 loops=1) Output: id2 Workers Planned: 8 Workers Launched: 7 -> Parallel Seq Scan on public.t (cost=0.00..79320.18 rows=1243 width=4) (actual time=0.221..5.181 rows=15 loops=7) Output: id2 Filter: (t.id1 = 100) Rows Removed by Filter: 15241 Worker 0: actual time=0.129..4.840 rows=15 loops=1 Worker 1: actual time=0.125..4.924 rows=15 loops=1 Worker 2: actual time=0.314..5.249 rows=17 loops=1 Worker 3: actual time=0.252..5.341 rows=15 loops=1 Worker 4: actual time=0.163..5.179 rows=15 loops=1 Worker 5: actual time=0.422..5.248 rows=15 loops=1 Worker 6: actual time=0.139..5.489 rows=16 loops=1 Planning Time: 0.084 ms Execution Time: 38.880 ms I had a quick look at the code and I started wondering if we can't achieve the same performance improvement without batching by e.g.: - Only set the latch if new data is written to an empty queue. Otherwise, the leader should anyways keep try reading from the queues without waiting for the latch, so no need to set the latch again. - Reorganize struct shm_mq. There seems to be false sharing happening between at least mq_ring_size and the atomics and potentially also between the atomics. I'm wondering if the that's not the root cause of the "slow atomics" observed in [1]? I'm happy to do some profiling. Alternatively, we could always set the latch if numberTuples in ExecutePlan() is reasonably low. To do so, the DestReceiver's receive() method would only need an additional "force flush" argument. A slightly different but related problem is when some workers have already produced enough rows to answer the LIMIT query, but other workers are still running without producing any new rows. In that case the "already done" workers will stop running even though they haven't reached 1/4th of the queue size, because the for-loop in execMain.c bails out in the following condition: if (numberTuples && numberTuples == current_tuple_count) break; Subsequently, the leader will end the plan and then wait in the Gather node for all workers to shutdown. However, workers still running but not producing any new rows will never reach the following condition in execMain.c to check if they're supposed to stop (the shared memory queue dest receiver will return false on detached queues): /* * If we are not able to send the tuple, we assume the destination * has closed and no more tuples can be sent. If that's the case, * end the loop. */ if (!dest->receiveSlot(slot, dest)) break; Reproduction steps for this problem are below. Here the worker getting the first table page will be done right away, but the query takes as long as it takes to scan all pages of the entire table. CREATE TABLE bar (col INT); INSERT INTO bar SELECT generate_series(1, 5000000); SET max_parallel_workers_per_gather = 8; EXPLAIN ANALYZE VERBOSE SELECT col FROM bar WHERE col = 1 LIMIT 1; Limit (cost=0.00..1.10 rows=1 width=4) (actual time=32.289..196.200 rows=1 loops=1) Output: col -> Gather (cost=0.00..30939.03 rows=28208 width=4) (actual time=32.278..196.176 rows=1 loops=1) Output: col Workers Planned: 8 Workers Launched: 7 -> Parallel Seq Scan on public.bar (cost=0.00..30939.03 rows=3526 width=4) (actual time=137.251..137.255 rows=0 loops=7) Output: col Filter: (bar.col = 1) Rows Removed by Filter: 713769 Worker 0: actual time=160.177..160.181 rows=0 loops=1 Worker 1: actual time=160.111..160.115 rows=0 loops=1 Worker 2: actual time=0.043..0.047 rows=1 loops=1 Worker 3: actual time=160.040..160.044 rows=0 loops=1 Worker 4: actual time=160.167..160.171 rows=0 loops=1 Worker 5: actual time=160.018..160.022 rows=0 loops=1 Worker 6: actual time=160.201..160.205 rows=0 loops=1 Planning Time: 0.087 ms Execution Time: 196.247 ms We would need something similar to CHECK_FOR_INTERRUPTS() which returns a NULL slot if a parallel worker is supposed to stop execution (we could e.g. check if the queue got detached). Or could we amend CHECK_FOR_INTERRUPTS() to just stop the worker gracefully if the queue got detached? Jasper Smit, Spiros Agathos and Dimos Stamatakis helped working on this. [1] https://www.postgresql.org/message-id/flat/CAFiTN-tVXqn_OG7tHNeSkBbN%2BiiCZTiQ83uakax43y1sQb2OBA%40m... -- David Geier (ServiceNow) ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Performance issues with parallelism and LIMIT 2023-02-01 13:41 Performance issues with parallelism and LIMIT David Geier <[email protected]> @ 2023-02-08 10:42 ` Tomas Vondra <[email protected]> 2023-02-20 18:18 ` Re: Performance issues with parallelism and LIMIT David Geier <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tomas Vondra @ 2023-02-08 10:42 UTC (permalink / raw) To: David Geier <[email protected]>; pgsql-hackers; +Cc: Robert Haas <[email protected]>; [email protected] On 2/1/23 14:41, David Geier wrote: > Hi hackers, > > While migrating from PostgreSQL 14 to 15, we encountered the following > performance degradation caused by commit 46846433a03dff: "shm_mq: Update > mq_bytes_written less often", discussion in [1]. > > The batching can make queries with a LIMIT clause run significantly > slower compared to PostgreSQL 14, because neither the ring buffer write > position is updated, nor the latch to inform the leader that there's > data available is set, before a worker's queue is 1/4th full. This can > be seen in the number of rows produced by a parallel worker. Worst-case, > the data set is large and all rows to answer the query appear early, but > are not big enough to fill the queue to 1/4th (e.g. when the LIMIT and > the tuple sizes are small). Here is an example to reproduce the problem. > Yeah, this is a pretty annoying regression. We already can hit poor behavior when matching rows are not distributed uniformly in the tables (which is what LIMIT costing assumes), and this makes it more likely to hit similar issues. A bit like when doing many HTTP requests makes it more likely to hit at least one 99% outlier. > ... > > I had a quick look at the code and I started wondering if we can't > achieve the same performance improvement without batching by e.g.: > > - Only set the latch if new data is written to an empty queue. > Otherwise, the leader should anyways keep try reading from the queues > without waiting for the latch, so no need to set the latch again. > > - Reorganize struct shm_mq. There seems to be false sharing happening > between at least mq_ring_size and the atomics and potentially also > between the atomics. I'm wondering if the that's not the root cause of > the "slow atomics" observed in [1]? I'm happy to do some profiling. > > Alternatively, we could always set the latch if numberTuples in > ExecutePlan() is reasonably low. To do so, the DestReceiver's receive() > method would only need an additional "force flush" argument. > No opinion on these options, but worth a try. Alternatively, we could try the usual doubling approach - start with a low threshold (and set the latch frequently), and then gradually increase it up to the 1/4. That should work both for queries expecting only few rows and those producing a lot of data. > > A slightly different but related problem is when some workers have > already produced enough rows to answer the LIMIT query, but other > workers are still running without producing any new rows. In that case > the "already done" workers will stop running even though they haven't > reached 1/4th of the queue size, because the for-loop in execMain.c > bails out in the following condition: > > if (numberTuples && numberTuples == current_tuple_count) > break; > > Subsequently, the leader will end the plan and then wait in the Gather > node for all workers to shutdown. However, workers still running but not > producing any new rows will never reach the following condition in > execMain.c to check if they're supposed to stop (the shared memory queue > dest receiver will return false on detached queues): > > /* > * If we are not able to send the tuple, we assume the > destination > * has closed and no more tuples can be sent. If that's the > case, > * end the loop. > */ > if (!dest->receiveSlot(slot, dest)) > break; > > Reproduction steps for this problem are below. Here the worker getting > the first table page will be done right away, but the query takes as > long as it takes to scan all pages of the entire table. > Ouch! > ... > > We would need something similar to CHECK_FOR_INTERRUPTS() which returns > a NULL slot if a parallel worker is supposed to stop execution (we could > e.g. check if the queue got detached). Or could we amend > CHECK_FOR_INTERRUPTS() to just stop the worker gracefully if the queue > got detached? > That sounds reasonable, but I'm not very familiar the leader-worker communication, so no opinion on how it should be done. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Performance issues with parallelism and LIMIT 2023-02-01 13:41 Performance issues with parallelism and LIMIT David Geier <[email protected]> 2023-02-08 10:42 ` Re: Performance issues with parallelism and LIMIT Tomas Vondra <[email protected]> @ 2023-02-20 18:18 ` David Geier <[email protected]> 2023-02-22 12:22 ` Re: Performance issues with parallelism and LIMIT Tomas Vondra <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David Geier @ 2023-02-20 18:18 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; pgsql-hackers; +Cc: Robert Haas <[email protected]>; [email protected] Hi, On 2/8/23 11:42, Tomas Vondra wrote: > On 2/1/23 14:41, David Geier wrote: > > Yeah, this is a pretty annoying regression. We already can hit poor > behavior when matching rows are not distributed uniformly in the tables > (which is what LIMIT costing assumes), and this makes it more likely to > hit similar issues. A bit like when doing many HTTP requests makes it > more likely to hit at least one 99% outlier. Are you talking about the use of ordering vs filtering indexes in queries where there's both an ORDER BY and a filter present (e.g. using an ordering index but then all rows passing the filter are at the end of the table)? If not, can you elaborate a bit more on that and maybe give an example. > No opinion on these options, but worth a try. Alternatively, we could > try the usual doubling approach - start with a low threshold (and set > the latch frequently), and then gradually increase it up to the 1/4. > > That should work both for queries expecting only few rows and those > producing a lot of data. I was thinking about this variant as well. One more alternative would be latching the leader once a worker has produced 1/Nth of the LIMIT where N is the number of workers. Both variants have the disadvantage that there are still corner cases where the latch is set too late; but it would for sure be much better than what we have today. I also did some profiling and - at least on my development laptop with 8 physical cores - the original example, motivating the batching change is slower than when it's disabled by commenting out: if (force_flush || mqh->mqh_send_pending > (mq->mq_ring_size >> 2)) SET parallel_tuple_cost TO 0; CREATE TABLE b (a int); INSERT INTO b SELECT generate_series(1, 200000000); ANALYZE b; EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM b; Gather (cost=1000.00..1200284.61 rows=200375424 width=4) (actual rows=200000000 loops=1) Workers Planned: 7 Workers Launched: 7 -> Parallel Seq Scan on b (cost=0.00..1199284.61 rows=28625061 width=4) (actual rows=25000000 loops=8) Always latch: 19055 ms Batching: 19575 ms If I find some time, I'll play around a bit more and maybe propose a patch. >> ... >> >> We would need something similar to CHECK_FOR_INTERRUPTS() which returns >> a NULL slot if a parallel worker is supposed to stop execution (we could >> e.g. check if the queue got detached). Or could we amend >> CHECK_FOR_INTERRUPTS() to just stop the worker gracefully if the queue >> got detached? >> > That sounds reasonable, but I'm not very familiar the leader-worker > communication, so no opinion on how it should be done. I think an extra macro that needs to be called from dozens of places to check if parallel execution is supposed to end is the least preferred approach. I'll read up more on how CHECK_FOR_INTERRUPTS() works and if we cannot actively signal the workers that they should stop. -- David Geier (ServiceNow) ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Performance issues with parallelism and LIMIT 2023-02-01 13:41 Performance issues with parallelism and LIMIT David Geier <[email protected]> 2023-02-08 10:42 ` Re: Performance issues with parallelism and LIMIT Tomas Vondra <[email protected]> 2023-02-20 18:18 ` Re: Performance issues with parallelism and LIMIT David Geier <[email protected]> @ 2023-02-22 12:22 ` Tomas Vondra <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tomas Vondra @ 2023-02-22 12:22 UTC (permalink / raw) To: David Geier <[email protected]>; pgsql-hackers; +Cc: Robert Haas <[email protected]>; [email protected] On 2/20/23 19:18, David Geier wrote: > Hi, > > On 2/8/23 11:42, Tomas Vondra wrote: >> On 2/1/23 14:41, David Geier wrote: >> >> Yeah, this is a pretty annoying regression. We already can hit poor >> behavior when matching rows are not distributed uniformly in the tables >> (which is what LIMIT costing assumes), and this makes it more likely to >> hit similar issues. A bit like when doing many HTTP requests makes it >> more likely to hit at least one 99% outlier. > Are you talking about the use of ordering vs filtering indexes in > queries where there's both an ORDER BY and a filter present (e.g. using > an ordering index but then all rows passing the filter are at the end of > the table)? If not, can you elaborate a bit more on that and maybe give > an example. Yeah, roughly. I don't think the explicit ORDER BY is a requirement for this to happen - it's enough when the part of the plan below LIMIT produces many rows, but the matching rows are at the end. >> No opinion on these options, but worth a try. Alternatively, we could >> try the usual doubling approach - start with a low threshold (and set >> the latch frequently), and then gradually increase it up to the 1/4. >> >> That should work both for queries expecting only few rows and those >> producing a lot of data. > > I was thinking about this variant as well. One more alternative would be > latching the leader once a worker has produced 1/Nth of the LIMIT where > N is the number of workers. Both variants have the disadvantage that > there are still corner cases where the latch is set too late; but it > would for sure be much better than what we have today. > > I also did some profiling and - at least on my development laptop with 8 > physical cores - the original example, motivating the batching change is > slower than when it's disabled by commenting out: > > if (force_flush || mqh->mqh_send_pending > (mq->mq_ring_size >> 2)) > > SET parallel_tuple_cost TO 0; > CREATE TABLE b (a int); > INSERT INTO b SELECT generate_series(1, 200000000); > ANALYZE b; > EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM b; > > Gather (cost=1000.00..1200284.61 rows=200375424 width=4) (actual > rows=200000000 loops=1) > Workers Planned: 7 > Workers Launched: 7 > -> Parallel Seq Scan on b (cost=0.00..1199284.61 rows=28625061 > width=4) (actual rows=25000000 loops=8) > > Always latch: 19055 ms > Batching: 19575 ms > > If I find some time, I'll play around a bit more and maybe propose a patch. > OK. Once you have a WIP patch maybe share it and I'll try to do some profiling too. >>> ... >>> >>> We would need something similar to CHECK_FOR_INTERRUPTS() which returns >>> a NULL slot if a parallel worker is supposed to stop execution (we could >>> e.g. check if the queue got detached). Or could we amend >>> CHECK_FOR_INTERRUPTS() to just stop the worker gracefully if the queue >>> got detached? >>> >> That sounds reasonable, but I'm not very familiar the leader-worker >> communication, so no opinion on how it should be done. > > I think an extra macro that needs to be called from dozens of places to > check if parallel execution is supposed to end is the least preferred > approach. I'll read up more on how CHECK_FOR_INTERRUPTS() works and if > we cannot actively signal the workers that they should stop. > IMHO if this requires adding another macro to a bunch of ad hoc places is rather inconvenient. It'd be much better to fix this in a localized manner (especially as it seems related to a fairly specific place). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-02-22 12:22 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-11 06:41 [PATCH v22 2/5] Deprecate ReindexStmt->concurrent.. Justin Pryzby <[email protected]> 2023-02-01 13:41 Performance issues with parallelism and LIMIT David Geier <[email protected]> 2023-02-08 10:42 ` Re: Performance issues with parallelism and LIMIT Tomas Vondra <[email protected]> 2023-02-20 18:18 ` Re: Performance issues with parallelism and LIMIT David Geier <[email protected]> 2023-02-22 12:22 ` Re: Performance issues with parallelism and LIMIT Tomas Vondra <[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