Re: BUG #11457: The below query crashes 9.3.5, but not 9.3.4

The following bug has been logged on the website:

Bug reference: 11457
Logged by: Nelson Page
Email address: npage(at)dynamicsignal(dot)com
PostgreSQL version: 9.3.5
Operating system: WIndows 8.1
Description:

SELECT

"GroupBy1"."A1" AS "C1"

FROM ( SELECT Count(1) AS "A1"

FROM (SELECT

"UnionAll4"."C1"

FROM (SELECT

"UnionAll3"."C1"

FROM (SELECT

"UnionAll2"."C1"

FROM (SELECT

"UnionAll1"."activityId" AS "C1"

FROM (SELECT

"Extent1"."activityId"

FROM "ActivityBlogCampaign" AS "Extent1"

UNION ALL

SELECT

"Extent2"."activityId"

FROM "ActivitySubmitArticle" AS "Extent2") AS "UnionAll1"

UNION ALL

SELECT

"Extent3"."activityId"

FROM "ActivityCuratedPost" AS "Extent3") AS "UnionAll2"

UNION ALL

SELECT

"Extent4"."activityId"

FROM "ActivitySurvey" AS "Extent4") AS "UnionAll3"

UNION ALL

SELECT

"Extent5"."activityId"

FROM "ActivityHashTag" AS "Extent5") AS "UnionAll4"

UNION ALL

SELECT

"Extent6"."activityId"

FROM "ActivityShareArticle" AS "Extent6") AS "UnionAll5"

INNER JOIN "Activity" AS "Extent7" ON "UnionAll5"."C1" =
"Extent7"."activityId"
GROUP BY "Extent7"."activityId"

) As "GroupBy1"

On Fri, Sep 19, 2014 at 7:23 PM, <npage(at)dynamicsignal(dot)com> wrote:
> SELECT
> [stuff]
> ) As "GroupBy1"
I tried to come up with a minimum schema close to what is used in your
example, aka 7 relations, 6 unioning on an ID column with an extra
inner join, and then tried the same query but I could not trigger the
crash on master, latest REL9_3_STABLE (9474c9d) or even 9.3.5.

Having only a query (on 7 relations btw!) and not a self-contained
test-case makes it harder to reproduce a crash. Could you provide at
least a minimum schema that triggers the crash so as we could
reproduce it more easily? I am attaching as well the self-contained
test case with your query and the minimal schema I could come up with
for the moment just for reference.
Regards,
--
Michael

Hi Michael,

I've attached the create scripts for those tables. I'm relatively new to postgresql, so if that's not as helpful as I think it is, let me know what else I can provide.

Thanks,
Nelson

-----Original Message-----
From: Michael Paquier [mailto:michael(dot)paquier(at)gmail(dot)com]
Sent: Saturday, September 20, 2014 10:44 AM
To: Nelson Page
Cc: pgsql-bugs(at)postgresql(dot)org
Subject: Re: [BUGS] BUG #11457: The below query crashes 9.3.5, but not 9.3.4

On Fri, Sep 19, 2014 at 7:23 PM, <npage(at)dynamicsignal(dot)com> wrote:
> SELECT
> [stuff]
> ) As "GroupBy1"
I tried to come up with a minimum schema close to what is used in your example, aka 7 relations, 6 unioning on an ID column with an extra inner join, and then tried the same query but I could not trigger the crash on master, latest REL9_3_STABLE (9474c9d) or even 9.3.5.

Having only a query (on 7 relations btw!) and not a self-contained test-case makes it harder to reproduce a crash. Could you provide at least a minimum schema that triggers the crash so as we could reproduce it more easily? I am attaching as well the self-contained test case with your query and the minimal schema I could come up with for the moment just for reference.
Regards,
--
Michael

Nelson Page wrote:
> Hi Michael,
>
> I've attached the create scripts for those tables. I'm relatively new
> to postgresql, so if that's not as helpful as I think it is, let me
> know what else I can provide.

FWIW I can reproduce the crash in 9.3 HEAD with these scripts, so that's
probably helpful enough. Table "Article" (not sent) is referenced in an
FK, but I just commented out that clause. Backtrace is:

#0 0x00007fd34d9ca1a5 in *__GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x00007fd34d9cd420 in *__GI_abort () at abort.c:92
#2 0x0000000000768c27 in ExceptionalCondition (conditionName=conditionName(at)entry=0x8c76b0 "!(!bms_overlap(appendrel->relids, required_outer))",
errorType=errorType(at)entry=0x7a080c "FailedAssertion",
fileName=fileName(at)entry=0x8c7470 "../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/util/relnode.c",
lineNumber=lineNumber(at)entry=953) at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/utils/error/assert.c:54
#3 0x000000000063c3a5 in get_appendrel_parampathinfo (appendrel=appendrel(at)entry=0x7fd3444db9c0, required_outer=required_outer(at)entry=0x7fd3444e40e8)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/util/relnode.c:953
#4 0x0000000000636293 in create_append_path (rel=rel(at)entry=0x7fd3444db9c0, subpaths=subpaths(at)entry=0x7fd3444f6bd8,
required_outer=required_outer(at)entry=0x7fd3444e40e8)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/util/pathnode.c:898
#5 0x0000000000605df0 in set_append_rel_pathlist (rti=1146046832, rel=0x7fd3444db9c0, root=0x7fd344506c80, rte=<optimized out>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:852
#6 set_rel_pathlist (root=root(at)entry=0x7fd344506c80, rel=0x7fd3444db9c0, rti=1146046832, rti(at)entry=1, rte=<optimized out>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:306
#7 0x0000000000606407 in set_base_rel_pathlists (root=<optimized out>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:208
#8 make_one_rel (root=root(at)entry=0x7fd344506c80, joinlist=joinlist(at)entry=0x7fd3444df710)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:138
#9 0x0000000000620d7d in query_planner (root=root(at)entry=0x7fd344506c80, tlist=tlist(at)entry=0x7fd3444db700, tuple_fraction=0,
tuple_fraction(at)entry=<error reading variable: Could not find type for DW_OP_GNU_const_type>, limit_tuples=-1,
qp_callback=qp_callback(at)entry=0x6218d0 <standard_qp_callback>, qp_extra=qp_extra(at)entry=0x7fff4c709470,
cheapest_path=cheapest_path(at)entry=0x7fff4c709460, sorted_path=sorted_path(at)entry=0x7fff4c709468, num_groups=num_groups(at)entry=0x7fff4c709450)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planmain.c:261
#10 0x0000000000622157 in grouping_planner (root=root(at)entry=0x7fd344506c80, tuple_fraction=0,
tuple_fraction(at)entry=<error reading variable: Could not find type for DW_OP_GNU_const_type>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planner.c:1214
#11 0x0000000000624acb in subquery_planner (glob=0x27e0bc0, parse=parse(at)entry=0x27efdf0, parent_root=parent_root(at)entry=0x27f1ab0,
hasRecursion=hasRecursion(at)entry=0 '\000', tuple_fraction=0, subroot=subroot(at)entry=0x7fff4c709638)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planner.c:558
#12 0x00000000006050ef in set_subquery_pathlist (rte=<optimized out>, rti=1, rel=<optimized out>, root=<optimized out>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:1231
#13 set_rel_size (root=root(at)entry=0x27f1ab0, rel=<optimized out>, rti=rti(at)entry=1, rte=<optimized out>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:264
#14 0x00000000006063a6 in set_base_rel_sizes (root=<optimized out>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:179
#15 make_one_rel (root=root(at)entry=0x27f1ab0, joinlist=joinlist(at)entry=0x27f25b0)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/path/allpaths.c:137
#16 0x0000000000620d7d in query_planner (root=root(at)entry=0x27f1ab0, tlist=tlist(at)entry=0x27f2498, tuple_fraction=0,
tuple_fraction(at)entry=<error reading variable: Could not find type for DW_OP_GNU_const_type>, limit_tuples=-1,
qp_callback=qp_callback(at)entry=0x6218d0 <standard_qp_callback>, qp_extra=qp_extra(at)entry=0x7fff4c709850,
cheapest_path=cheapest_path(at)entry=0x7fff4c709840, sorted_path=sorted_path(at)entry=0x7fff4c709848, num_groups=num_groups(at)entry=0x7fff4c709830)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planmain.c:261
#17 0x0000000000622157 in grouping_planner (root=root(at)entry=0x27f1ab0, tuple_fraction=0,
tuple_fraction(at)entry=<error reading variable: Could not find type for DW_OP_GNU_const_type>)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planner.c:1214
#18 0x0000000000624acb in subquery_planner (glob=glob(at)entry=0x27e0bc0, parse=parse(at)entry=0x271c428, parent_root=parent_root(at)entry=0x0,
hasRecursion=hasRecursion(at)entry=0 '\000', tuple_fraction=0, subroot=subroot(at)entry=0x7fff4c7099e8)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planner.c:558
#19 0x0000000000624df7 in standard_planner (parse=0x271c428, cursorOptions=0, boundParams=0x0)
at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/plan/planner.c:209
#20 0x0000000000696549 in pg_plan_query (querytree=<optimized out>, cursorOptions=cursorOptions(at)entry=0, boundParams=boundParams(at)entry=0x0)
at ../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/tcop/postgres.c:753

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> FWIW I can reproduce the crash in 9.3 HEAD with these scripts, so that's
> probably helpful enough. Table "Article" (not sent) is referenced in an
> FK, but I just commented out that clause. Backtrace is:

> #0 0x00007fd34d9ca1a5 in *__GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1 0x00007fd34d9cd420 in *__GI_abort () at abort.c:92
> #2 0x0000000000768c27 in ExceptionalCondition (conditionName=conditionName(at)entry=0x8c76b0 "!(!bms_overlap(appendrel->relids, required_outer))",
> errorType=errorType(at)entry=0x7a080c "FailedAssertion",
> fileName=fileName(at)entry=0x8c7470 "../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/optimizer/util/relnode.c",
> lineNumber=lineNumber(at)entry=953) at ../../../../../../../../../../pgsql/source/REL9_3_STABLE/src/backend/utils/error/assert.c:54

Hm, probably the same thing then as
http://www.postgresql.org/message-id/2326379.AOuSqtNClj@klinga.prans.org

I was poking at that one earlier this evening. The immediate fix is clear
enough (generate_implied_equalities_for_column should be considering
"grandparent" appendrels), but I'm not sure yet whether we need to change
the logic in generate_join_implied_equalities.

In the meantime, the OP could probably dodge the problem by not nesting
the UNION ALLs like that ...

regards, tom lane

On Tue, Sep 23, 2014 at 1:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hm, probably the same thing then as
> http://www.postgresql.org/message-id/2326379.AOuSqtNClj@klinga.prans.org
Yes, both cases are similar, with the use of two levels of relations
introducing.

> I was poking at that one earlier this evening. The immediate fix is clear
> enough (generate_implied_equalities_for_column should be considering
> "grandparent" appendrels), but I'm not sure yet whether we need to change
> the logic in generate_join_implied_equalities.

I could reproduce it as well with the scripts previously sent, and
this regression has been introduced by a2db7b7 on REL9_3_STABLE,
backpatched from a87c729 on master. Hence I imagine that this is
reproducible down to 9.1. Also, looking at the code the additions in
add_child_rel_equivalences to bypass the case where parent is not
mentioned in equivclass seems to be the root cause of the regression.

I have not come up yet with a patch fixing this issue though without
introducing regressions in other code paths... But I have been able at
least to come up with the attached minimalistic example to reproduce
the problem. Something similar to that could be part of a regression
test to add in the final patch.
Regards,
--
Michael

As mentioned in the original title, we cannot reproduce this crash in 9.3.4, but can in 9.3.5. So our particular crash does not go back to 9.1 (unless the 9.1 you mention is some other context maybe?)

Regards,
Nelson

-----Original Message-----
From: Michael Paquier [mailto:michael(dot)paquier(at)gmail(dot)com]
Sent: Friday, September 26, 2014 12:29 AM
To: Tom Lane
Cc: Alvaro Herrera; Nelson Page; pgsql-bugs(at)postgresql(dot)org
Subject: Re: [BUGS] BUG #11457: The below query crashes 9.3.5, but not 9.3.4

On Tue, Sep 23, 2014 at 1:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hm, probably the same thing then as
> http://www.postgresql.org/message-id/2326379.AOuSqtNClj@klinga.prans.o
> rg
Yes, both cases are similar, with the use of two levels of relations introducing.

> I was poking at that one earlier this evening. The immediate fix is
> clear enough (generate_implied_equalities_for_column should be
> considering "grandparent" appendrels), but I'm not sure yet whether we
> need to change the logic in generate_join_implied_equalities.

I could reproduce it as well with the scripts previously sent, and this regression has been introduced by a2db7b7 on REL9_3_STABLE, backpatched from a87c729 on master. Hence I imagine that this is reproducible down to 9.1. Also, looking at the code the additions in add_child_rel_equivalences to bypass the case where parent is not mentioned in equivclass seems to be the root cause of the regression.

I have not come up yet with a patch fixing this issue though without introducing regressions in other code paths... But I have been able at least to come up with the attached minimalistic example to reproduce the problem. Something similar to that could be part of a regression test to add in the final patch.
Regards,
--
Michael

Nelson Page wrote:
> As mentioned in the original title, we cannot reproduce this crash in 9.3.4, but can in 9.3.5. So our particular crash does not go back to 9.1 (unless the 9.1 you mention is some other context maybe?)

Bug fixes are backpatched to supported branches; 9.3.4 is older than
9.1.14. 9.1.14 does contain the backpatched bug fix which seems to have
introduced the new bug:

commit 555d0b2000e33fd1ad2721015996a66c43bbb3cd
Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Date: Thu Jun 26 10:41:10 2014 -0700

Back-patch "Fix EquivalenceClass processing for nested append relations".

When we committed a87c729153e372f3731689a7be007bc2b53f1410, we somehow
failed to notice that it didn't merely improve plan quality for expression
indexes; there were very closely related cases that failed outright with
"could not find pathkey item to sort". The failing cases seem to be those
where the planner was already capable of selecting a MergeAppend plan,
and there was inheritance involved: the lack of appropriate eclass child
members would prevent prepare_sort_from_pathkeys() from succeeding on the
MergeAppend's child plan nodes for inheritance child tables.

Accordingly, back-patch into 9.1 through 9.3, along with an extra
regression test case covering the problem.

Per trouble report from Michael Glaesemann.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Nelson Page wrote:
>> As mentioned in the original title, we cannot reproduce this crash in 9.3.4, but can in 9.3.5. So our particular crash does not go back to 9.1 (unless the 9.1 you mention is some other context maybe?)

> Bug fixes are backpatched to supported branches; 9.3.4 is older than
> 9.1.14. 9.1.14 does contain the backpatched bug fix which seems to have
> introduced the new bug:

The Assert failure mentioned in the other thread reproduces for me back to
9.2; that one is contingent on trying to build a parameterized path, so
of course it wouldn't occur before 9.2. But I imagine other variants of
this issue can be reproduced further back. The fundamental problem is
that we still have work to do to make nested appendrels behave properly.

I think basically the issue here is that in some situations we really want
to find the top-level ancestor appendrel, not just the immediate parent,
as it is the top-level ancestor that will be mentioned in join clauses,
ORDER BY, etc.

I came across a closely related issue too: in
generate_join_implied_equalities_broken we try to translate original join
clauses to apply to a particular child rel by applying the Var translation
from the AppendRelInfo for that child. I think this is wrong for the same
reasons: the original join clause will mention top-level ancestor Vars,
meaning we need to successively apply all the translations working down to
the descendant if we want a correct result.

We could probably hack solutions to these problems without changing the
AppendRelInfo data structures, but I'm wondering if it wouldn't be a good
idea to redefine them or at least add more fields to make it easier to
work with multi-level appendrel ancestry.

regards, tom lane

On Sat, Sep 27, 2014 at 1:57 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> We could probably hack solutions to these problems without changing the
> AppendRelInfo data structures, but I'm wondering if it wouldn't be a good
> idea to redefine them or at least add more fields to make it easier to
> work with multi-level appendrel ancestry.
>
Hm. Something like oldest_parent_reltype and oldest_parent_relid when
defining it? IMHO it would be nice to avoid simple hacks if possible but is
changing AppendRelInfo really something back-patchable at this point?
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Sat, Sep 27, 2014 at 1:57 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> We could probably hack solutions to these problems without changing the
>> AppendRelInfo data structures, but I'm wondering if it wouldn't be a good
>> idea to redefine them or at least add more fields to make it easier to
>> work with multi-level appendrel ancestry.

> Hm. Something like oldest_parent_reltype and oldest_parent_relid when
> defining it? IMHO it would be nice to avoid simple hacks if possible but is
> changing AppendRelInfo really something back-patchable at this point?

It seems unlikely that any extensions are creating these structs for
themselves. So I think we could safely add fields at the end in back
branches. We've done that before in other planner structs.

regards, tom lane

On Mon, Sep 29, 2014 at 11:06 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> > On Sat, Sep 27, 2014 at 1:57 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> We could probably hack solutions to these problems without changing the
> >> AppendRelInfo data structures, but I'm wondering if it wouldn't be a good
> >> idea to redefine them or at least add more fields to make it easier to
> >> work with multi-level appendrel ancestry.
>
> > Hm. Something like oldest_parent_reltype and oldest_parent_relid when
> > defining it? IMHO it would be nice to avoid simple hacks if possible but is
> > changing AppendRelInfo really something back-patchable at this point?
>
> It seems unlikely that any extensions are creating these structs for
> themselves. So I think we could safely add fields at the end in back
> branches. We've done that before in other planner structs.

OK. So I have been able to put my head into that and using the
multiple tips given out by Tom I have finished with the patch
attached. The fix done is rather simple: when creating a AppendRelInfo
entry for a join (pull_up_union_leaf_queries) or a union
(expand_inherited_rtentry), I added a new field in AppendRelInfo to
track the top-level parent, value that is afterwards used in
generate_implied_equalities_for_column when a child relation is
checked.

Patch contains as well a regression test with UNION ALL and a join.
Also, I imagine that the new functions I introduced would make more
sense in a different file, and that perhaps the new variables are not
correctly named, if you have any thoughts just let me know and I'll
update the fix. Regression tests are passing, but it may be possible
that some other code paths are broken, in this case I guess that we
could improve the fix by covering those other code paths as well with
additional regression tests. Also, I think that it would make sense to
track as well the composite type Oid of the top-level parent, I have
just not added it yet because it did not seem necessary for this
particular patch, just let me know your thoughts on the matter.

Note that this patch fixes as well the test case reported here:
http://www.postgresql.org/message-id/2326379.AOuSqtNClj@klinga.prans.org
Regards,
--
Michael

