citusdata / citusdata/citus

Fix whole-row target alias deparsing in distributed INSERT RETURNING

Open
#8,762 1 comment 0 reactions 0 assignees View on GitHub
backport bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.