my-ship-it commented on issue #1867:
URL: https://github.com/apache/cloudberry/issues/1867#issuecomment-5113691196
## Root Cause Analysis
Thanks @adnanhamdussalam for the reproducer and @liang8283 for narrowing it
down. I dug into this — below is the full analysis.
### Summary
This is not a wrong-results bug and not an ORCA optimizer failure per se: it
is a **GPORCA planner fallback** triggered on **every UPDATE of an AO / AOCO /
PAX table whose SET list does not touch a distribution (or partition) key
column**. ORCA aborts DXL→PlStmt translation, and the whole statement — joins,
aggregates and all — is re-planned by the Postgres planner. The spill files and
the slowdown reported here come from that fallback plan.
The defect was introduced by a CBDB-specific patch that forces `isSplit =
true` for append-optimized tables in the *translator*, while the *optimizer*
has already produced a plan with no Split node. The two ends disagree, and the
translator then looks up a column that nothing produces.
### Reproduction (verified on current `main`)
```sql
CREATE TABLE t (a int, b int) WITH (appendonly=true) DISTRIBUTED BY (a);
SET optimizer_trace_fallback = on;
EXPLAIN UPDATE t SET b = 1;
INFO: GPORCA failed to produce a plan, falling back to Postgres-based
planner
DETAIL: DXL-to-PlStmt Translation: Attribute number 7 not found in project
list
```
| Case (`optimizer=on`) | Result |
|---|---|
| heap, non-distribution column | GPORCA (in-place update) |
| **AO row, non-distribution column** | **fallback** |
| **AOCO, non-distribution column** | **fallback** |
| **AO, `DISTRIBUTED RANDOMLY`** | **fallback** |
| AO, distribution column (`SET a = 1`) | GPORCA (Split Update) |
| AO, DELETE | GPORCA |
The number in the message is an ORCA **colid**, not a `pg_attribute.attnum`.
It follows `n_columns + n_updated_columns + 4` (2 cols / 1 updated → 7; 3 / 1 →
8; 3 / 2 → 9; 4 / 1 → 9), i.e. it is always the **last colref allocated — the
DML action column**. For the reporter's 52+ column table this lands on 72,
matching the title.
### Mechanism
Three code sites, in order:
**1. The optimizer downgrades the split update, ignoring storage type.**
`src/backend/gporca/libgpopt/src/operators/CExpressionPreprocessor.cpp:3232`
(`ConvertSplitUpdateToInPlaceUpdate`, invoked unconditionally at `:3612`) turns
`CLogicalUpdate(fSplit=true)` into `fSplit=false` whenever no distribution or
partition key column is modified. Storage type is never consulted.
**2. The action column becomes an orphan.**
`src/backend/gporca/libgpopt/src/xforms/CXformUpdate2DML.cpp:99` allocates
`pcrAction` unconditionally, but only the `fSplit == true` branch (`:102`)
builds the `CLogicalSplit` that projects it. `CTranslatorExprToDXL.cpp:5590`
then writes that colid into the DXL `ActionCol` attribute regardless. Harmless
as long as nobody resolves it.
**3. The translator resurrects `isSplit` and resolves it.**
`src/backend/gpopt/translate/CTranslatorDXLToPlStmt.cpp`:
```c
BOOL isSplit = phy_dml_dxlop->FSplit(); // :5257 -> false in the
DXL
...
if (md_rel->IsNonBlockTable()) // :5290 -> AO / AOCO / PAX
{
isSplit = true; // AO tables are always use split updates
}
...
if (m_cmd_type == CMD_UPDATE && isSplit) // :5364
AddJunkTargetEntryForColId(&dml_target_list, &child_context,
phy_dml_dxlop->ActionColId(), "DMLAction");
```
The lookup misses the child translate context and
`CMappingColIdVarPlStmt.cpp:201` raises `ExmiDXL2PlStmtAttributeNotFound`,
which ORCA reports as a fallback.
### The premise behind the override is incorrect
The comment asserts that AO tables always require a split update. The
Postgres planner in CBDB does not agree: `check_splitupdate()`
(`src/backend/optimizer/prep/preptlist.c:129`) requests a split update **only
when a distribution key column is modified**, independent of storage type. An
AO in-place UPDATE is executed as visimap-delete plus same-segment append
inside the table AM, and since the distribution key is unchanged the tuple
never has to move between segments. That is exactly the plan the fallback
produces today, and it is correct. So the executor has no such requirement —
ORCA has been paying for a constraint that does not exist.
### Provenance and blast radius
```
1659ae11ae5 [ORCA] Implemented InPlaceUpdate ... (upstream #13889,
cherry-picked)
f573ee1d39b Fix failure of ORCA non-split update for CBDB <-- introduces
the override
0af03caefc6 [ORCA] Support PAX AM in ORCA <-- widens
IsAORowOrColTable() to IsNonBlockTable(), pulling PAX in
```
`f573ee1d39b` is the very next commit after the in-place-update xform; both
landed in the ORCA sync of 2024-11-01.
- **Affected releases:** `2.0.0-incubating` and later, including current
`main`. `1.6.0` is not affected.
- **Affected statements:** UPDATE on AO row, AOCO and PAX tables where the
SET list touches no distribution/partition key column. INSERT, DELETE, and
distribution-key UPDATE are unaffected.
- **Severity:** no incorrect results; ORCA is silently disabled for the
entire statement. With the default `optimizer_trace_fallback = off` there is no
diagnostic at all — the only symptom is a plan regression.
### Why regression tests did not catch it
The same commit rewrote the affected `*_optimizer` expected outputs to match
the fallback plan: `src/test/regress/expected/ao_locks_optimizer.out` and
`src/test/isolation2/output/uao/parallel_update_optimizer.source` both went
from `n segments` to `1 segment`, which is exactly the Postgres planner's
directly-dispatched in-place update. The regression was baked into the expected
files.
### Proposed fix
**Option A (preferred) — drop the override and let ORCA match the Postgres
planner.**
```diff
--- a/src/backend/gpopt/translate/CTranslatorDXLToPlStmt.cpp
- if (md_rel->IsNonBlockTable())
- {
- isSplit = true; // AO tables are always use split updates
- }
```
Aligns ORCA with `check_splitupdate()`, restores ORCA planning for these
statements, and yields the better plan (no Split node, no Explicit Redistribute
Motion, direct dispatch stays available). Requires re-validating the
AO/AOCO/PAX update suites and reverting the expected-output changes listed
above.
**Option B (conservative) — teach the optimizer the same rule the translator
assumes.**
```diff
--- a/src/backend/gporca/libgpopt/src/operators/CExpressionPreprocessor.cpp
BOOL split_update = false;
+ // AO/AOCO/PAX tables cannot be updated in place
+ if (tabdesc->IsNonBlockTable())
+ split_update = true;
```
(`CTableDescriptor::IsNonBlockTable()` already exists —
`CTableDescriptor.h:241`.) This restores pre-2024-11 behaviour and honours the
original intent with a minimal, low-risk change, at the cost of a plan that is
strictly worse than what the Postgres planner produces.
Recommendation: land Option A, keeping Option B as the fallback if PAX turns
out to have a genuine in-place-update restriction. Either way a regression test
should be added that runs a non-distribution-key UPDATE against AO, AOCO and
PAX under `optimizer_trace_fallback = on` and asserts that no fallback occurs.
### Notes for @adnanhamdussalam
- The slow plan you have today *is* the Postgres planner's plan. Adding an
explicit `SET optimizer = off` around those UPDATEs will not make anything
slower — it only skips a wasted ORCA planning attempt. Getting an actual ORCA
plan requires the heap / CTAS workaround @liang8283 described, or this fix.
- There is no GUC workaround: `ConvertSplitUpdateToInPlaceUpdate` runs
unconditionally during preprocessing, with no trace flag guarding 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
