citusdata / citusdata/citus

PG19: contain eager aggregation in the Citus join restriction hook

Open
#8,771 2 comments 0 reactions 0 assignees View on GitHub
pg19_features
Dominant language
C
Stars
12.8k
Forks
794
Avg merge
2d 14h
Merged PRs (30d)
31

Description

Part of #8597 (Phase 3 — later feature integration).

## Problem

PG19 adds eager aggregation, and `enable_eager_aggregate` **defaults to on**. The planner now builds a second, *grouped* `RelOptInfo` for join relations. Citus's join restriction hook fires for those grouped rels with no guard, so Citus records duplicate join restrictions that carry the **same relids** as the plain join rel.

## Call chain (traced in 19beta2 source)

- `optimizer/path/joinrels.c:920-1005` — builds `grouped_rel` for a join rel.
- `optimizer/path/joinrels.c:1055` — calls `populate_joinrel_with_paths(..., grouped_rel, ...)`.
- `populate_joinrel_with_paths` calls `add_paths_to_joinrel` in every jointype branch.
- `optimizer/path/joinpath.c:379-381` — `set_join_pathlist_hook` fires here, **with no grouped-rel guard**.

Citus side:

- `src/backend/distributed/planner/distributed_planner.c:2101-2141` — `multi_join_restriction_hook` unconditionally appends a `JoinRestriction` to the context and ORs `hasSemiJoin` / `hasOuterJoin`.

## The relation-side hook is safe — verified, not assumed

- `optimizer/path/allpaths.c:589-590` — `set_rel_pathlist_hook` fires.
- `allpaths.c:620` — `set_grouped_rel_pathlist()` runs *after* it.
- `allpaths.c:1384-1403` — grouped paths are built onto `rel->grouped_rel`, a **separate** `RelOptInfo`.

So `multi_relation_restriction_hook` (`distributed_planner.c:2150`) needs no change. Only the join hook is exposed.

## Impact

- **Certain:** planning work that Citus collects and then discards, on every distributed join, by default.
- **Plausible but unproven:** duplicate restrictions with identical relids altering a resulting plan. This has *not* been demonstrated and should not be described as a correctness bug until it is.

Why it is still worth treating as a risk class rather than a curiosity: `src/backend/distributed/planner/recursive_planning.c:512-532` documents the PG16 `695f5deb7` precedent, where an upstream change to *when* join hooks are invoked produced silently incorrect Citus plans.

## Options

1. Force `enable_eager_aggregate = off` for the duration of Citus restriction collection (narrow, reversible, matches the existing outer-join GUC handling around `shared_library_init.c:1591-1606`).
2. Use the new PG19 levers instead — `join_path_setup_hook` (`joinpath.c:177-179`, `optimizer/paths.h:32-38`) and `pgs_mask` / `PGS_JOIN_ANY` (`pathnodes.h:71,97,263,1039,3616-3626`) — which allow suppressing the grouped variant directly rather than filtering after the fact.

Option 1 is the smaller change; option 2 is the more precise one.

## First step

Reproduce on a live PG19 cluster: a distributed join with grouping, `enable_eager_aggregate` on vs off, comparing both the collected restriction count and the final plan. That determines whether this is a performance-only fix or a correctness fix.

Contributor guide

Open the contributing guide

Research direction

Start with a live PG19 cluster and reproduce a distributed join with grouping while toggling enable_eager_aggregate, comparing collected restriction counts and final plans. Read optimizer/path/joinrels.c, optimizer/path/joinpath.c, and distributed_planner.c:2101-2141; done means the chosen integration avoids duplicate grouped-join restrictions without changing valid planning behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, postgresql
Domain
backend, databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.