From: Yura Sokolov <y.sokolov@postgrespro.ru>
To: pgsql-hackers@lists.postgresql.org
Cc: David Rowley <dgrowleyml@gmail.com>
Subject: Fix BUG #17335: Duplicate result rows in Gather node
Date: Thu, 30 Dec 2021 14:14:32 +0300
Message-ID: <b59605fecb20ba9ea94e70ab60098c237c870628.camel@postgrespro.ru> (raw)
Good day, hackers.
Problem:
- Append path is created with explicitely parallel_aware = true
- It has two child, one is trivial, other is parallel_aware = false .
Trivial child is dropped.
- Gather/GatherMerge path takes Append path as a child and thinks
its child is parallel_aware = true.
- But Append path is removed at the last since it has only one child.
- Now Gather/GatherMerge thinks its child is parallel_aware, but it
is not.
Gather/GatherMerge runs its child twice: in a worker and in a leader,
and gathers same rows twice.
Reproduction code attached (repro.sql. Included as a test as well).
Suggested quick (and valid) fix in the patch attached:
- If Append has single child, then copy its parallel awareness.
Bug were introduced with commit 8edd0e79460b414b1d971895312e549e95e12e4f
"Suppress Append and MergeAppend plan nodes that have a single child."
During discussion, it were supposed [1] those fields should be copied:
> I haven't looked into whether this does the right things for parallel
> planning --- possibly create_[merge]append_path need to propagate up
> parallel-related path fields from the single child?
But it were not so obvious [2].
Better fix could contain removing Gather/GatherMerge node as well if
its child is not parallel aware.
Bug is reported in https://postgr.es/m/flat/17335-4dc92e1aea3a78af%40postgresql.org
Since no way to add thread from pgsql-bugs to commitfest, I write here.
[1] https://postgr.es/m/17500.1551669976%40sss.pgh.pa.us
[2] https://postgr.es/m/CAKJS1f_Wt_tL3S32R3wpU86zQjuHfbnZbFt0eqm%3DqcRFcdbLvw%40mail.gmail.com
----
regards
Yura Sokolov
y.sokolov@postgrespro.ru
funny.falcon@gmail.com
Attachments:v1-0001-Quick-fix-to-duplicate-result-rows-after-Append-p.patch (text/x-patch, 10.8K) download
From 47c6e161de4fc9d2d6eff45f427ebf49b4c9d11c Mon Sep 17 00:00:00 2001
From: Yura Sokolov <y.sokolov@postgrespro.ru>
Date: Mon, 20 Dec 2021 11:48:10 +0300
Subject: [PATCH v1] Quick fix to duplicate result rows after Append path
removal.
It could happen Append path is created with "parallel_aware" flag,
but its single child is not. Append path parent (Gather or Gather Merge)
thinks its child is parallel_aware, but after Append path removal Gather's
child become not parallel_aware. Then when Gather/Gather Merge decides
to run child in several workers or worker + leader participation, it
gathers duplicate result rows from several child path invocations.
With this fix Append path copies its single child parallel_aware / cost /
workers values.
Copied `num_workers == 0` triggers assert `num_workers > 0` in
cost_gather_merge function. But changing this assert to `num_workers >= 0`
doesn't lead to any runtime and/or logical error.
Fixes bug 17335
https://postgr.es/m/flat/17335-4dc92e1aea3a78af%40postgresql.org
---
src/backend/optimizer/path/costsize.c | 2 +-
src/backend/optimizer/util/pathnode.c | 3 +
.../expected/gather_removed_append.out | 131 ++++++++++++++++++
src/test/regress/parallel_schedule | 1 +
.../regress/sql/gather_removed_append.sql | 82 +++++++++++
5 files changed, 218 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/gather_removed_append.out
create mode 100644 src/test/regress/sql/gather_removed_append.sql
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.cindex 1e4d404f024..9949c3ab555 100644--- a/src/backend/optimizer/path/costsize.c+++ b/src/backend/optimizer/path/costsize.c@@ -440,7 +440,7 @@ cost_gather_merge(GatherMergePath *path, PlannerInfo *root,
* be overgenerous since the leader will do less work than other workers
* in typical cases, but we'll go with it for now.
*/
- Assert(path->num_workers > 0);+ Assert(path->num_workers >= 0);
N = (double) path->num_workers + 1;
logN = LOG2(N);
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.cindex af5e8df26b4..2ff4678937a 100644--- a/src/backend/optimizer/util/pathnode.c+++ b/src/backend/optimizer/util/pathnode.c@@ -1340,6 +1340,9 @@ create_append_path(PlannerInfo *root,
pathnode->path.startup_cost = child->startup_cost;
pathnode->path.total_cost = child->total_cost;
pathnode->path.pathkeys = child->pathkeys;
+ pathnode->path.parallel_aware = child->parallel_aware;+ pathnode->path.parallel_safe = child->parallel_safe;+ pathnode->path.parallel_workers = child->parallel_workers;
}
else
cost_append(pathnode);
diff --git a/src/test/regress/expected/gather_removed_append.out b/src/test/regress/expected/gather_removed_append.outnew file mode 100644index 00000000000..f6e861ce59d--- /dev/null+++ b/src/test/regress/expected/gather_removed_append.out@@ -0,0 +1,131 @@+-- Test correctness of parallel query execution after removal+-- of Append path due to single non-trivial child.+DROP TABLE IF EXISTS gather_append_1, gather_append_2;+NOTICE: table "gather_append_1" does not exist, skipping+NOTICE: table "gather_append_2" does not exist, skipping+CREATE TABLE gather_append_1 (+ fk int,+ f bool+);+INSERT INTO gather_append_1 (fk, f) SELECT i, i%50=0 from generate_series(1, 2000) as i;+CREATE INDEX gather_append_1_ix on gather_append_1 (f);+CREATE TABLE gather_append_2 (+ fk int,+ val serial+);+INSERT INTO gather_append_2 (fk) SELECT fk from gather_append_1, generate_series(1, 5) as i;+ANALYZE gather_append_1, gather_append_2;+SET max_parallel_workers_per_gather = 0;+-- Find correct rows count+SELECT count(1)+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk);+ count +-------+ 200+(1 row)++SET parallel_setup_cost = 0;+SET parallel_tuple_cost = 0.1;+SET min_parallel_table_scan_size = 0;+SET max_parallel_workers_per_gather = 2;+SELECT count(1)+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk);+ count +-------+ 200+(1 row)++EXPLAIN (ANALYZE true, COSTS false, TIMING false, SUMMARY false, VERBOSE false, BUFFERS false)+SELECT count(1)+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk);+ QUERY PLAN +---------------------------------------------------------------------------------------------------------+ Finalize Aggregate (actual rows=1 loops=1)+ -> Gather (actual rows=1 loops=1)+ Workers Planned: 1+ Workers Launched: 1+ Single Copy: true+ -> Partial Aggregate (actual rows=1 loops=1)+ -> Parallel Hash Left Join (actual rows=200 loops=1)+ Hash Cond: (gather_append_1.fk = gather_append_2.fk)+ -> Index Scan using gather_append_1_ix on gather_append_1 (actual rows=40 loops=1)+ Index Cond: (f = true)+ -> Parallel Hash (actual rows=10000 loops=1)+ Buckets: 16384 Batches: 1 Memory Usage: 544kB+ -> Parallel Seq Scan on gather_append_2 (actual rows=10000 loops=1)+(13 rows)++-- Result rows in root node should be equal to non-parallel count+EXPLAIN (ANALYZE true, COSTS false, TIMING false, SUMMARY false, VERBOSE false, BUFFERS false)+SELECT val+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk)+ORDER BY val;+ QUERY PLAN +---------------------------------------------------------------------------------------------------+ Gather Merge (actual rows=200 loops=1)+ Workers Planned: 0+ Workers Launched: 0+ -> Sort (actual rows=200 loops=1)+ Sort Key: gather_append_2.val+ Sort Method: quicksort Memory: 25kB+ -> Parallel Hash Left Join (actual rows=200 loops=1)+ Hash Cond: (gather_append_1.fk = gather_append_2.fk)+ -> Index Scan using gather_append_1_ix on gather_append_1 (actual rows=40 loops=1)+ Index Cond: (f = true)+ -> Parallel Hash (actual rows=10000 loops=1)+ Buckets: 16384 Batches: 1 Memory Usage: 519kB+ -> Parallel Seq Scan on gather_append_2 (actual rows=10000 loops=1)+(13 rows)++-- Result rows in root node should be equal to non-parallel count+EXPLAIN (ANALYZE true, COSTS false, TIMING false, SUMMARY false, VERBOSE false, BUFFERS false)+SELECT val+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk)+ORDER BY val;+ QUERY PLAN +---------------------------------------------------------------------------------------------------+ Gather Merge (actual rows=200 loops=1)+ Workers Planned: 0+ Workers Launched: 0+ -> Sort (actual rows=200 loops=1)+ Sort Key: gather_append_2.val+ Sort Method: quicksort Memory: 25kB+ -> Parallel Hash Left Join (actual rows=200 loops=1)+ Hash Cond: (gather_append_1.fk = gather_append_2.fk)+ -> Index Scan using gather_append_1_ix on gather_append_1 (actual rows=40 loops=1)+ Index Cond: (f = true)+ -> Parallel Hash (actual rows=10000 loops=1)+ Buckets: 16384 Batches: 1 Memory Usage: 519kB+ -> Parallel Seq Scan on gather_append_2 (actual rows=10000 loops=1)+(13 rows)+diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_scheduleindex 5b0c73d7e37..84f2f81255d 100644--- a/src/test/regress/parallel_schedule+++ b/src/test/regress/parallel_schedule@@ -100,6 +100,7 @@ test: rules psql psql_crosstab amutils stats_ext collate.linux.utf8
test: select_parallel
test: write_parallel
test: vacuum_parallel
+test: gather_removed_append
# no relation related tests can be put in this group
test: publication subscription
diff --git a/src/test/regress/sql/gather_removed_append.sql b/src/test/regress/sql/gather_removed_append.sqlnew file mode 100644index 00000000000..df1b796a4f6--- /dev/null+++ b/src/test/regress/sql/gather_removed_append.sql@@ -0,0 +1,82 @@+-- Test correctness of parallel query execution after removal+-- of Append path due to single non-trivial child.++DROP TABLE IF EXISTS gather_append_1, gather_append_2;++CREATE TABLE gather_append_1 (+ fk int,+ f bool+);++INSERT INTO gather_append_1 (fk, f) SELECT i, i%50=0 from generate_series(1, 2000) as i;++CREATE INDEX gather_append_1_ix on gather_append_1 (f);++CREATE TABLE gather_append_2 (+ fk int,+ val serial+);++INSERT INTO gather_append_2 (fk) SELECT fk from gather_append_1, generate_series(1, 5) as i;++ANALYZE gather_append_1, gather_append_2;++SET max_parallel_workers_per_gather = 0;++-- Find correct rows count+SELECT count(1)+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk);++SET parallel_setup_cost = 0;+SET parallel_tuple_cost = 0.1;+SET min_parallel_table_scan_size = 0;+SET max_parallel_workers_per_gather = 2;++SELECT count(1)+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk);++EXPLAIN (ANALYZE true, COSTS false, TIMING false, SUMMARY false, VERBOSE false, BUFFERS false)+SELECT count(1)+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk);++-- Result rows in root node should be equal to non-parallel count+EXPLAIN (ANALYZE true, COSTS false, TIMING false, SUMMARY false, VERBOSE false, BUFFERS false)+SELECT val+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk)+ORDER BY val;++-- Result rows in root node should be equal to non-parallel count+EXPLAIN (ANALYZE true, COSTS false, TIMING false, SUMMARY false, VERBOSE false, BUFFERS false)+SELECT val+FROM (+ SELECT fk FROM gather_append_1 WHERE f+ UNION ALL+ SELECT fk FROM gather_append_1 WHERE false+) as t+LEFT OUTER JOIN gather_append_2+USING (fk)+ORDER BY val;--
2.34.1
repro.sql (application/sql, 4.6K) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: y.sokolov@postgrespro.ru, pgsql-hackers@lists.postgresql.org, dgrowleyml@gmail.com
Subject: Re: Fix BUG #17335: Duplicate result rows in Gather node
In-Reply-To: <b59605fecb20ba9ea94e70ab60098c237c870628.camel@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox