Fix whole-row target alias deparsing in distributed INSERT RETURNING
- Dominant language
- C
- Stars
- 12.8k
- Forks
- 794
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 31
Description
> **Updated after investigation.** This was originally filed under PG19 feature work. It has
> since been **reproduced on PG18 on plain `main`**, so it is a general, version-neutral Citus
> deparser bug, not a PG19 defect. The PG19 `ON CONFLICT DO SELECT` failure is one *symptom*
> of it. Labels and scope updated accordingly.
## Problem
Distributed `INSERT` deparsing fails when `RETURNING` references the target relation as a whole
row. Citus emits a cast from the *shard's* physical composite to the *logical* distributed
table's composite type:
```sql
target_alias.*::logical_table_type
```
On the worker the row has physical type `target_`, and PostgreSQL cannot cast between two
named composite types:
```text
ERROR: cannot cast type target_ to target
```
### Reproduction (PG18, plain `main`, 2-node cluster, `citus.log_remote_commands = on`)
```sql
CREATE TABLE target (tid int PRIMARY KEY, balance int);
SELECT create_distributed_table('target', 'tid');
INSERT INTO target AS t VALUES (1, 100) RETURNING t;
```
Worker query actually sent, verbatim:
```text
INSERT INTO public.target_8762000 AS t (tid, balance) VALUES (1, 100)
RETURNING t.*::public.target AS t
ERROR: cannot cast type target_8762000 to target
```
`UPDATE`/`DELETE`/`SELECT` are **not** affected — they already emit the shard type. This is the
built-in positive control that isolates the defect to `INSERT`:
```text
UPDATE public.target_8762000 t SET balance = (balance OPERATOR(pg_catalog.+) 1)
WHERE (tid OPERATOR(pg_catalog.=) 1)
RETURNING t.*::public.target_8762000 AS t --> (1,101)
```
## Root cause
Citus's copied ruleutils has **two independent** shard-naming mechanisms:
1. Marking the RTE `CITUS_RTE_SHARD` → `generate_rte_shard_name(rte)`. Populated by
`UpdateRelationToShardNames()`; used by `SELECT`/`UPDATE`/`DELETE`/`MERGE`.
2. `context->distrelid` + `context->shardid` → `generate_relation_or_shard_name(...)`. Used by
`deparse_shard_query()`.
`DeparseTaskQuery()` (`planner/deparse_shard_query.c`) routes `CMD_INSERT` to
`deparse_shard_query()`, and `UpdateRelationToShardNames()` cannot run on `INSERT`. So an
`INSERT`'s target RTE is **never** `CITUS_RTE_SHARD`.
In `get_variable()`, a top-level whole-row `Var` gets a `::type` suffix (upstream PostgreSQL's
deliberate hack to stop the parser re-expanding `tab.*` into a column list). The
`CITUS_RTE_SHARD` test fails on `INSERT`, so it falls through to the generic branch and emits
`format_type_with_typemod(var->vartype, ...)` — the logical type. `deparse_shard_query()` *does*
set `distrelid`/`shardid`, which is the available hook.
## Fix
Add a branch to `get_variable()` that uses the shard's composite type when deparsing a top-level
whole-row `Var` of the distributed table inside a shard query:
```text
INSERT INTO public.target_8762000 AS t (tid, balance) VALUES (1, 100)
RETURNING t.*::public.target_8762000 AS t --> (1,100)
```
**Deviation from this issue's original proposal, deliberate:** the original text proposed
`ROW(alias.*)::logical_table_type`. Casting to the **shard** type is preferred because (a) it is
exactly what Citus already does on the `UPDATE`/`DELETE`/`SELECT` paths, so `INSERT` becomes
consistent rather than introducing a third pattern; (b) it is an identity cast, so no row is
rebuilt and there is no exposure to dropped-column or attribute-ordering skew that a `ROW(...)`
reconstruction can hit. Happy to switch if maintainers prefer the original formulation — it is a
one-line change.
## Affected versions / branches
`main` is now **PG17/18/19** (PG16 dropped in #8757, PG19 added in #8753). The buggy block is
byte-identical in every copied ruleutils on `main` and on both live release branches, so all
need the fix:
| Branch | `default_version` | ruleutils copies carrying the bug |
|---|---|---|
| `main` | — | `ruleutils_17.c`, `ruleutils_18.c`, `ruleutils_19.c` |
| `release-14.0` | 14.3-1 | `ruleutils_16.c`, `ruleutils_17.c`, `ruleutils_18.c` |
| `release-13.2` | 13.5-1 | `ruleutils_15.c`, `ruleutils_16.c`, `ruleutils_17.c`, `ruleutils_18.c` |
The fix is C-only and ports verbatim: `context->distrelid`, `context->shardid` and
`generate_relation_or_shard_name()` all exist with identical signatures in every one of those
files, so **no `#if PG_VERSION_NUM` guard is required**. Backport to `release-13.2` and
`release-14.0` after the `main` PR merges.
## Out of scope — separate, adjacent defect
An `INSERT` whose `RETURNING` names the table **without** an alias fails differently and is
**not** fixed here:
```sql
INSERT INTO target VALUES (2, 200) RETURNING target;
```
```text
INSERT INTO public.target_8762001 (tid, balance) VALUES (2, 200)
RETURNING target.*::public.target_8762001 AS target
ERROR: missing FROM-clause entry for table "target"
```
Note the cast is now *correct* — only the refname is unresolvable, which proves the two defects
are independent. Cause: `get_insert_query_def()` prints the shard name and then calls stock
`get_rte_alias()`, which suppresses the alias because `refname` (`target`) equals
`get_relation_name(rte->relid)`, the *logical* name. `get_update_query_def()` has a dedicated
shard branch that appends the alias unconditionally, which is why `UPDATE`/`DELETE` work
unaliased.
A fix was prototyped and measured working, but it appends `AS ` to **every** plain
distributed `INSERT`, changing deparse output in `pg17.out`, `pg17_json.out`,
`single_node_enterprise.out` and `sql_procedure_no_transaction_block.out` — which contradicts the
"existing deparser output remains correct" criterion below. To be filed separately.
`ON CONFLICT` is already covered by the fix above, because Citus injects an alias on that path:
```text
INSERT INTO public.r_8763000 AS citus_table_alias (id, v) VALUES (1, 99)
ON CONFLICT(id) DO UPDATE SET v = excluded.v
RETURNING citus_table_alias.*::public.r_8763000 AS r --> (1,99)
```
## PG19 context (original report)
PG19 `ON CONFLICT DO SELECT` was otherwise verified correct across router and multi-shard VALUES
inserts, colocated `INSERT...SELECT` pushdown, coordinator fallback and repartition plans,
distributed/reference/Citus-local/plain-local/mixed variants, RETURNING values and cardinality,
and `FOR UPDATE`/`FOR SHARE` lock preservation on workers. PG17/18 reject `DO SELECT` at parse
time, but ordinary whole-row `INSERT ... RETURNING` reproduces the same deparser failure there —
which is what established that this is version-neutral.
## Acceptance criteria
- [ ] Ordinary distributed `INSERT ... RETURNING ` whole-row succeeds and returns the
**correct value** (not merely no error).
- [ ] Column-expanded `RETURNING t.*` remains unchanged.
- [ ] `ON CONFLICT DO UPDATE ... RETURNING ` whole-row succeeds.
- [ ] `UPDATE`/`DELETE` whole-row `RETURNING` remain correct (regression guards).
- [ ] Existing PG17/18 deparser output is unchanged — no churn in existing `expected/*.out`.
- [ ] Version-neutral regression coverage lives in an existing deparser/RETURNING test file, not
in `pg19.sql`.
- [ ] PG19 `ON CONFLICT DO SELECT RETURNING old/new/` succeeds for representative router
and non-router paths.
- [ ] Backported to `release-13.2` and `release-14.0`.
Contributor guide
Research direction
Start in planner/deparse_shard_query.c and the copied ruleutils_17.c, ruleutils_18.c, and ruleutils_19.c files; compare get_variable() with the existing UPDATE/DELETE/SELECT shard-name behavior and inspect the distrelid/shardid path. Run or extend the existing deparser/RETURNING regression coverage for aliased whole-row INSERT and ON CONFLICT, then verify column-expanded RETURNING and existing outputs remain unchanged before backporting to the listed release branches.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, postgresql
- Domain
- databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 65/100