Bogus lateral-reference-propagation logic in create_lateral_join_info
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2019-02-05T21:56:52Z
Lists: pgsql-hackers
Attachments
- fix-lateral-relid-propagation.patch (text/x-diff) patch
While poking at bug #15613 (in which FDWs are failing to mark created Paths with correct outer-reference sets), I thought it'd be a good idea to add some asserts to Path creation saying that every Path should be parameterized by at least whatever the relation's required LATERAL references are. I did that as per the added assertions in relnode.c below. I didn't expect any failures in the existing regression tests, since we know those don't exercise bug #15613, but darn if the addition to get_appendrel_parampathinfo didn't blow up in the core tests. A bit of excavation later, it turns out that this is a bug since day 1 in create_lateral_join_info. It needs to propagate lateral_relids and related fields from appendrel parents to children, but it was not, in the original code, accounting for the possibility of grandchildren. Those need to get the lateral fields propagated from their topmost ancestor too, but they weren't. This leads to having an intermediate appendrel that is marked with some lateral_relids when its children are not, causing add_paths_to_append_rel to believe that unparameterized paths can be built, triggering the new assertion. I'm not sure if there are any worse consequences; the regression test case that's triggering the Assert seems to work otherwise, so we might be accidentally failing to fail. But it's not supposed to be like that. Commit 0a480502b hacked this code up to deal with grandchildren for the case of partitioned tables, but the regression test that's falling over involves nested UNION ALL subqueries, which are not that. Rather than add another RTE-kind exception, though, I think we ought to rewrite it completely and get rid of the nested loops in favor of one traversal of the append_rel_list. This does require assuming that the append_rel_list has ancestor entries before descendant entries, but that's okay because of the way the list is built. (I note that 0a480502b is effectively assuming that ancestors appear before children in the RTE list, which is no safer an assumption.) Also, I'd really like to know why I had to put in the exception seen in the loop for AppendRelInfos that do not point to a valid parent. It seems to me that that is almost certainly working around a bug in the partitioning logic. (Without it, the partition_prune regression test crashes.) Or would somebody like to own up to having created that state of affairs intentionally? If so why? Anyway, I propose to commit and back-patch the initsplan.c part of the attached. The added asserts in relnode.c should probably not go in until we have a bug #15613 fix that will prevent postgres_fdw from triggering them, so I'll do that later. regards, tom lane
Commits
-
Propagate lateral-reference information to indirect descendant relations.
- b33d5e076eec 9.4.21 landed
- 27293553f4c0 9.5.16 landed
- 41740b9efb11 9.6.12 landed
- a6ea72779a98 10.7 landed
- 45ae2031ec15 11.2 landed
- bdd9a99aac3b 12.0 landed
-
Expand partitioned table RTEs level by level, without flattening.
- 0a480502b092 11.0 cited