Skip to content

Let ORCA plan in-place UPDATE of AO tables; fix AO row scan expanding unread columns - #2094

Merged
tuhaihe merged 2 commits into
apache:mainfrom
lss602726449:cherrypick-orca-ao-inplace-update
Oct 16, 2026
Merged

tuhaihe merged 2 commits into
apache:mainfrom
lss602726449:cherrypick-orca-ao-inplace-update

Conversation

@lss602726449

Copy link
Copy Markdown
Contributor

Cherry-picked two commits from the hashdata-lightning repo.

Commits

Conflict resolution notes

  • src/test/regress/greenplum_schedule: added only orca_split_update to the ORCA group. Dropped orca_material_rescan from the lightning hunk because its test files do not exist in this repo.
  • Kept contrib/pax_storage/src/test/vec/expected/partition_prune_optimizer.out and contrib/vectorization/src/test/regress/expected/partition_prune_aocs_optimizer.out deleted (modify/delete conflicts — these files do not exist in cloudberry).

🤖 Generated with Claude Code

@lss602726449
lss602726449 force-pushed the cherrypick-orca-ao-inplace-update branch 3 times, most recently from dd81d08 to afbf741 Compare October 9, 2026 02:26

@yjhjstz yjhjstz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Alena0704

Copy link
Copy Markdown
Collaborator

Please add initial authors of the patches and the projects where these commits came from in commit messages.

@Alena0704

Copy link
Copy Markdown
Collaborator

I've noticed some UPDATEs now dispatch to all segments and use two-phase commit instead of single-segment execution. Could you benchmark this regression, especially on larger clusters?
Also please add targeted regression tests for partial attribute reads and materialization, including reading a materialized slot after the scan advances to another buffer or ends.

@leborchuk leborchuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lss602726449

lss602726449 commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Please add initial authors of the patches and the projects where these commits came from in commit messages.

initial authors is me

I've noticed some UPDATEs now dispatch to all segments and use two-phase commit instead of single-segment execution. Could you benchmark this regression, especially on larger clusters?
Also please add targeted regression tests for partial attribute reads and materialization, including reading a materialized slot after the scan advances to another buffer or ends.

I have't test this. But I just enable update in orca, and you can use PG optimizer if you like. It's just optional.

@Alena0704

Copy link
Copy Markdown
Collaborator

Please add initial authors of the patches and the projects where these commits came from in commit messages.

initial authors is me

I've noticed some UPDATEs now dispatch to all segments and use two-phase commit instead of single-segment execution. Could you benchmark this regression, especially on larger clusters?
Also please add targeted regression tests for partial attribute reads and materialization, including reading a materialized slot after the scan advances to another buffer or ends.

I have't test this. But I just enable update in orca, and you can use PG optimizer if you like. It's just optional.

Please add initial authors of the patches and the projects where these commits came from in commit messages.

initial authors is me

I've noticed some UPDATEs now dispatch to all segments and use two-phase commit instead of single-segment execution. Could you benchmark this regression, especially on larger clusters?
Also please add targeted regression tests for partial attribute reads and materialization, including reading a materialized slot after the scan advances to another buffer or ends.

I have't test this. But I just enable update in orca, and you can use PG optimizer if you like. It's just optional.

Sorry. I couldn't find the initial commit and when you linked to the commit of another author in the PR I thought it was another author. As for test, I have noticed that the query plan in one of the tests became worse and because of this suggested to run benchmark.
Anyway, I think changes look fine in the commit.

lss602726449 and others added 2 commits October 16, 2026 14:00
Any UPDATE on an AO, AOCS or PAX table that does not modify a
distribution key column made GPORCA give up and silently fall back to
the Postgres planner:

  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

With optimizer_trace_fallback off, which is the default, none of this
reaches the client; only EXPLAIN's "Optimizer:" line gives it away.

Since PG14 a non-split UPDATE builds the new tuple from the plan slot
plus the old tuple, and ExecModifyTable fetches that old tuple by tid.
Append-optimized tables cannot do that lookup -- it would mean scanning
the block directory and the visibility map per tuple, and the block
directory is not guaranteed to exist -- so appendonly_tuple_fetch_row_
version() errors out with "feature not supported on appendoptimized
relations".

The planner works around this by adding a wholerow junk column for such
tables (see the GPDB_14_MERGE_FIXME in add_row_identity_columns), which
makes oldtuple non-NULL and skips the fetch.  ORCA has no such column,
so CTranslatorDXLToPlStmt::TranslateDXLDml() forced a split update
instead.  But CExpressionPreprocessor::ConvertSplitUpdateToInPlaceUpdate()
had already downgraded the plan to an in-place update -- it only looks at
whether a distribution or partition key column is modified, never at the
access method -- so the DMLAction column the forced split needs was not
in the plan, and the translation failed.

The old tuple is only needed for the columns the subplan does not
assign.  ORCA's non-split UPDATE assigns every live column, so nothing
reads it.  Record that in ri_projectNewNeedsOldTuple while building the
update projection and skip the fetch when it is false, which lets an
append-optimized table run the plan, and drop the forced split.

This is what the FIXME asks for: the update plan now has the same shape
whether it comes from the planner or from ORCA, and it matches what the
storage engine already does, since appendonly_tuple_update() is itself a
delete followed by an append.  Iceberg needs no special case either --
its UPDATE writes the position delete and the new row in one
ExecForeignUpdate call.

The skip is limited to append-optimized relations for now.  Letting heap
take it as well would remove the wholerow junk column the planner ships
for those tables, but needs the trigger, EPQ and cross-partition update
paths audited first.

Forcing the split the other way round was tried first and rejected: it
made nine queries that update the same row twice in one statement fail
with "multiple updates to a row by the same query is not allowed", which
only a split update raises.

The answer files change where a recorded fallback disappears, where an
UPDATE plan gains its ORCA shape, and in ao_locks and
uao/parallel_update_{row,column}, which go back to locking all segments
because ORCA does not do direct dispatch for these updates.

See: Issue#603
(cherry picked from commit befb03fea31fd7d4f75a0ab4cdd624f83705777a)
…gineering/dev_collaboration#965

## Summary

Fixes field-engineering/dev_collaboration#965: a `count(1)` over a 281 column
append-only **row** table, qualified on three date columns, was 6x slower than the same
data in a heap table and 14x slower than AOCS.

### Root cause

`AppendOnlyExecutorReadBlock_ProcessTuple()` called `memtuple_deform()` and then
`ExecStoreVirtualTuple()`, and `appendonly_slot_callbacks()` returned `TTSOpsVirtual`.
A virtual slot promises its values are already in `tts_values[]`, so
`ExecStoreVirtualTuple()` sets `tts_nvalid = natts` and `slot_getattr()` never reaches
`getsomeattrs` — the scan has no choice but to expand **every** column of **every**
tuple. Heap tables are unaffected because `TTSOpsBufferHeapTuple` deforms incrementally
up to the highest attribute number the query references.

Scan cost therefore grew with the **width of the table** instead of the columns actually
read. Measured on a release build (`-O3`, no cassert), 500k rows, query touching only
attno 2:

| table columns | AO row before | heap |
|---|---|---|
| 20 | 46.9 ms | 35.7 ms |
| 100 | 114.2 ms | 36.8 ms |
| 281 | **236.0 ms** | 39.1 ms |

This is not how the format is meant to be read. A MemTuple keeps precomputed attribute
offsets in its binding, so any attribute is reachable in constant time, and before the
PostgreSQL 12 merge (`19cd1cf4b6`) the slot carried that binding and fetched attributes
on demand. That merge dropped the MemTuple slot shape — which at the time could only
exist as extra fields in the shared `TupleTableSlot` struct — and converted the tuple
eagerly instead. PostgreSQL 12 also introduced `TupleTableSlotOps` precisely so an access
method can keep tuples in its native format, which is what this MR finally uses.

**The same code is present in GPDB 7 and Apache Cloudberry main**, both verified to show
the identical regression (GPDB 7 release: 193.8 ms vs heap 28.9 ms at 281 columns). This
is an inherited PG12-merge artifact, not something introduced by Lightning.

### Change

Add `TTSOpsMemTuple`, a slot holding the MemTuple together with the binding needed to
interpret it, expanding attributes in `getsomeattrs`. Because offsets are precomputed,
`tts_nvalid` alone is the resume point — no separate deforming cursor (the `off` field
heap and minimal tuple slots carry) is needed. `memtuple_deform_range()` reuses the
existing inlinable internal helper so full-column access keeps the old bulk-deform speed.

Two details are easy to miss when adding a slot type here, both found by running the full
regression suite:

- `gp_segment_id` and `gp_foreign_server` are answered by the slot even though no tuple
  format carries them, so `getsysattr` must keep the special cases that
  `tts_virtual_getsysattr` documents. Omitting them broke ~59 tests.
