On Fri, Sep 8, 2017 at 12:34 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Sep 5, 2017 at 7:01 AM, Ashutosh Bapat > <ashutosh.ba...@enterprisedb.com> wrote: >> accumulate_append_subpath() is executed for every path instead of >> every relation, so changing it would collect the same list multiple >> times. Instead, I found the old way of associating all intermediate >> partitions with the root partitioned relation work better. Here's the >> updated patch set. > > When I tried out patch 0001, it crashed repeatedly during 'make check' > because of an assertion failure in get_partitioned_child_rels.
Running "make check" on the whole patchset doesn't give that failure. So I didn't notice the crash since I was running regression on the whole patchset. Sorry for that. Fortunately git rebase -i allows us to execute shell commands while applying patches, so I have set it up to compile each patch and run regression. Hopefully that will catch such errors in future. That process showed me that patch 0003-In-add_paths_to_append_rel-get-partitioned_rels-for-.patch fixes that crash by calling get_partitioned_child_rels() only on the root partitioned table for which we have set up child_rels list. Amit Langote has a similar fix reported in his reply. So, we will discuss it there. > It > seemed to me that the way the patch was refactoring > expand_inherited_rtentry involved more code rearrangement than > necessary, so I reverted all the code rearrangement and just kept the > functional changes, and all the crashes went away. (That refactoring > also failed to initialize has_child properly.) In so doing, I > reintroduced the problem that the PartitionedChildRelInfo lists > weren't getting set up correctly, but after some thought I realized > that was just because expand_single_inheritance_child() was choosing > between adding an RTE and adding the OID to partitioned_child_rels, > whereas for an intermediate partitioned table it needs to do both. So > I inserted a trivial fix for that problem (replacing "else" with a new > "if"-test), basically: > > - else > + > + if (childrte->relkind == RELKIND_PARTITIONED_TABLE) > > Please check out the attached version of the patch. In addition to > the above simplifications, I did some adjustments to the comments in > various places - some just grammar and others a bit more substantive. > And I think I broke a long line in one place, too. > > One thing I notice is that if I rip out the changes to initsplan.c, > the new regression test still passes. If it's possible to write a > test that fails without those changes, I think it would be a good idea > to include one in the patch. That's certainly one of the subtler > parts of this patch, IMHO. Amit Langote has replied on these points as well. So, I will comment in a reply to his reply. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