On Tue, Sep 30, 2014 at 5:04 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> OK. So I have been able to put my head into that and using the
> multiple tips given out by Tom I have finished with the patch
> attached.
Attached is an improved version of this patch with better names for
the new functions and variables. I added more comments at the same
time.
Regards,
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> Attached is an improved version of this patch with better names for
> the new functions and variables. I added more comments at the same
> time.

After studying this a bit more, I've gotten disillusioned with the
idea of precalculating the top_parent_relid for an AppendRelInfo.

1. It's too complicated to maintain. Your patch misses out doing
something when expand_inherited_rtentry() creates a child rel of
a rel that's already somebody else's child, and much harder to fix,
it misses out doing something when pull_up_simple_subquery() merges
a child query's append_rel_list with the parent query's. That
action could well result in some of the pulled-up child AppendRelInfos
now being children of pre-existing parent AppendRelInfos.
These things could probably be fixed, but:

2. There's no evidence that we actually have a performance problem we
need to fix in this area. Multilevel parentage is pretty rare, else
we'd have noticed problems here before. By the time we fix #1, we could
easily waste more cycles maintaining the data structure than we save.

3. Keeping the topmost parent relid only helps in some places anyhow.
In particular, in generate_join_implied_equalities_broken, we really
have to apply all the translation steps down from the top rel. (It
took me several hours of fooling around to generate a test case for
this ... but the attached patch includes a new regression test file
that exercises that code, and it shows a failure both with HEAD and
with your patch.)

4. Also, in generate_implied_equalities_for_column and
check_partial_indexes, it seems prudent to me to exclude all appendrel
parents of the child we're considering, not only the topmost. This is
probably moot at the moment but it might not be so forever, in particular
if we ever get around to fixing the problem that sub-SELECTs containing
WHERE clauses can't be pulled up as appendrels. (That'd result in WHERE
clauses associated with intermediate-level appendrels, and I think that
might lead us to generate bogus join paths in the same way as the current
problem does.)

Accordingly, I propose a patch more like the attached. This doesn't
try to change the data structures, but it does take the viewpoint that
all current callers of find_childrel_appendrelinfo() need to be fixed
to explicitly consider multiple levels of parent appendrels.

regards, tom lane

On Wed, Oct 1, 2014 at 2:43 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Accordingly, I propose a patch more like the attached. This doesn't
> try to change the data structures, but it does take the viewpoint that
> all current callers of find_childrel_appendrelinfo() need to be fixed
> to explicitly consider multiple levels of parent appendrels.
>
This approach is far more solid than my previous stuff and makes the
approach I took really brittle... Looking at your code, I am wondering if
it would not be safer to add some assertions in find_childrel_top_parent
and find_childrel_parents, something like a check on RELOPT_BASEREL for
rel->reloptkind before returning a result. Er, those new functions should
always finishing by scanning a base rel, right?

Btw, this code will need minor adjustments for REL9_2_STABLE and
REL9_3_STABLE. Is a backpatch down to 9.1 to be considered?
--
Michael

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> This approach is far more solid than my previous stuff and makes the
> approach I took really brittle...

Hey, the idea to add a field was mine, so don't beat yourself up about
it :-). Sometimes it takes working out an idea to realize it's bad.

> Looking at your code, I am wondering if
> it would not be safer to add some assertions in find_childrel_top_parent
> and find_childrel_parents, something like a check on RELOPT_BASEREL for
> rel->reloptkind before returning a result. Er, those new functions should
> always finishing by scanning a base rel, right?

Yeah, not a bad idea.

> Btw, this code will need minor adjustments for REL9_2_STABLE and
> REL9_3_STABLE. Is a backpatch down to 9.1 to be considered?

The bugs we're working on here are definitely demonstrable back to 9.2.
I'm not sure about 9.1 yet; but considering that a87c72915 had to get
back-patched as far as 9.1, there may well be some manifestations of
these problems visible in 9.1. (The new regression test that I wrote
doesn't seem to show any big problems there, but maybe I'm just short
a case or two.)

regards, tom lane