server crashed with TRAP: FailedAssertion("!(!parallel_aware || pathnode->path.parallel_safe)"
Hi,
I am getting a server crash for below test case.
postgres=# CREATE TABLE test( c1 int, c2 int, c3 text) partition by
range(c1);
CREATE TABLE
postgres=# create table test_p1 partition of test for values from
(minvalue) to (0);
CREATE TABLE
postgres=# create table test_p2 partition of test for values from (0) to
(maxvalue);
CREATE TABLE
postgres=#
postgres=# select (select max((select t1.c2 from test t1 where t1.c1 =
t2.c1))) from test t2;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>
--*logfile*
TRAP: FailedAssertion("!(!parallel_aware || pathnode->path.parallel_safe)",
File: "pathnode.c", Line: 1288)
2018-06-13 12:48:41.577 IST [52050] LOG: server process (PID 52061) was
terminated by signal 6: Aborted
2018-06-13 12:48:41.577 IST [52050] DETAIL: Failed process was running:
select (select max((select t1.c2 from test t1 where t1.c1 = t2.c1))) from
test t2;
*--core file *
Core was generated by `postgres: edb postgres [local]
SELECT '.
Program terminated with signal 6, Aborted.
#0 0x0000003dd2632495 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-23.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-57.el6.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0 0x0000003dd2632495 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x0000003dd2633c75 in abort () at abort.c:92
#2 0x0000000000a32782 in ExceptionalCondition (conditionName=0xc23718
"!(!parallel_aware || pathnode->path.parallel_safe)", errorType=0xc23411
"FailedAssertion",
fileName=0xc2357d "pathnode.c", lineNumber=1288) at assert.c:54
#3 0x00000000007f1a3d in create_append_path (root=0x27a5930,
rel=0x27c25f0, subpaths=0x27c4e60, partial_subpaths=0x0,
required_outer=0x0, parallel_workers=2,
parallel_aware=true, partitioned_rels=0x27c36f0, rows=-1) at
pathnode.c:1288
#4 0x0000000000797d40 in add_paths_to_append_rel (root=0x27a5930,
rel=0x27c25f0, live_childrels=0x27c4958) at allpaths.c:1700
#5 0x00000000007d3392 in apply_scanjoin_target_to_paths (root=0x27a5930,
rel=0x27c25f0, scanjoin_targets=0x27c4558,
scanjoin_targets_contain_srfs=0x0,
scanjoin_target_parallel_safe=false, tlist_same_exprs=true) at
planner.c:6962
#6 0x00000000007cb218 in grouping_planner (root=0x27a5930,
inheritance_update=false, tuple_fraction=0) at planner.c:2014
#7 0x00000000007c943a in subquery_planner (glob=0x27855e0,
parse=0x26c7250, parent_root=0x0, hasRecursion=false, tuple_fraction=0) at
planner.c:966
#8 0x00000000007c7ff7 in standard_planner (parse=0x26c7250,
cursorOptions=256, boundParams=0x0) at planner.c:405
#9 0x00000000007c7d1f in planner (parse=0x26c7250, cursorOptions=256,
boundParams=0x0) at planner.c:263
#10 0x00000000008c461e in pg_plan_query (querytree=0x26c7250,
cursorOptions=256, boundParams=0x0) at postgres.c:809
#11 0x00000000008c4751 in pg_plan_queries (querytrees=0x27a58f8,
cursorOptions=256, boundParams=0x0) at postgres.c:875
#12 0x00000000008c4a26 in exec_simple_query (query_string=0x26c5798 "select
(select max((select t1.c2 from test t1 where t1.c1 = t2.c1))) from test
t2;") at postgres.c:1050
#13 0x00000000008c8e67 in PostgresMain (argc=1, argv=0x26ef2a0,
dbname=0x26ef100 "postgres", username=0x26c2298 "edb") at postgres.c:4153
#14 0x0000000000826657 in BackendRun (port=0x26e7060) at postmaster.c:4361
#15 0x0000000000825dc5 in BackendStartup (port=0x26e7060) at
postmaster.c:4033
#16 0x00000000008221a7 in ServerLoop () at postmaster.c:1706
#17 0x0000000000821ad9 in PostmasterMain (argc=3, argv=0x26c01f0) at
postmaster.c:1379
#18 0x0000000000748cb8 in main (argc=3, argv=0x26c01f0) at main.c:228
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
I am getting a server crash for below test case.
postgres=# CREATE TABLE test( c1 int, c2 int, c3 text) partition by
range(c1);
CREATE TABLE
postgres=# create table test_p1 partition of test for values from
(minvalue) to (0);
CREATE TABLE
postgres=# create table test_p2 partition of test for values from (0) to
(maxvalue);
CREATE TABLE
postgres=#
postgres=# select (select max((select t1.c2 from test t1 where t1.c1 =
t2.c1))) from test t2;
server closed the connection unexpectedly
Reproduced here. The assert seems to be happening because
add_paths_to_append_rel is trying to create a parallel path for
an appendrel that is marked consider_parallel = false.
This appears to be the fault of commit ab7271677, whose authors I've cc'd:
the stanza starting at about allpaths.c:1672 is bullheadedly creating a
parallel path whether that's allowed or not. Fixing it might be as simple
as adding "rel->consider_parallel" to the conditions there.
regards, tom lane
I wrote:
This appears to be the fault of commit ab7271677, whose authors I've cc'd:
the stanza starting at about allpaths.c:1672 is bullheadedly creating a
parallel path whether that's allowed or not. Fixing it might be as simple
as adding "rel->consider_parallel" to the conditions there.
Oh, and while I'm bitching: the same commit has created this exceedingly
dangerous coding pattern in create_append_path:
pathnode->subpaths = list_concat(subpaths, partial_subpaths);
foreach(l, subpaths)
{
Path *subpath = (Path *) lfirst(l);
Because list_concat() modifies its first argument, this will usually
have the effect that the foreach traverses the partial_subpaths as
well as the main subpaths. But it's very unclear to the reader whether
that's intended or not. Worse, if we had *only* partial subpaths so
that subpaths was initially NIL, then the loop would fail to traverse
the partial subpaths, resulting in inconsistent behavior.
It looks to me like traversal of the partial subpaths is the right
thing here, in which case we should do
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)or perhaps better
- pathnode->subpaths = list_concat(subpaths, partial_subpaths);
+ pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);to make the behavior clear and consistent. But not being familiar
with the partial-subpaths morass, I'm not sure.
regards, tom lane
On Thu, Jun 14, 2018 at 9:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
I am getting a server crash for below test case.
postgres=# CREATE TABLE test( c1 int, c2 int, c3 text) partition by
range(c1);
CREATE TABLE
postgres=# create table test_p1 partition of test for values from
(minvalue) to (0);
CREATE TABLE
postgres=# create table test_p2 partition of test for values from (0) to
(maxvalue);
CREATE TABLE
postgres=#
postgres=# select (select max((select t1.c2 from test t1 where t1.c1 =
t2.c1))) from test t2;
server closed the connection unexpectedlyReproduced here. The assert seems to be happening because
add_paths_to_append_rel is trying to create a parallel path for
an appendrel that is marked consider_parallel = false.This appears to be the fault of commit ab7271677, whose authors I've cc'd:
the stanza starting at about allpaths.c:1672 is bullheadedly creating a
parallel path whether that's allowed or not. Fixing it might be as simple
as adding "rel->consider_parallel" to the conditions there.
Yeah, or perhaps disallow creation of any partial paths at the first
place like in attached. This will save us some work as well.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_pa_path_generation_v1.patchapplication/octet-stream; name=fix_pa_path_generation_v1.patchDownload
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 146298828d8..3ada379f8bc 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1383,7 +1383,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
List *pa_partial_subpaths = NIL;
List *pa_nonpartial_subpaths = NIL;
bool partial_subpaths_valid = true;
- bool pa_subpaths_valid = enable_parallel_append;
+ bool pa_subpaths_valid;
List *all_child_pathkeys = NIL;
List *all_child_outers = NIL;
ListCell *l;
@@ -1391,6 +1391,9 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
bool build_partitioned_rels = false;
double partial_rows = -1;
+ /* If appropriate, consider parallel append */
+ pa_subpaths_valid = enable_parallel_append && rel->consider_parallel;
+
/*
* AppendPath generated for partitioned tables must record the RT indexes
* of partitioned tables that are direct or indirect children of this
On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
This appears to be the fault of commit ab7271677, whose authors I've cc'd:
the stanza starting at about allpaths.c:1672 is bullheadedly creating a
parallel path whether that's allowed or not. Fixing it might be as simple
as adding "rel->consider_parallel" to the conditions there.Oh, and while I'm bitching: the same commit has created this exceedingly
dangerous coding pattern in create_append_path:pathnode->subpaths = list_concat(subpaths, partial_subpaths);
foreach(l, subpaths)
{
Path *subpath = (Path *) lfirst(l);Because list_concat() modifies its first argument, this will usually
have the effect that the foreach traverses the partial_subpaths as
well as the main subpaths. But it's very unclear to the reader whether
that's intended or not. Worse, if we had *only* partial subpaths so
that subpaths was initially NIL, then the loop would fail to traverse
the partial subpaths, resulting in inconsistent behavior.It looks to me like traversal of the partial subpaths is the right
thing here, in which case we should do- foreach(l, subpaths) + foreach(l, pathnode->subpaths)or perhaps better
- pathnode->subpaths = list_concat(subpaths, partial_subpaths); + pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);to make the behavior clear and consistent.
I agree with your analysis and proposed change. However, I think in
practice, it might not lead to any bug as in the loop, we are
computing parallel_safety and partial_subpaths should be
parallel_safe.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Jun 16, 2018 at 10:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jun 14, 2018 at 9:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> writes:
I am getting a server crash for below test case.
postgres=# select (select max((select t1.c2 from test t1 where t1.c1 =
t2.c1))) from test t2;
server closed the connection unexpectedlyReproduced here. The assert seems to be happening because
add_paths_to_append_rel is trying to create a parallel path for
an appendrel that is marked consider_parallel = false.This appears to be the fault of commit ab7271677, whose authors I've cc'd:
the stanza starting at about allpaths.c:1672 is bullheadedly creating a
parallel path whether that's allowed or not. Fixing it might be as simple
as adding "rel->consider_parallel" to the conditions there.Yeah, or perhaps disallow creation of any partial paths at the first
place like in attached. This will save us some work as well.
Attached patch contains test case as well. I have tried to come up
with some simple test, but couldn't come up with anything much simpler
than reported by Rajkumar, so decided to use the test case provided by
him.
Added to Open Items list.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_pa_path_generation_v2.patchapplication/octet-stream; name=fix_pa_path_generation_v2.patchDownload
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 1462988..3ada379 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1383,7 +1383,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
List *pa_partial_subpaths = NIL;
List *pa_nonpartial_subpaths = NIL;
bool partial_subpaths_valid = true;
- bool pa_subpaths_valid = enable_parallel_append;
+ bool pa_subpaths_valid;
List *all_child_pathkeys = NIL;
List *all_child_outers = NIL;
ListCell *l;
@@ -1391,6 +1391,9 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
bool build_partitioned_rels = false;
double partial_rows = -1;
+ /* If appropriate, consider parallel append */
+ pa_subpaths_valid = enable_parallel_append && rel->consider_parallel;
+
/*
* AppendPath generated for partitioned tables must record the RT indexes
* of partitioned tables that are direct or indirect children of this
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index a07cd50..2c49de6 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -132,6 +132,32 @@ select sp_test_func() order by 1;
foo
(2 rows)
+-- Parallel Append is not be used when the subpath depends on the outer param
+create table part_pa_test(a int, b int) partition by range(a);
+create table part_pa_test_p1 partition of part_pa_test for values from (minvalue) to (0);
+create table part_pa_test_p2 partition of part_pa_test for values from (0) to (maxvalue);
+explain (costs off)
+ select (select max((select pa1.b from part_pa_test pa1 where pa1.a = pa2.a)))
+ from part_pa_test pa2;
+ QUERY PLAN
+--------------------------------------------------------------
+ Aggregate
+ -> Gather
+ Workers Planned: 3
+ -> Parallel Append
+ -> Parallel Seq Scan on part_pa_test_p1 pa2
+ -> Parallel Seq Scan on part_pa_test_p2 pa2_1
+ SubPlan 2
+ -> Result
+ SubPlan 1
+ -> Append
+ -> Seq Scan on part_pa_test_p1 pa1
+ Filter: (a = pa2.a)
+ -> Seq Scan on part_pa_test_p2 pa1_1
+ Filter: (a = pa2.a)
+(14 rows)
+
+drop table part_pa_test;
-- test with leader participation disabled
set parallel_leader_participation = off;
explain (costs off)
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 7db75b0..9a08a8e 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -55,6 +55,15 @@ $$ select 'foo'::varchar union all select 'bar'::varchar $$
language sql stable;
select sp_test_func() order by 1;
+-- Parallel Append is not be used when the subpath depends on the outer param
+create table part_pa_test(a int, b int) partition by range(a);
+create table part_pa_test_p1 partition of part_pa_test for values from (minvalue) to (0);
+create table part_pa_test_p2 partition of part_pa_test for values from (0) to (maxvalue);
+explain (costs off)
+ select (select max((select pa1.b from part_pa_test pa1 where pa1.a = pa2.a)))
+ from part_pa_test pa2;
+drop table part_pa_test;
+
-- test with leader participation disabled
set parallel_leader_participation = off;
explain (costs off)
On 16 June 2018 at 19:30, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Jun 16, 2018 at 10:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Yeah, or perhaps disallow creation of any partial paths at the first
place like in attached. This will save us some work as well.Attached patch contains test case as well. I have tried to come up
with some simple test, but couldn't come up with anything much simpler
than reported by Rajkumar, so decided to use the test case provided by
him.
Thanks for the patch. I had a look at it, and it looks good to me. One
minor comment :
+-- Parallel Append is not be used when the subpath depends on the outer param
"is not be used" => "is not to be used"
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
On 16 June 2018 at 10:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
This appears to be the fault of commit ab7271677, whose authors I've cc'd:
the stanza starting at about allpaths.c:1672 is bullheadedly creating a
parallel path whether that's allowed or not. Fixing it might be as simple
as adding "rel->consider_parallel" to the conditions there.Oh, and while I'm bitching: the same commit has created this exceedingly
dangerous coding pattern in create_append_path:pathnode->subpaths = list_concat(subpaths, partial_subpaths);
foreach(l, subpaths)
{
Path *subpath = (Path *) lfirst(l);Because list_concat() modifies its first argument, this will usually
have the effect that the foreach traverses the partial_subpaths as
well as the main subpaths. But it's very unclear to the reader whether
that's intended or not. Worse, if we had *only* partial subpaths so
that subpaths was initially NIL, then the loop would fail to traverse
the partial subpaths, resulting in inconsistent behavior.It looks to me like traversal of the partial subpaths is the right
thing here, in which case we should do- foreach(l, subpaths) + foreach(l, pathnode->subpaths)or perhaps better
- pathnode->subpaths = list_concat(subpaths, partial_subpaths); + pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);to make the behavior clear and consistent.
I agree with your analysis and proposed change. However, I think in
practice, it might not lead to any bug as in the loop, we are
computing parallel_safety and partial_subpaths should be
parallel_safe.
Will have a look at this soon.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
On Mon, Jun 18, 2018 at 3:09 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
On 16 June 2018 at 19:30, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Jun 16, 2018 at 10:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Yeah, or perhaps disallow creation of any partial paths at the first
place like in attached. This will save us some work as well.Attached patch contains test case as well. I have tried to come up
with some simple test, but couldn't come up with anything much simpler
than reported by Rajkumar, so decided to use the test case provided by
him.Thanks for the patch. I had a look at it, and it looks good to me. One
minor comment :+-- Parallel Append is not be used when the subpath depends on the outer param
"is not be used" => "is not to be used"
Fixed in the attached patch. I will wait for a day or two to see if
Tom or Robert wants to say something and then commit.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
fix_pa_path_generation_v3.patchapplication/octet-stream; name=fix_pa_path_generation_v3.patchDownload
From 2315795a3a29bf42182b10f1442b07bfef29f162 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Tue, 19 Jun 2018 08:56:49 +0530
Subject: [PATCH] Don't consider parallel append for parallel unsafe paths.
Commit ab72716778 overlooked to enforce the check that the parallel
unsafe paths should not be considered for generating parallel append
paths. This commit fixes that check.
Initial analysis by Tom Lane.
Reported-by: Rajkumar Raghuwanshi
Author: Amit Kapila and Rajkumar Raghuwanshi
Reviewed-by: Amit Khandekar
Discussion: https://postgr.es/m/CAKcux6=tPJ6nJ08r__nU_pmLQiC0xY15Fn0HvG1Cprsjdd9s_Q@mail.gmail.com
---
src/backend/optimizer/path/allpaths.c | 5 ++++-
src/test/regress/expected/select_parallel.out | 26 ++++++++++++++++++++++++++
src/test/regress/sql/select_parallel.sql | 9 +++++++++
3 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 1462988..3ada379 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1383,7 +1383,7 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
List *pa_partial_subpaths = NIL;
List *pa_nonpartial_subpaths = NIL;
bool partial_subpaths_valid = true;
- bool pa_subpaths_valid = enable_parallel_append;
+ bool pa_subpaths_valid;
List *all_child_pathkeys = NIL;
List *all_child_outers = NIL;
ListCell *l;
@@ -1391,6 +1391,9 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
bool build_partitioned_rels = false;
double partial_rows = -1;
+ /* If appropriate, consider parallel append */
+ pa_subpaths_valid = enable_parallel_append && rel->consider_parallel;
+
/*
* AppendPath generated for partitioned tables must record the RT indexes
* of partitioned tables that are direct or indirect children of this
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index a07cd50..e48e394 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -132,6 +132,32 @@ select sp_test_func() order by 1;
foo
(2 rows)
+-- Parallel Append is not to be used when the subpath depends on the outer param
+create table part_pa_test(a int, b int) partition by range(a);
+create table part_pa_test_p1 partition of part_pa_test for values from (minvalue) to (0);
+create table part_pa_test_p2 partition of part_pa_test for values from (0) to (maxvalue);
+explain (costs off)
+ select (select max((select pa1.b from part_pa_test pa1 where pa1.a = pa2.a)))
+ from part_pa_test pa2;
+ QUERY PLAN
+--------------------------------------------------------------
+ Aggregate
+ -> Gather
+ Workers Planned: 3
+ -> Parallel Append
+ -> Parallel Seq Scan on part_pa_test_p1 pa2
+ -> Parallel Seq Scan on part_pa_test_p2 pa2_1
+ SubPlan 2
+ -> Result
+ SubPlan 1
+ -> Append
+ -> Seq Scan on part_pa_test_p1 pa1
+ Filter: (a = pa2.a)
+ -> Seq Scan on part_pa_test_p2 pa1_1
+ Filter: (a = pa2.a)
+(14 rows)
+
+drop table part_pa_test;
-- test with leader participation disabled
set parallel_leader_participation = off;
explain (costs off)
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 7db75b0..31045d7 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -55,6 +55,15 @@ $$ select 'foo'::varchar union all select 'bar'::varchar $$
language sql stable;
select sp_test_func() order by 1;
+-- Parallel Append is not to be used when the subpath depends on the outer param
+create table part_pa_test(a int, b int) partition by range(a);
+create table part_pa_test_p1 partition of part_pa_test for values from (minvalue) to (0);
+create table part_pa_test_p2 partition of part_pa_test for values from (0) to (maxvalue);
+explain (costs off)
+ select (select max((select pa1.b from part_pa_test pa1 where pa1.a = pa2.a)))
+ from part_pa_test pa2;
+drop table part_pa_test;
+
-- test with leader participation disabled
set parallel_leader_participation = off;
explain (costs off)
--
1.8.3.1
On Mon, Jun 18, 2018 at 11:36 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Fixed in the attached patch. I will wait for a day or two to see if
Tom or Robert wants to say something and then commit.
The patch LGTM but the commit message could perhaps use a little
word-smithing, e.g. "Commit ab72716778 allowed Parallel Append paths
to be generated for a relation that is not parallel safe. Prevent
that from happening."
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Jun 19, 2018 at 7:45 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jun 18, 2018 at 11:36 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
Fixed in the attached patch. I will wait for a day or two to see if
Tom or Robert wants to say something and then commit.The patch LGTM but the commit message could perhaps use a little
word-smithing, e.g. "Commit ab72716778 allowed Parallel Append paths
to be generated for a relation that is not parallel safe. Prevent
that from happening."
Changed as per your suggestion and pushed!
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
On 16 June 2018 at 10:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It looks to me like traversal of the partial subpaths is the right
thing here, in which case we should do- foreach(l, subpaths) + foreach(l, pathnode->subpaths)or perhaps better
- pathnode->subpaths = list_concat(subpaths, partial_subpaths); + pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);to make the behavior clear and consistent.
I agree with your analysis and proposed change. However, I think in
practice, it might not lead to any bug as in the loop, we are
computing parallel_safety and partial_subpaths should be
parallel_safe.Will have a look at this soon.
Did you get a chance to look at it? I have committed the patch which
fixes the problem reported in this thread, so I am inclined to close
the corresponding entry in Open Items list, but I am afraid that we
will lose track of this suggestion if I close it.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On 20 June 2018 at 14:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
On 16 June 2018 at 10:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It looks to me like traversal of the partial subpaths is the right
thing here, in which case we should do- foreach(l, subpaths) + foreach(l, pathnode->subpaths)or perhaps better
- pathnode->subpaths = list_concat(subpaths, partial_subpaths); + pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);to make the behavior clear and consistent.
I agree with your analysis and proposed change. However, I think in
practice, it might not lead to any bug as in the loop, we are
computing parallel_safety and partial_subpaths should be
parallel_safe.Will have a look at this soon.
Did you get a chance to look at it?
Not yet, but I have planned to do this by tomorrow.
I have committed the patch which
fixes the problem reported in this thread, so I am inclined to close
the corresponding entry in Open Items list, but I am afraid that we
will lose track of this suggestion if I close it.
Yes I agree.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
On 20 June 2018 at 14:28, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
On 20 June 2018 at 14:24, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
On 16 June 2018 at 10:44, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It looks to me like traversal of the partial subpaths is the right
thing here, in which case we should do- foreach(l, subpaths) + foreach(l, pathnode->subpaths)or perhaps better
- pathnode->subpaths = list_concat(subpaths, partial_subpaths); + pathnode->subpaths = subpaths = list_concat(subpaths, partial_subpaths);to make the behavior clear and consistent.
I agree with your analysis and proposed change. However, I think in
practice, it might not lead to any bug as in the loop, we are
computing parallel_safety and partial_subpaths should be
parallel_safe.Will have a look at this soon.
Did you get a chance to look at it?
Not yet, but I have planned to do this by tomorrow.
After list_concat, the subpaths no longer has only non-partial paths,
which it is supposed to have. So it anyways should not be used in the
subsequent code in that function. So I think the following change
should be good.
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)
Attached patch contains the above change.
You are right that the partial paths do not need to be used for
evaluating the pathnode->path.parallel_safe field. But since we also
have the parameterization-related assertion in the same loop, I think
it is ok to do both the things in one loop, covering all paths.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
Attachments:
fix_subpaths_issue.patchapplication/octet-stream; name=fix_subpaths_issue.patchDownload
commit 395fd75a6e9845bca9f32af51a4dbfe666aba5ff
Author: Amit Khandekar <amit.khandekar@enterprisedb.com>
Date: Thu Jun 21 11:45:19 2018 +0530
Correctly use the return value of list_concat
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index e190ad4..dbf9adc 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1274,7 +1274,7 @@ create_append_path(PlannerInfo *root,
pathnode->first_partial_path = list_length(subpaths);
pathnode->subpaths = list_concat(subpaths, partial_subpaths);
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)
{
Path *subpath = (Path *) lfirst(l);
On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
On 20 June 2018 at 14:28, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
Did you get a chance to look at it?
Not yet, but I have planned to do this by tomorrow.
After list_concat, the subpaths no longer has only non-partial paths,
which it is supposed to have. So it anyways should not be used in the
subsequent code in that function. So I think the following change
should be good.
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)Attached patch contains the above change.
Thanks, Tom, would you like to go-ahead and commit this change as this
is suggested by you or would you like me to take care of this or do
you want to wait for Robert's input?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
After list_concat, the subpaths no longer has only non-partial paths,
which it is supposed to have. So it anyways should not be used in the
subsequent code in that function. So I think the following change
should be good.
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)
Patch LGTM.
Thanks, Tom, would you like to go-ahead and commit this change as this
is suggested by you or would you like me to take care of this or do
you want to wait for Robert's input?
Please push --- I'm busy getting ready to leave for vacation ...
regards, tom lane
Thanks for commit. I have verified reported case. it is fixed now.
Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
On Thu, Jun 21, 2018 at 7:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Amit Kapila <amit.kapila16@gmail.com> writes:
On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <amitdkhan.pg@gmail.com>
wrote:
After list_concat, the subpaths no longer has only non-partial paths,
which it is supposed to have. So it anyways should not be used in the
subsequent code in that function. So I think the following change
should be good.
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)Patch LGTM.
Thanks, Tom, would you like to go-ahead and commit this change as this
is suggested by you or would you like me to take care of this or do
you want to wait for Robert's input?Please push --- I'm busy getting ready to leave for vacation ...
regards, tom lane
On Thu, Jun 21, 2018 at 7:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
After list_concat, the subpaths no longer has only non-partial paths,
which it is supposed to have. So it anyways should not be used in the
subsequent code in that function. So I think the following change
should be good.
- foreach(l, subpaths)
+ foreach(l, pathnode->subpaths)Patch LGTM.
Okay, pushed and updated Open Items page as well.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com