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 <[hidden email]> 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 |
In reply to this post by Tom Lane-2
On Thu, Jun 14, 2018 at 9:54 PM, Tom Lane <[hidden email]> wrote:
> Rajkumar Raghuwanshi <[hidden email]> 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. > place like in attached. This will save us some work as well. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com |
In reply to this post by Tom Lane-2
On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <[hidden email]> 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 |
In reply to this post by akapila
On Sat, Jun 16, 2018 at 10:44 AM, Amit Kapila <[hidden email]> wrote:
> On Thu, Jun 14, 2018 at 9:54 PM, Tom Lane <[hidden email]> wrote: >> Rajkumar Raghuwanshi <[hidden email]> 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 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. >> > > 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 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 |
On 16 June 2018 at 19:30, Amit Kapila <[hidden email]> wrote:
> On Sat, Jun 16, 2018 at 10:44 AM, Amit Kapila <[hidden email]> 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 |
In reply to this post by akapila
On 16 June 2018 at 10:44, Amit Kapila <[hidden email]> wrote:
> On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <[hidden email]> 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 |
In reply to this post by Amit Khandekar-2
On Mon, Jun 18, 2018 at 3:09 PM, Amit Khandekar <[hidden email]> wrote:
> On 16 June 2018 at 19:30, Amit Kapila <[hidden email]> wrote: >> On Sat, Jun 16, 2018 at 10:44 AM, Amit Kapila <[hidden email]> 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" > Tom or Robert wants to say something and then commit. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com |
On Mon, Jun 18, 2018 at 11:36 PM, Amit Kapila <[hidden email]> 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 <[hidden email]> wrote:
> On Mon, Jun 18, 2018 at 11:36 PM, Amit Kapila <[hidden email]> 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 |
In reply to this post by Amit Khandekar-2
On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <[hidden email]> wrote:
> On 16 June 2018 at 10:44, Amit Kapila <[hidden email]> wrote: >> On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <[hidden email]> 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 <[hidden email]> wrote:
> On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <[hidden email]> wrote: >> On 16 June 2018 at 10:44, Amit Kapila <[hidden email]> wrote: >>> On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <[hidden email]> 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 <[hidden email]> wrote:
> On 20 June 2018 at 14:24, Amit Kapila <[hidden email]> wrote: >> On Mon, Jun 18, 2018 at 3:11 PM, Amit Khandekar <[hidden email]> wrote: >>> On 16 June 2018 at 10:44, Amit Kapila <[hidden email]> wrote: >>>> On Thu, Jun 14, 2018 at 10:05 PM, Tom Lane <[hidden email]> 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 |
On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <[hidden email]> wrote:
> On 20 June 2018 at 14:28, Amit Khandekar <[hidden email]> 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 <[hidden email]> writes:
> On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <[hidden email]> 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 <[hidden email]> wrote: Amit Kapila <[hidden email]> writes: |
In reply to this post by Tom Lane-2
On Thu, Jun 21, 2018 at 7:21 PM, Tom Lane <[hidden email]> wrote:
> Amit Kapila <[hidden email]> writes: >> On Thu, Jun 21, 2018 at 11:51 AM, Amit Khandekar <[hidden email]> 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 |
Free forum by Nabble | Edit this page |