citusdata / citusdata/citus

PG19: reject FOR PORTION OF modifications planned by Citus

Open
#8,761 1 comment 0 reactions 0 assignees Claimed by @ihalatci View on GitHub
pg19_features
Dominant language
C
Stars
12.8k
Forks
794
Avg merge
2d 14h
Merged PRs (30d)
31

Description

## Problem

PostgreSQL 19 adds temporal modifications:

```sql
UPDATE table_name FOR PORTION OF valid_at FROM ... TO ...;
DELETE FROM table_name FOR PORTION OF valid_at FROM ... TO ...;
```

When Citus plans/deparses these statements, it silently drops the `FOR PORTION OF` clause. PostgreSQL's rewritten overlap predicate and range intersection remain, but the executor-only leftover-row insertion phase is lost.

## Verified impact

Starting from one row valid over `[2020-01-01,2030-01-01)` and modifying/deleting `[2023-01-01,2025-01-01)`:

- Correct PostgreSQL UPDATE produces left, changed-middle, and right fragments.
- Citus remote UPDATE keeps only the middle fragment.
- Correct PostgreSQL DELETE preserves left and right fragments.
- Citus remote DELETE removes the entire overlapping row.
- RETURNING and command tags remain plausible, making the loss difficult to detect.
- Remote execution also omits the INSERT trigger cycles PostgreSQL fires for leftover rows.

Reproduced across router, multi-shard, single-shard, reference, Citus-local, partitioned, modifying-CTE, and ordinary-local-target/distributed-source paths. Local execution can appear correct accidentally; forcing remote execution exposes deterministic loss.

If the temporal range is the distribution column, leftover fragments can belong to different shards, so simply restoring clause deparsing would still be unsafe.

## Root cause

PG19 `Query.forPortionOf` is carried to the PostgreSQL executor, which locks the original tuple and inserts untouched left/right ranges. Citus's PG19 ruleutils copy does not emit the temporal clause and sends an ordinary rewritten UPDATE/DELETE to workers. The worker executor therefore never runs temporal leftover handling.

## Severity

High PG19 support release blocker: valid SQL causes deterministic silent data loss/corruption without an error.

## Proposed minimum fix

Explicitly reject every PG19 temporal modification that enters Citus modification planning:

- Add a PG19-gated check for `queryTree->forPortionOf != NULL` in `ModifyPartialQuerySupported()` before routing/target-list validation.
- Return `ERRCODE_FEATURE_NOT_SUPPORTED` with a stable message.
- Do not create router-only, reference, Citus-local, local-execution, or partitioned exceptions.
- Pure PostgreSQL local-table statements that never enter Citus planning remain supported.

## Acceptance criteria

- Ordinary PostgreSQL local temporal UPDATE/DELETE remains correct.
- Distributed temporal UPDATE and DELETE reject consistently with local execution on/off.
- Modifying CTEs reject consistently.
- Ordinary local targets that read distributed sources reject consistently.
- Temporal DELETE with the period as distribution column rejects before any row loss.
- PG17/18 builds and behavior remain unchanged through version-gated field access.
- PG19 regression coverage verifies the rejection boundaries and unchanged local behavior.

## Non-goals

Real distributed support requires preserving the original temporal clause, shard-correct leftover routing, reference replication, partition tuple routing, EvalPlanQual/locking, trigger semantics, and RETURNING behavior. It is out of scope for this rejection fix.
## Scope clarifications

- PostgreSQL 19 grammar permits `FOR PORTION OF` on UPDATE and DELETE only. MERGE and its reduced action syntax do not accept the clause, so no `CreateMergePlan()` guard is needed.
- The one PG19-gated check in `ModifyPartialQuerySupported()` covers both supported temporal target spellings:
- `FROM start_time TO end_time`
- `(portion)`
- PostgreSQL inserts zero to two range leftovers, or at most one multirange leftover. Leftovers preserve original values in non-period columns, including the distribution column; the verified Citus defect is silent **loss** of leftover rows after deparsing, not demonstrated cross-shard misrouting.
- PostgreSQL does not require INSERT privilege for these executor-generated leftovers. Any future real support must preserve that privilege behavior; the rejection fix does not emulate leftovers.
## Accepted local implementation

Local commit `9453db38c` implements the approved minimum rejection in `ModifyPartialQuerySupported()` with mandatory PG19 gating and focused PG19 regression coverage for both temporal target forms and all validated Citus planning boundaries.

Validation:

- PG17/18/19 `-Werror` builds succeeded.
- PG19 and PG18 focused base/planner runs passed 17/17.
- PG17 relevant `pg19`, `multi_modifications`, and `citus_local_tables_queries` tests passed.
- `make check-style`, final diff checks, and independent release review passed.

No MERGE guard is needed because PostgreSQL 19 grammar does not admit `FOR PORTION OF` in MERGE.

Implementation PR: #8763.

Contributor guide

Open the contributing guide

Research direction

Read ModifyPartialQuerySupported() and the PG19 regression coverage referenced in the issue, including pg19, multi_modifications, and citus_local_tables_queries. Compare the accepted local commit and implementation PR #8763; done means consistent rejection at the Citus planning boundaries while ordinary local temporal UPDATE/DELETE and PG17/18 behavior remain unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, postgresql, sql
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
15/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.