From 14e9b2a752efaa427ce1b400b9aaa5a636898a04 Mon Sep 17 00:00:00 2001 From: Amit Kapila Date: Tue, 4 Sep 2018 10:06:09 +0530 Subject: [PATCH] Prohibit pushing subqueries containing window function calculation to workers. Allowing window function calculation in workers leads to inconsistent results because if the input row ordering is not fully deterministic, the output of window functions might vary across workers. The fix is to treat them as parallel-restricted. In the passing, improve the coding pattern in max_parallel_hazard_walker so that it has a chain of mutually-exclusive if ... else if ... else if ... else if ... IsA tests. Reported-by: Marko Tiikkaja Bug: 15324 Author: Amit Kapila Reviewed-by: Tom Lane Backpatch-through: 9.6 Discussion: https://postgr.es/m/CAL9smLAnfPJCDUUG4ckX2iznj53V7VSMsYefzZieN93YxTNOcw@mail.gmail.com --- src/backend/optimizer/util/clauses.c | 16 +++++++++++- src/test/regress/expected/select_parallel.out | 26 +++++++++++++++++++ src/test/regress/sql/select_parallel.sql | 6 +++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index a04ad6e99e..ee6f4cdf4d 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -1190,12 +1190,26 @@ max_parallel_hazard_walker(Node *node, max_parallel_hazard_context *context) return true; } - if (IsA(node, NextValueExpr)) + else if (IsA(node, NextValueExpr)) { if (max_parallel_hazard_test(PROPARALLEL_UNSAFE, context)) return true; } + /* + * Treat window functions as parallel-restricted because we aren't sure + * whether the input row ordering is fully deterministic, and the output + * of window functions might vary across workers if not. (In some cases, + * like where the window frame orders by a primary key, we could relax + * this restriction. But it doesn't currently seem worth expending extra + * effort to do so.) + */ + else if (IsA(node, WindowFunc)) + { + if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context)) + return true; + } + /* * As a notational convenience for callers, look through RestrictInfo. */ diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out index f1b8cd4337..13cdd3d5cc 100644 --- a/src/test/regress/expected/select_parallel.out +++ b/src/test/regress/expected/select_parallel.out @@ -958,6 +958,32 @@ select count(*) from tenk1; reset force_parallel_mode; reset role; +-- Window function calculation can't be pushed to workers. +explain (costs off, verbose) + select count(*) from tenk1 a where (unique1, two) in + (select unique1, row_number() over() from tenk1 b); + QUERY PLAN +---------------------------------------------------------------------------------------------- + Aggregate + Output: count(*) + -> Hash Semi Join + Hash Cond: ((a.unique1 = b.unique1) AND (a.two = (row_number() OVER (?)))) + -> Gather + Output: a.unique1, a.two + Workers Planned: 4 + -> Parallel Seq Scan on public.tenk1 a + Output: a.unique1, a.two + -> Hash + Output: b.unique1, (row_number() OVER (?)) + -> WindowAgg + Output: b.unique1, row_number() OVER (?) + -> Gather + Output: b.unique1 + Workers Planned: 4 + -> Parallel Index Only Scan using tenk1_unique1 on public.tenk1 b + Output: b.unique1 +(18 rows) + -- to increase the parallel query test coverage SAVEPOINT settings; SET LOCAL force_parallel_mode = 1; diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql index 7771e055ab..3d1dffe61d 100644 --- a/src/test/regress/sql/select_parallel.sql +++ b/src/test/regress/sql/select_parallel.sql @@ -362,6 +362,12 @@ select count(*) from tenk1; reset force_parallel_mode; reset role; +-- Window function calculation can't be pushed to workers. +explain (costs off, verbose) + select count(*) from tenk1 a where (unique1, two) in + (select unique1, row_number() over() from tenk1 b); + + -- to increase the parallel query test coverage SAVEPOINT settings; SET LOCAL force_parallel_mode = 1; -- 2.40.0