- Not every slot reaches the scan through `table_slot_create()`.
  `AppendOnlySegmentFileFullCompaction()` built a virtual slot directly and passed it to
  `appendonly_getnextslot()`; in a release build the assertion is compiled out and the
  larger slot struct would be written past the end of the smaller one. It now asks the
  access method for the slot type, and `ProcessTuple` additionally falls back to expanding
  the tuple up front for any non-MemTuple slot, so a caller bringing its own slot cannot
  be handed one it cannot read.

## Test plan

Release build (`-O3`, `debug_assertions=off`), demo cluster, 500k rows.

The two tables below pin one variable each. Query is
`SELECT count(1) FROM t WHERE c2 > 0`, i.e. it references attno 2 only, unless stated.

**1. Cost no longer depends on how wide the table is.** Query fixed on attno 2, table width varied:

| table columns | before | after | heap |
|---|---|---|---|
| 20 | 46.9 ms | 35.9 ms | 35.7 ms |
| 50 | 67.3 ms | 36.9 ms | 38.4 ms |
| 100 | 114.2 ms | 36.0 ms | 36.8 ms |
| 200 | 171.1 ms | 37.6 ms | 39.0 ms |
| **281** | **236.0 ms** | **41.4 ms** | 39.1 ms |
| 281, all columns populated | 415.0 ms | 112.7 ms | 103.6 ms |

**2. Cost now depends on the highest attribute number referenced, exactly as heap does.**
Table fixed at 281 columns, referenced column varied (`count(cN)`):

| referenced up to | after | heap |
|---|---|---|
| c2 | 37.3 ms | 36.1 ms |
| c50 | 68.2 ms | 56.4 ms |
| c140 | 121.5 ms | 95.4 ms |
| c281 | 212.4 ms | 156.2 ms |

Slopes: AO 0.63 ms per column per 500k rows, heap 0.43 ms. So the win is not that MemTuple
deforms faster than heap — it does not — it is that unread columns are no longer deformed at
all. A query reaching the last column costs what it always cost; the flat line in table 1 is
a consequence of referencing attno 2, not an unconditional property.

The reported query references attno 9, 12 and 13, so it now expands 13 columns instead of
281. `getsomeattrs` fills a prefix, so the gap columns (1-8, 10, 11) are still expanded;
skipping those would need per-column projection, which `tts_nvalid` cannot express (it is a
single watermark, so validity is necessarily a prefix) and which would mean claiming
unfilled columns are valid. Not attempted here.

Full-column access is not penalised: `count(c281)` 217.7 ms (before the change a bare scan
already cost 236.0 ms because it deformed everything), `sum(c281)` on fully populated data
415.9 ms vs 415.0 ms.

**Correctness**
- [x] `SELECT * FROM ao EXCEPT SELECT * FROM heap` and the reverse both return 0 rows, over
      all 281 columns, for sparse and fully populated data
- [x] System columns unchanged: `ctid` / `tableoid` / `gp_segment_id` work, the four MVCC
      columns still error (a MemTuple carries no MVCC fields; visibility lives in the visimap)
- [x] Materialize-dependent paths: CTE MATERIALIZED, ORDER BY, CTAS round trip, cursor
      across statements
- [x] `make installcheck` A/B on the same cluster: **baseline 36/723 failed, with this
      change 36/723 failed, failure sets identical**. The 36 are environmental in this
      build (no pax_storage / iceberg AM, ORCA disabled).
- [x] Builds clean under `-O3 -Werror`

**Not done**
- [ ] No regression test added for the wide-table-narrow-projection behaviour. Both bugs
      above surfaced only from the full suite, so this path is under-covered; a test
      exercising a wide table plus the VACUUM / CLUSTER / CREATE INDEX paths would be
      worth adding. Happy to add it to this MR if preferred.
- [ ] Not yet verified on GPDB 7 / upstream Cloudberry, though the patch applies to the
      identical code there.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

(cherry picked from commit 47a358712540acfa3ecd7ee1cb778ed01d5298f4)
@tuhaihe
tuhaihe force-pushed the cherrypick-orca-ao-inplace-update branch from afbf741 to db834d4 Compare October 16, 2026 06:00
@tuhaihe
tuhaihe enabled auto-merge (rebase) October 16, 2026 06:00
@tuhaihe
tuhaihe merged commit 342c89d into apache:main Oct 16, 2026
118 of 119 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants