Check if subpath can do motion.#219
Conversation
'cdbpath_create_motion_path' function doesn't check subpath's params Relids. When Path contains index scan with quals it leads to segfault. Fix adds a few additional checks of can motion node be generated atop of subpath. Additional regression test included. Related to issue #12228. Partial backport of 9cc1da6 (#10012).
leskin-in
left a comment
leskin-in
left a comment
There was a problem hiding this comment.
Please run pgindent on the code changes.
leskin-in
left a comment
leskin-in
left a comment
There was a problem hiding this comment.
Run pgindent on the modified code from src/backend/cdb/cdbpath.c.
Changed TODO comment to |
@InnerLife0, as we discuss yesterday, this statement is incorrect in common. We pass
and current PR implementation breaks down that. The function must never return NULL if We may use It seems that we may apply motion to subpath with references to another part of join if pathkeys are provided in the current 6X implementaion. @maksm90, what do you think? |
src/backend/cdb/cdbpath.c
Outdated
There was a problem hiding this comment.
@maksm90 I was confused about the type of subpath varaible. It's the argument passed by reference. So in case of condition is true and there is Materialize on top of subpath, we'll remove it anyway after patch.
|
New version of PR is available. Please, re-review. |
|
@InnerLife0 please rebase this PR on top of |
…e. Adding missed blank lines. Mentioning broken regression test. (cherry picked from commit 3237523)
InnerLife0
force-pushed
the
ADBDEV-1715-2
branch
from
55e4bac to
cf9616b
Compare
|
Rebased. Commit ebfb060 left as is, as it was already cherry-picked by me to the new PR on the pivotal side. Another commits squashed to one solid solution, which includes removing of our old solution and can be cherry-picked to the mentioned PR. |
'if' conditions backported from 9cc1da6 partially duplicates the logic over require_existing_order flag. Instead of applying additional logic, we can move require_existing_order flag analysis higher, before SingleQE and SegmentGeneral, in the place where it really make sense. Adding additional comments and warning message, so we can find what going wrong even on release version. Adding missed Insist's to cdbpath_create_motion_path callers.
InnerLife0
force-pushed
the
ADBDEV-1715-2
branch
from
cf9616b to
66832ea
Compare
Fix ORCA panic on nonexistent hash opfamilies. Originally commit 5e04eb1 introduced opfamilies handling and tracking in CDistributionSpecHashed class. Its code contains PopulateDefaultOpfamilies() function, which fills opfamily array in cases when it's not passed in ctor. The initial idea translated by the class was that m_opfamilies must be always initialized in case of set EopttraceConsiderOpfamiliesForDistribution flag. However, some specific operations like "parallel union all" are able to work with data types, which do not have corresponding legacy opclass in GG 6. For example, these types are arrays (see get_legacy_cdbhash_opclass_for_base_type() in cdblegacyhash.c), json, etc. And in order to support them in union all cases commit 47a72cc allowed storing null m_opfamilies by removing an assert in PopulateDefaultOpfamilies(). That allowed orca to create CDistributionSpecHashedNoOp for union all without opfamilies. It worked ok since CDistributionSpecHashedNoOp has it's own matching rules and do not use opfamilies. Nevertheless 47a72cc broke the initial concept of not empty opfamilies in CDistributionSpecHashed and that made some severe errors possible to occur. In example provided in tests, ORCA faced segfault on accessing the null pointer (like in CDistributionSpecHashed::FMatchSubset()). This patch prevents invalid initialization of CDistributionSpecHashed by adding an exception raise into PopulateDefaultOpfamilies() to preserve class opfamily invariant. Since we still need specific parallel union all case to be planned, the ctors of CDistributionSpecHashed are modified to support CDistributionSpecHashedNoOp case.
5 participants
UPD: After long debates we decided to do our own fix, which will not be a backport from master branch, because backported commit contains some duplicate logic.
cdbpath_create_motion_pathfunction checks subpath's params Relids, but do it not in place for Bottleneck locus type. When Path contains index scan with quals it leads to segfault.This PR moves require_existing_order flag analysis higher, before SingleQE and SegmentGeneral, in the place where it really makes sense.
Additionally it adds missed Insist's in the places where we can get NULL value accidentally, and warning message, so we can find what going wrong even on release version.
Additional regression test included.
The info below is obsolete.
cdbpath_create_motion_pathfunction doesn't check subpath's params Relids. When Path contains index scan with quals it leads to segfault.Fix adds a few additional checks of can motion node be generated atop of subpath. Additional regression test included.
Related to issue #12228. Partial backport of 9cc1da6 (#10012).
Fix is partial, because we don’t need to first bug, mentioned in commit message. We just don’t have some new functions to fix them (
add_rowid_to_path). More, regression tests, provided in fix, works well despite of fact orca provides another seems valid plan.There are functions which ignores NULL value returned by
cdbpath_create_motion_path.cdbpath_create_motion_pathcan already return NULL in several cases, but PR increases the probability of such event. As returning NULL fromcdbpath_create_motion_pathis not something new and the original commit doesn't contain any additional fixes, we decided to not add NULL value analysis tocdbpath_create_motion_pathcallers, but rather think about PR and mention it publicly at least.