You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudberry.apache.org by "Alena0704 (via GitHub)" <gi...@apache.org> on 2026/05/06 07:38:11 UTC
[PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Alena0704 opened a new pull request, #1719:
URL: https://github.com/apache/cloudberry/pull/1719
### What does this PR do?
Fixes a query hang in ORCA when a CTE over a replicated table is referenced
from a different executor slice than its Producer (e.g. from a scalar
subquery, or via duplicate-hazard motion in a JOIN).
Before Expr→DXL translation, the patch walks the physical tree and tracks
which slice each CTE Producer and Consumer lives on (slices are delimited
by Motion nodes). If any Consumer is on a different slice than its Producer
*and* the Producer's distribution is replicated-like
(StrictReplicated / TaintedReplicated / Universal), ORCA raises an
unsupported-feature exception and falls back to the Postgres planner.
The replicated filter is essential: ordinary cross-slice CTE plans
(non-replicated Producer with Gather/Redistribute Consumer) are a normal
ORCA pattern and must not trigger fallback.
Inspired by greengage 51fe92e, but extends it: 51fe92e's single-point check
doesn't trigger when a CTE over a replicated table is referenced from a
scalar subquery, so the query hangs. This PR replaces it with a whole-tree
walker that catches both cases.
### Type of Change
- [x] Bug fix (non-breaking change)
- [ ] New feature (non-breaking change)
- [ ] Breaking change (fix or feature with breaking changes)
- [ ] Documentation update
### Breaking Changes
None.
### Test Plan
- [x] Unit tests added/updated
- [x] Integration tests added/updated
- [ ] Passed `make installcheck`
- [ ] Passed `make -C src/test installcheck-cbdb-parallel`
Two regression cases added:
- `qp_orca_fallback`: original 51fe92e reproducer — JOIN over a CTE
referencing replicated tables with duplicate-hazard motion.
- `shared_scan`: scalar-subquery reproducer where a CTE over a replicated
table is referenced from multiple `(SELECT … FROM cte)` expressions.
Guarded by `statement_timeout = '15s'` so the test fails fast if the
hang regresses (40k rows in the driving table are needed to push ORCA
to the cross-slice plan — fewer rows would make the test silently pass
without the fix).
### Impact
**Performance:** Negligible. The added walker runs once per query, only
over the physical tree, and only when CTE Producers/Consumers are present.
The set of queries that newly fall back to the Postgres planner is narrow
(replicated CTE Producer with a cross-slice Consumer) — these queries
previously hung, so any plan is an improvement.
**User-facing changes:** Queries matching the pattern above now complete
instead of hanging. Users will see
`INFO: GPORCA failed to produce a plan, falling back to planner` /
`DETAIL: Feature not supported: CTE Consumer placed on a different slice
than its replicated Producer` in those cases.
**Dependencies:** None.
### Checklist
- [x] Followed [contribution guide](https://cloudberry.apache.org/contribute/code)
- [ ] Added/updated documentation
- [x] Reviewed code for security implications
- [ ] Requested review from cloudberry committers
### Additional Context
Cherry-picked from open-gpdb/gpdb#375 (commit
`3a9aebfe3afb27dd060c16b5cfa7932e7a2ed43f`).
Files changed:
- `src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h`
- `src/backend/gporca/libgpopt/src/base/CUtils.cpp`
- `src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp`
- `src/test/regress/{sql,expected}/qp_orca_fallback{,.out,_optimizer.out}`
- `src/test/regress/{sql,expected}/shared_scan{,.out,_optimizer.out}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206538031
##########
src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp:
##########
@@ -4250,6 +4264,10 @@ CTranslatorExprToDXL::BuildScalarSubplans(
{
const ULONG size = pdrgpcrInner->Size();
+ // Fallback to Postgres optimizer if the SubPlan's inner expression contains a
+ // CTE Consumer whose Producer lives outside this subtree. Such a Consumer
+ // would become a cross-slice Shared Scan reader without a local Producer,
+ // which can hang the query or fail at execution time.
Review Comment:
fixed but i rewrote the comment on my way - now it matches with condition when we try to detect all Consumers that placed on a different slice than its replicated Producer. it looks like this now:
```
// Fallback to Postgres optimizer if the SubPlan's inner expression contains a
// CTE Consumer placed on a different slice than its replicated Producer.
// Such a Consumer becomes a cross-slice Shared Scan reader without a local
// Producer, which can hang the query or fail at execution time.
if (CUtils::FHasCrossSliceReplicatedCTEConsumer(m_mp, pexprInner))
{
GPOS_RAISE(
gpopt::ExmaDXL, gpopt::ExmiExpr2DXLUnsupportedFeature,
GPOS_WSZ_LIT(
"CTE Consumer placed on a different slice than its replicated Producer"));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "yjhjstz (via GitHub)" <gi...@apache.org>.
yjhjstz commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3204525613
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+ CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
Review Comment:
remove `FHasUnpairedCTEConsumer` dead code ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "zhangwenchao-123 (via GitHub)" <gi...@apache.org>.
zhangwenchao-123 commented on PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#issuecomment-4411934827
LGTM to fallback temporary, but maybe the best way to fix the root cause is fixing the Sequence::pdsRequired, in my opinion, this path is a illegal path which should be pruned by pdsRequired restrition. What's more, maybe we can add restriction between CCTEReq and CCTEMap by checking the motion worker number, only when they have the same motion workers, CCTEMap::FSatisfies will return true and we will get a legal path. Prefer to the first path, it's more formal than second which looks like a remedial plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206577638
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild, cteConsumers,
+ cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id), sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ // Producer's distribution comes from its only child -- inspect
+ // it there. Skip non-replicated Producers; they cannot trigger
+ // the cross-slice issue we are checking for.
+ GPOS_ASSERT(1 == pexpr->Arity());
+ CExpression *pexprChild = (*pexpr)[0];
+ CDrvdPropPlan *pdpplan =
+ CDrvdPropPlan::Pdpplan(pexprChild->PdpDerive());
+
+ if (FReplicatedLikeDistribution(pdpplan->Pds()->Edt()))
+ {
+ prodInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+ }
+ else if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ // Consumer is a leaf -- record (cteId, curSlice) and let the
+ // caller decide later, once the whole tree has been walked.
+ consInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+
+ BOOL isMotion = CUtils::FPhysicalMotion(pop);
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (pexprChild->Pop()->FScalar())
+ {
+ continue;
+ }
+
+ ULONG childSlice = curSlice;
+ if (isMotion)
+ {
+ (*pNextSlice)++;
+ childSlice = *pNextSlice;
+ }
+
+ CollectCTESlices(mp, pexprChild, childSlice, pNextSlice, prodInfos,
+ consInfos);
+ }
+}
+
+BOOL
+CUtils::FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ if (NULL == pexpr)
+ {
+ return false;
+ }
+
+ CTEInfoArray *prodInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ CTEInfoArray *consInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ ULONG nextSlice = 0;
+
+ CollectCTESlices(mp, pexpr, 0 /*curSlice*/, &nextSlice, prodInfos,
+ consInfos);
+
+ BOOL cross = false;
+
+ for (ULONG ic = 0; ic < consInfos->Size(); ic++)
+ {
+ SCTEInfo *cons = (*consInfos)[ic];
+
+ for (ULONG ip = 0; ip < prodInfos->Size(); ip++)
+ {
+ SCTEInfo *prod = (*prodInfos)[ip];
+ if (prod->cteId == cons->cteId && prod->sliceId != cons->sliceId)
+ {
+ cross = true;
+ goto lExit;
Review Comment:
Agree. As I understood it might be dangerous since we can jump over object construction or skip destructors. I replaced it with the a small function that uses a plain return true for the early exit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206583270
##########
src/test/regress/sql/shared_scan.sql:
##########
@@ -120,3 +120,28 @@ where
(data_hour = date_trunc('day',data_hour) and stat.schema_name || '.' ||stat.table_name not in (select table_nm_23 from tbls_daily_report_23))
and (stat.schema_name || '.' ||stat.table_name not in (select table_nm_onl_act from tbls_w_onl_actl_data))
or (stat.schema_name || '.' ||stat.table_name in (select table_nm_onl_act from tbls_w_onl_actl_data));
+
+-- ORCA should fallback when a CTE over a replicated table is referenced
+-- from multiple scalar subqueries.
+-- ss_t1 needs enough rows (40000) to push ORCA to the cross-slice plan;
+-- with fewer rows the bug does not manifest and the test would silently
+-- pass even without the fix.
+CREATE TABLE ss_t1 AS
+ SELECT generate_series(1, 40000) id
+ DISTRIBUTED BY (id);
+CREATE TABLE ss_t2 AS
+ SELECT * FROM (VALUES (1, 10), (2, 20)) AS v(id, v)
+ DISTRIBUTED REPLICATED;
Review Comment:
Added it with ignore. see the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206580303
##########
src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp:
##########
@@ -265,6 +265,20 @@ CTranslatorExprToDXL::PdxlnTranslate(CExpression *pexpr,
GPOS_ASSERT(nullptr == m_pdpplan);
+ // Walk the physical tree and detect a CTE Consumer placed on a
+ // different slice than its Producer when the Producer's output is
+ // replicated-like (StrictReplicated/TaintedReplicated/Universal).
+ // Fall back to the Postgres optimizer if it is detected because
+ // it breaks Producer-Consumer locality and can hang the
+ // query at execution.
+ if (CUtils::FHasCrossSliceReplicatedCTEConsumer(m_mp, pexpr))
+ {
+ GPOS_RAISE(
+ gpdxl::ExmaDXL, gpdxl::ExmiExpr2DXLUnsupportedFeature,
Review Comment:
fixed in last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3193847884
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
Review Comment:
These new helpers are recursive but don’t call GPOS_CHECK_STACK_SIZE (unlike nearby recursive walkers such as FHasCTEAnchor). Consider adding GPOS_CHECK_STACK_SIZE at the top of CollectConsumersAndProducers() (and any other recursive helpers added here) to avoid stack overflow on deep expression trees.
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild, cteConsumers,
+ cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id), sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
Review Comment:
CollectCTESlices() is also recursive and currently lacks a GPOS_CHECK_STACK_SIZE guard. Adding it at the top of the function would align with other deep-tree walkers in this file and reduce the risk of stack overflow on deeply nested plans.
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+ CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
Review Comment:
Naming is inconsistent with surrounding boolean helpers (e.g., FHasCTEAnchor, FHasSubqueryOrApply): hasUnpairedCTEConsumer() starts with a lowercase 'h' and lacks the usual 'F' prefix. Consider renaming to match existing conventions to keep the public CUtils API consistent.
##########
src/test/regress/sql/shared_scan.sql:
##########
@@ -120,3 +120,28 @@ where
(data_hour = date_trunc('day',data_hour) and stat.schema_name || '.' ||stat.table_name not in (select table_nm_23 from tbls_daily_report_23))
and (stat.schema_name || '.' ||stat.table_name not in (select table_nm_onl_act from tbls_w_onl_actl_data))
or (stat.schema_name || '.' ||stat.table_name in (select table_nm_onl_act from tbls_w_onl_actl_data));
+
+-- ORCA should fallback when a CTE over a replicated table is referenced
+-- from multiple scalar subqueries.
+-- ss_t1 needs enough rows (40000) to push ORCA to the cross-slice plan;
+-- with fewer rows the bug does not manifest and the test would silently
+-- pass even without the fix.
+CREATE TABLE ss_t1 AS
+ SELECT generate_series(1, 40000) id
+ DISTRIBUTED BY (id);
+CREATE TABLE ss_t2 AS
+ SELECT * FROM (VALUES (1, 10), (2, 20)) AS v(id, v)
+ DISTRIBUTED REPLICATED;
Review Comment:
The test creates ss_t1/ss_t2 without first dropping them. To make the script more robust to partial runs or reruns (e.g., after a failure), consider adding a guarded `DROP TABLE IF EXISTS ss_t1, ss_t2;` before the CREATE TABLE statements (similar to earlier patterns in this file).
##########
src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp:
##########
@@ -4250,6 +4264,10 @@ CTranslatorExprToDXL::BuildScalarSubplans(
{
const ULONG size = pdrgpcrInner->Size();
+ // Fallback to Postgres optimizer if the SubPlan's inner expression contains a
+ // CTE Consumer whose Producer lives outside this subtree. Such a Consumer
+ // would become a cross-slice Shared Scan reader without a local Producer,
+ // which can hang the query or fail at execution time.
Review Comment:
This comment says the function will fall back to the Postgres optimizer when the SubPlan’s inner expression contains an unpaired CTE Consumer, but there is no corresponding check/raise in the code below. Either implement the described fallback (e.g., by detecting the condition before translating) or adjust/remove the comment to avoid misleading future maintainers.
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+ CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
+
+ static BOOL FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp,
+ CExpression *pexpr);
Review Comment:
CollectConsumersAndProducers() / hasUnpairedCTEConsumer() are added to the public CUtils API but appear unused in the codebase (no call sites found). If they’re not needed, consider removing them or making them file-local to avoid expanding the public surface area; if they are intended to be used, wiring them into the relevant translation path would make that intent clear.
##########
src/backend/gporca/libgpopt/src/translate/CTranslatorExprToDXL.cpp:
##########
@@ -265,6 +265,20 @@ CTranslatorExprToDXL::PdxlnTranslate(CExpression *pexpr,
GPOS_ASSERT(nullptr == m_pdpplan);
+ // Walk the physical tree and detect a CTE Consumer placed on a
+ // different slice than its Producer when the Producer's output is
+ // replicated-like (StrictReplicated/TaintedReplicated/Universal).
+ // Fall back to the Postgres optimizer if it is detected because
+ // it breaks Producer-Consumer locality and can hang the
+ // query at execution.
+ if (CUtils::FHasCrossSliceReplicatedCTEConsumer(m_mp, pexpr))
+ {
+ GPOS_RAISE(
+ gpdxl::ExmaDXL, gpdxl::ExmiExpr2DXLUnsupportedFeature,
Review Comment:
This file generally qualifies DXL fallback exceptions via the gpopt namespace (e.g., gpopt::ExmaDXL / gpopt::ExmiExpr2DXLUnsupportedFeature elsewhere in this file). For consistency, consider using the same qualification here as well (or unqualified, since gpdxl is brought into gpopt in the header).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206577638
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild, cteConsumers,
+ cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id), sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ // Producer's distribution comes from its only child -- inspect
+ // it there. Skip non-replicated Producers; they cannot trigger
+ // the cross-slice issue we are checking for.
+ GPOS_ASSERT(1 == pexpr->Arity());
+ CExpression *pexprChild = (*pexpr)[0];
+ CDrvdPropPlan *pdpplan =
+ CDrvdPropPlan::Pdpplan(pexprChild->PdpDerive());
+
+ if (FReplicatedLikeDistribution(pdpplan->Pds()->Edt()))
+ {
+ prodInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+ }
+ else if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ // Consumer is a leaf -- record (cteId, curSlice) and let the
+ // caller decide later, once the whole tree has been walked.
+ consInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+
+ BOOL isMotion = CUtils::FPhysicalMotion(pop);
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (pexprChild->Pop()->FScalar())
+ {
+ continue;
+ }
+
+ ULONG childSlice = curSlice;
+ if (isMotion)
+ {
+ (*pNextSlice)++;
+ childSlice = *pNextSlice;
+ }
+
+ CollectCTESlices(mp, pexprChild, childSlice, pNextSlice, prodInfos,
+ consInfos);
+ }
+}
+
+BOOL
+CUtils::FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ if (NULL == pexpr)
+ {
+ return false;
+ }
+
+ CTEInfoArray *prodInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ CTEInfoArray *consInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ ULONG nextSlice = 0;
+
+ CollectCTESlices(mp, pexpr, 0 /*curSlice*/, &nextSlice, prodInfos,
+ consInfos);
+
+ BOOL cross = false;
+
+ for (ULONG ic = 0; ic < consInfos->Size(); ic++)
+ {
+ SCTEInfo *cons = (*consInfos)[ic];
+
+ for (ULONG ip = 0; ip < prodInfos->Size(); ip++)
+ {
+ SCTEInfo *prod = (*prodInfos)[ip];
+ if (prod->cteId == cons->cteId && prod->sliceId != cons->sliceId)
+ {
+ cross = true;
+ goto lExit;
Review Comment:
Agree. As I understood it might be dangerous since we can jump over object construction or skip destructors. I replaced it with the a small function that uses a plain return true for the early exit (see last commit).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "leborchuk (via GitHub)" <gi...@apache.org>.
leborchuk merged PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206512765
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
Review Comment:
fixed. i added it in the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "yjhjstz (via GitHub)" <gi...@apache.org>.
yjhjstz commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3204279318
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild, cteConsumers,
+ cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id), sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ // Producer's distribution comes from its only child -- inspect
+ // it there. Skip non-replicated Producers; they cannot trigger
+ // the cross-slice issue we are checking for.
+ GPOS_ASSERT(1 == pexpr->Arity());
+ CExpression *pexprChild = (*pexpr)[0];
+ CDrvdPropPlan *pdpplan =
+ CDrvdPropPlan::Pdpplan(pexprChild->PdpDerive());
+
+ if (FReplicatedLikeDistribution(pdpplan->Pds()->Edt()))
+ {
+ prodInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+ }
+ else if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ // Consumer is a leaf -- record (cteId, curSlice) and let the
+ // caller decide later, once the whole tree has been walked.
+ consInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+
+ BOOL isMotion = CUtils::FPhysicalMotion(pop);
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (pexprChild->Pop()->FScalar())
+ {
+ continue;
+ }
+
+ ULONG childSlice = curSlice;
+ if (isMotion)
+ {
+ (*pNextSlice)++;
+ childSlice = *pNextSlice;
+ }
+
+ CollectCTESlices(mp, pexprChild, childSlice, pNextSlice, prodInfos,
+ consInfos);
+ }
+}
+
+BOOL
+CUtils::FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ if (NULL == pexpr)
+ {
+ return false;
+ }
+
+ CTEInfoArray *prodInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ CTEInfoArray *consInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ ULONG nextSlice = 0;
+
+ CollectCTESlices(mp, pexpr, 0 /*curSlice*/, &nextSlice, prodInfos,
+ consInfos);
+
+ BOOL cross = false;
+
+ for (ULONG ic = 0; ic < consInfos->Size(); ic++)
+ {
+ SCTEInfo *cons = (*consInfos)[ic];
+
+ for (ULONG ip = 0; ip < prodInfos->Size(); ip++)
+ {
+ SCTEInfo *prod = (*prodInfos)[ip];
+ if (prod->cteId == cons->cteId && prod->sliceId != cons->sliceId)
+ {
+ cross = true;
+ goto lExit;
Review Comment:
don't use goto in gporca.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#issuecomment-4427302279
> LGTM to fallback temporary
Agree with considering it as a temporary solution. Besides, I have another open PR in [open-gpdb](https://github.com/open-gpdb/gpdb/pull/370) where the Consumer adds its own local copy of the Producer's subtree, so orca is able to generate correct plan here. However, I noticed that it covers not all similar cases and because of this I started with fallback approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206515722
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+ CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
+
+ static BOOL FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp,
+ CExpression *pexpr);
Review Comment:
Fixed in the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206514261
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild, cteConsumers,
+ cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id), sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
Review Comment:
fixed. I added it in the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206518211
##########
src/backend/gporca/libgpopt/include/gpopt/base/CUtils.h:
##########
@@ -1026,6 +1026,21 @@ class CUtils
static CTableDescriptorHashSet *RemoveDuplicateMdids(
CMemoryPool *mp, CTableDescriptorHashSet *tabdescs);
+ // hash set from CTE ids
+ typedef CHashSet<ULONG, gpos::HashValue<ULONG>, gpos::Equals<ULONG>,
+ CleanupDelete<ULONG> >
+ UlongCteIdHashSet;
+
+ static void CollectConsumersAndProducers(CMemoryPool *mp,
+ CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet);
+
+ static BOOL hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr);
Review Comment:
yes,agree. added it in the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on code in PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#discussion_r3206577638
##########
src/backend/gporca/libgpopt/src/base/CUtils.cpp:
##########
@@ -978,6 +978,180 @@ CUtils::FHasCTEAnchor(CExpression *pexpr)
return false;
}
+// return CTEConsumers' and a set of CTEProducers' CTE ids in the given subtree
+void
+CUtils::CollectConsumersAndProducers(CMemoryPool *mp, CExpression *pexpr,
+ ULongPtrArray *cteConsumers,
+ UlongCteIdHashSet *cteProducerSet)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ cteConsumers->Append(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId()));
+ }
+ else if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ cteProducerSet->Insert(GPOS_NEW(mp) ULONG(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId()));
+ }
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (!pexprChild->Pop()->FScalar())
+ {
+ CollectConsumersAndProducers(mp, pexprChild, cteConsumers,
+ cteProducerSet);
+ }
+ }
+}
+
+BOOL
+CUtils::hasUnpairedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ BOOL hasUnpairedConsumer = false;
+
+ ULongPtrArray *cteConsumers = GPOS_NEW(mp) ULongPtrArray(mp);
+ UlongCteIdHashSet *cteProducerSet = GPOS_NEW(mp) UlongCteIdHashSet(mp);
+
+ CollectConsumersAndProducers(mp, pexpr, cteConsumers, cteProducerSet);
+
+ // check if every consumer's producer is in ProducerSet
+ for (ULONG ul = 0; ul < cteConsumers->Size(); ul++)
+ {
+ if (!cteProducerSet->Contains((*cteConsumers)[ul]))
+ {
+ hasUnpairedConsumer = true;
+ break;
+ }
+ }
+ cteConsumers->Release();
+ cteProducerSet->Release();
+
+ return hasUnpairedConsumer;
+}
+
+// True if the distribution is replicated-like.
+static BOOL
+FReplicatedLikeDistribution(CDistributionSpec::EDistributionType edt)
+{
+ return (CDistributionSpec::EdtStrictReplicated == edt ||
+ CDistributionSpec::EdtTaintedReplicated == edt ||
+ CDistributionSpec::EdtUniversal == edt);
+}
+
+struct SCTEInfo
+{
+ ULONG cteId;
+ ULONG sliceId;
+
+ SCTEInfo(ULONG cte_id, ULONG slice_id) : cteId(cte_id), sliceId(slice_id)
+ {
+ }
+};
+
+typedef CDynamicPtrArray<SCTEInfo, CleanupDelete<SCTEInfo> > CTEInfoArray;
+
+// Walk the physical tree, recording the slice id of every replicated
+// CTE Producer and every CTE Consumer. Slices are delimited by Motion
+// nodes: each non-scalar child of a Motion lives in a fresh slice --
+// same motId-stack idea as in apply_shareinput_xslice.
+static void
+CollectCTESlices(CMemoryPool *mp, CExpression *pexpr, ULONG curSlice,
+ ULONG *pNextSlice, CTEInfoArray *prodInfos,
+ CTEInfoArray *consInfos)
+{
+ COperator *pop = pexpr->Pop();
+
+ if (COperator::EopPhysicalCTEProducer == pop->Eopid())
+ {
+ // Producer's distribution comes from its only child -- inspect
+ // it there. Skip non-replicated Producers; they cannot trigger
+ // the cross-slice issue we are checking for.
+ GPOS_ASSERT(1 == pexpr->Arity());
+ CExpression *pexprChild = (*pexpr)[0];
+ CDrvdPropPlan *pdpplan =
+ CDrvdPropPlan::Pdpplan(pexprChild->PdpDerive());
+
+ if (FReplicatedLikeDistribution(pdpplan->Pds()->Edt()))
+ {
+ prodInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEProducer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+ }
+ else if (COperator::EopPhysicalCTEConsumer == pop->Eopid())
+ {
+ // Consumer is a leaf -- record (cteId, curSlice) and let the
+ // caller decide later, once the whole tree has been walked.
+ consInfos->Append(GPOS_NEW(mp) SCTEInfo(
+ CPhysicalCTEConsumer::PopConvert(pop)->UlCTEId(), curSlice));
+ }
+
+ BOOL isMotion = CUtils::FPhysicalMotion(pop);
+
+ for (ULONG ul = 0; ul < pexpr->Arity(); ul++)
+ {
+ CExpression *pexprChild = (*pexpr)[ul];
+
+ if (pexprChild->Pop()->FScalar())
+ {
+ continue;
+ }
+
+ ULONG childSlice = curSlice;
+ if (isMotion)
+ {
+ (*pNextSlice)++;
+ childSlice = *pNextSlice;
+ }
+
+ CollectCTESlices(mp, pexprChild, childSlice, pNextSlice, prodInfos,
+ consInfos);
+ }
+}
+
+BOOL
+CUtils::FHasCrossSliceReplicatedCTEConsumer(CMemoryPool *mp, CExpression *pexpr)
+{
+ if (NULL == pexpr)
+ {
+ return false;
+ }
+
+ CTEInfoArray *prodInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ CTEInfoArray *consInfos = GPOS_NEW(mp) CTEInfoArray(mp);
+ ULONG nextSlice = 0;
+
+ CollectCTESlices(mp, pexpr, 0 /*curSlice*/, &nextSlice, prodInfos,
+ consInfos);
+
+ BOOL cross = false;
+
+ for (ULONG ic = 0; ic < consInfos->Size(); ic++)
+ {
+ SCTEInfo *cons = (*consInfos)[ic];
+
+ for (ULONG ip = 0; ip < prodInfos->Size(); ip++)
+ {
+ SCTEInfo *prod = (*prodInfos)[ip];
+ if (prod->cteId == cons->cteId && prod->sliceId != cons->sliceId)
+ {
+ cross = true;
+ goto lExit;
Review Comment:
Agree. As I understood it might be dangerous since we can jump over object construction or skip destructors. I replaced it with the a small function that uses a plain return true for the early exit (see last commit).
Thank you for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org
Re: [PR] orca: fallback to Postgres optimizer on cross-slice replicated CTE Consumer [cloudberry]
Posted by "Alena0704 (via GitHub)" <gi...@apache.org>.
Alena0704 commented on PR #1719:
URL: https://github.com/apache/cloudberry/pull/1719#issuecomment-4427470214
> maybe the best way to fix the root cause is fixing the Sequence::pdsRequired, in my opinion, this path is an illegal path which should be pruned by pdsRequired restrition. What's more, maybe we can add restriction between CCTEReq and CCTEMap by checking the motion worker number, only when they have the same motion workers, CCTEMap::FSatisfies will return true and we will get a legal path. Prefer to the first path, it's more formal than the second which looks like a remedial plan.
Thanks for the suggestion. Agreed that pruning during search is cleaner than a post-hoc fallback, but both proposed variants only cover part of the cases.
Sequence::PdsRequired already does the right thing on its own boundary: when the first child derives Replicated, it requests EdtReplicated on the second child. The issue is that the Motion which breaks slice alignment is enforced deeper inside the second child's subtree (typically a Gather under a scalar subquery) - so it lands in a different slice than the Producer, and that's not constrained by the spec Sequence sets at its own boundary.
CCTEMap::FSatisfies + motion worker count - at FSatisfies time we only know the kind of distribution from CDistributionSpec. Concrete slice ids don't exist yet - they get assigned later, when Motion nodes are enforced, and because of this we can't catch the original cross-slice problem.
The current walker-based approach feels more general and reliable to me, because it catches the cross-slice problem regardless of which Motion produced it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cloudberry.apache.org
For additional commands, e-mail: commits-help@cloudberry.apache.org