Repository navigation
PG19: propagate EXPLAIN IO to worker nodes - #8807
ibrahim halatci (ihalatci) wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ihalatci-pg19-repack-lock-fix #8807 +/- ##
=================================================================
- Coverage 88.82% 88.82% -0.01%
=================================================================
Files 290 290
Lines 65495 65501 +6
Branches 8261 8262 +1
=================================================================
+ Hits 58175 58178 +3
- Misses 4937 4939 +2
- Partials 2383 2384 +1 🚀 New features to boost your workflow:
|
c2e0c3d to
3483bc2
Compare
DESCRIPTION: Update PostgreSQL 19 CI and regression coverage to RC1 Update build, nightly cassert, and pg_upgrade image pins to the published PostgreSQL 19 RC1 release images from merged citusdata/the-process#253 (image suffix `-va0a5582`). The stable image publication succeeded for merge commit `a0a5582b39e72cec476f7b6ba37a2cf97572b930` in [the-process build-test-images run](https://github.com/citusdata/the-process/actions/runs/38888435235). Preserve the existing exclusion of PG19 from Citus upgrade coverage. Add a distributed `ON CONFLICT` regression for PostgreSQL's fix to inference when an index expression on a NOT NULL column simplifies to a Var ([upstream commit](postgres/postgres@f9ccc5e)). No Citus source change was indicated: the regression fails on PostgreSQL 19 Beta 4 and passes on RC1. ## RC1 feature audit The audited upstream range is `REL_19_BETA4` (`b73d13c32c834a2c8e1c60cb92f79530376cedf1`) through `REL_19_RC1` (`0b8bff042a997fe50fcd1c4c83b9ef1cc9ed8a89`): 132 commits and 300 changed paths, with no feature-revert commit. The Citus umbrella features remain present in RC1: - REPACK / REPACK CONCURRENTLY: #8756 / #8805 remain relevant. - EXPLAIN IO: #8770 / #8807 remain relevant. - Constraint enforceability: #8758 / #8808 remain relevant. - JSON COPY: merged #8760 / #8765 remains compatible with RC1. - Distributed REPACK CONCURRENTLY support: #8772 remains a design-only exception without an implementation PR. The approved dependent topology is: - native stack `#8929 -> #8805 -> #8807`; - #8808 as an independent sibling based directly on this branch after removing unrelated DETACH changes. Version-neutral #8759 and #8762 are no longer subissues of #8782. PostgreSQL RC2 is scheduled for 2026-11-12 and GA for 2026-11-19. RC2 must be rechecked for the open upstream eager-aggregation and CHECK-enforcement items before final PG19 feature completion. ## Validation Local validation on PostgreSQL 19 RC1: `pg19` (7/7), `multi_upsert` (7/7), `multi_insert_select_conflict` (7/7), `multi_copy` (15/15), `multi_outer_join` (7/7), `multi_deparse_shard_query` (7/7), `multi_deparse_function` (7/7), and `multi_join_order_additional` (16/16, with base schedule and whole schedule line). `merge` also passed on Beta 4 after installing the missing `postgres_fdw` test dependency. The repartition planning/pruning/task-assignment debug task-ID output differences reproduce on both Beta 4 and RC1; no expected output was changed for them. The prior Citus head `7259ac9c94e9a6595eac092b18afc47b69197d4a` passed all automatic checks. This suffix-only update switches CI to the successfully published stable RC1 images; checks for the current head will run after this update. --------- Co-authored-by: Ibrahim Halatci <ihalatci@microsoft.com> Copilot-Session: f2aeb1f7-5872-4dc8-a288-2bdafcf80c04
3483bc2 to
a1f6c08
Compare
PG19 adds an IO option to EXPLAIN. Citus forwards a fixed set of options to the worker nodes, so IO was silently dropped: EXPLAIN (ANALYZE, IO) on a distributed table succeeded but the worker plans contained no IO information. Because IO requires ANALYZE, the option travels over the saved-plan path (WrapQueryForExplainAnalyze -> worker_save_query_explain_analyze -> ExplainWorkerPlan), not over BuildRemoteExplainQuery. The decisive gap was ExplainWorkerPlan(), which rebuilds instrument_option from scratch and did not set INSTRUMENT_IO, so forwarding the option alone would have had no effect. Forwards es->io through the option struct, the remote EXPLAIN query, the JSONB options blob and back into instrument_option, mirroring the existing PG17 MEMORY/SERIALIZE handling. Fixes #8770 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6481a75c-8b50-4476-832e-fbe9e4fb1196
a1f6c08 to
436d684
Compare
Onur Tirtir (onurctirtir)
left a comment
There was a problem hiding this comment.
approving with some suggestions
| CurrentDistributedQueryExplainOptions.memory = es->memory; | ||
| CurrentDistributedQueryExplainOptions.serialize = es->serialize; | ||
| #if PG_VERSION_NUM >= PG_VERSION_19 | ||
| CurrentDistributedQueryExplainOptions.io = es->io; |
There was a problem hiding this comment.
This propagates IO for EXPLAIN (ANALYZE, IO) <query>. It does not work for EXPLAIN (ANALYZE, IO) EXECUTE <prepared>.
CurrentDistributedQueryExplainOptions is only filled in here, in the ExplainOneQuery_hook. PostgreSQL does not call that hook for EXPLAIN EXECUTE. So the static global keeps either the defaults or the previous EXPLAIN's options.
Repro on PG19rc1 with this PR. ps is PREPARE ps(int) AS SELECT count(*) FROM t WHERE b > $1 on a distributed table.
- In a fresh session, run
EXPLAIN (ANALYZE, IO) EXECUTE ps(0). The worker receives"io": false, and there is noPrefetch:line. - In another session, first run
EXPLAIN (ANALYZE, IO, BUFFERS, VERBOSE) SELECT .... Then runEXPLAIN (ANALYZE, IO OFF, BUFFERS OFF, VERBOSE OFF) EXECUTE ps(0). The worker receives"verbose": true, "buffers": true, "io": true. The worker plan showsPrefetch:,Buffers:, andOutput:, even though all three were turned off.
This is pre-existing. BUFFERS and VERBOSE already behave the same way on main. So I don't think it has to block this PR. It would be worth a separate issue, though, since IO now inherits the same problem. A possible fix is to capture the options from the ExplainStmt in the utility hook, so that EXECUTE gets them too.
A test that catches it. It currently returns t for the second call, and the correct answer is f:
PREPARE explain_io_ps(int) AS SELECT count(*) FROM explain_io WHERE b > $1;
SELECT explain_has_prefetch($$
EXPLAIN (ANALYZE, IO, COSTS OFF, TIMING OFF, SUMMARY OFF)
SELECT count(*) FROM explain_io $$);
-- no IO requested, so must be f
SELECT explain_has_prefetch($$
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF) EXECUTE explain_io_ps(0) $$);| * can be NULL for some cases of es->analyze == true. The same is true for a | ||
| * distributed subplan: its saved output is keyed by task id and consumed once when | ||
| * replayed into a freshly planned task list, so a task without a matching entry | ||
| * falls through to a live remote EXPLAIN here. |
There was a problem hiding this comment.
This new sentence describes a bug as if it were the design.
For a subplan, every task is supposed to find its saved output. That is what #8017 added to fix #4212, so a subplan is not run twice. A task only falls through to a live EXPLAIN here when its task id in the re-plan differs from its task id in the first run. That is a bug. When it happens:
- one task prints another shard's plan, and
- another task runs the query a second time on the worker. The savepoint rolls it back.
The bug is pre-existing, because this PR does not touch ExtractAnalyzeStats(). Could you open an issue for it, and change this comment to call it a known bug with a link to that issue, instead of describing it as expected? For example:
* can be NULL for some cases of es->analyze == true. A distributed subplan
* can also end up here when its re-planned task ids do not match the ids its
* saved output was stored under; that is a known bug, see #XXXX.A repro for the issue. With 2 shards, row 6 goes to shard 8775001 (port 10702), and rows 7 and 8 go to shard 8775000 (port 10701):
SET citus.shard_count TO 2;
SET citus.next_shard_id TO 8775000;
CREATE TABLE t(a int, b int);
SELECT create_distributed_table('t', 'a');
SET citus.explain_all_tasks TO on;
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF, BUFFERS OFF)
WITH c AS (INSERT INTO t VALUES (6,66),(7,77),(8,88) RETURNING *) SELECT * FROM c;Actual output (PG19rc1):
-> Distributed Subplan 1_1
-> Custom Scan (Citus Adaptive) (actual rows=1.00 loops=1)
-> Task
Tuple data received from node: 8 bytes
Node: host=localhost port=10701 dbname=...
-> Insert on t_8775001 citus_table_alias (actual rows=1.00 loops=1)
-> Task
Tuple data received from node: 0 bytes
Node: host=localhost port=10702 dbname=...
-> Insert on t_8775001 citus_table_alias (actual rows=1.00 loops=1)
Expected:
- port 10701 shows
Insert on t_8775000with 2 rows. - port 10702 shows its saved plan, not a live re-run ("0 bytes").
- the subplan reports 3 rows, not 1.
| -- EXPLAIN ANALYZE of a distributed subplan makes Citus build the remote EXPLAIN | ||
| -- statement itself instead of reusing a plan saved by the worker, so IO has to | ||
| -- be forwarded on that path as well. The plan of the INSERT tasks reads nothing | ||
| -- and therefore has no IO section, the assertion here is the logged command. | ||
| SET citus.shard_count TO 2; | ||
| SET citus.next_shard_id TO 8770000; | ||
| CREATE TABLE explain_io_mod(a int, b int); | ||
| INSERT INTO explain_io_mod VALUES (1, 11); | ||
| SELECT create_distributed_table('explain_io_mod', 'a'); | ||
| SET citus.explain_all_tasks TO on; | ||
| SET citus.log_remote_commands TO on; | ||
| SET citus.grep_remote_commands TO '%IO TRUE%'; | ||
| SELECT explain_has_prefetch($$ | ||
| EXPLAIN (ANALYZE, IO, COSTS OFF, TIMING OFF, SUMMARY OFF) | ||
| WITH cte_1 AS (INSERT INTO explain_io_mod VALUES (6,66),(7,77),(8,88) RETURNING *) | ||
| SELECT * FROM cte_1 $$); |
There was a problem hiding this comment.
This test's oracle depends on the task id mismatch bug described on the RemoteExplain comment.
The IO TRUE remote command only appears here because one INSERT task fails to find its saved output. It then falls back to a live remote EXPLAIN. If someone fixes that mismatch, this test's output changes, even though IO propagation would still be correct. The comment "a distributed subplan makes Citus build the remote EXPLAIN statement itself instead of reusing a plan saved by the worker" is not true in general either. Read-only subplans reuse the saved plan.
Suggestion: replace this test with a read-only subplan. It goes through the same subplan explain path, and it gives a real positive and negative oracle on the plan itself. I verified both on PG19rc1 with this PR. Each task shows Prefetch: avg=1.00 max=1 capacity=94.
SET citus.explain_all_tasks TO on;
-- t
SELECT explain_has_prefetch($$
EXPLAIN (ANALYZE, IO, COSTS OFF, TIMING OFF, SUMMARY OFF)
WITH cte_1 AS MATERIALIZED (SELECT * FROM explain_io_mod)
SELECT count(*) FROM cte_1 $$);
-- f
SELECT explain_has_prefetch($$
EXPLAIN (ANALYZE, COSTS OFF, TIMING OFF, SUMMARY OFF)
WITH cte_1 AS MATERIALIZED (SELECT * FROM explain_io_mod)
SELECT count(*) FROM cte_1 $$);
RESET citus.explain_all_tasks;Optionally, also cover a non-text format, since the worker uses different keys there. With 2 shards and explain_all_tasks on, this returns 2:
DO $d$
DECLARE j json; n int;
BEGIN
EXECUTE 'EXPLAIN (ANALYZE, IO, COSTS OFF, TIMING OFF, SUMMARY OFF, FORMAT JSON) SELECT count(*) FROM explain_io_mod' INTO j;
SELECT count(*) INTO n FROM jsonb_path_query(j::jsonb, 'strict $.**."Average Prefetch Distance"');
RAISE NOTICE 'prefetch keys: %', n;
END $d$;For the record, I also checked IO on these paths: multi-shard, router, reference table, local execution on the coordinator, XML, YAML, and IO combined with WAL, MEMORY, and SERIALIZE. All of them work.
Propagate PostgreSQL 19 RC1 EXPLAIN IO to worker nodes.
Stack
Native stack layer 3: #8929 -> #8805 -> #8807
This PR is based on
ihalatci-pg19-repack-lock-fixat805849c8ed7b3fa5e77edf289c5e2b159b4bb529.Changes
ExplainState.ioto worker EXPLAIN commands on PostgreSQL 19+.INSTRUMENT_IOthrough the saved-plan EXPLAIN ANALYZE path.pg19regression tests.The diff relative to #8805 is limited to:
src/backend/distributed/planner/multi_explain.csrc/test/regress/bin/normalize.sedsrc/test/regress/sql/pg19.sqlsrc/test/regress/expected/pg19.outValidation
src/test/regress/citus_tests/run_test.py pg19: passed (7/7).src/test/regress/citus_tests/run_test.py multi_explain: passed (16/16).src/test/regress/citus_tests/run_test.py multi_mx_explain: passed (10/10).citus_indent --check: passed.ci/editorconfig.sh: passed.git diff --check: passed.make check-style: repository-baseline flake8 failures only; reproducedunchanged on PG19: use ShareUpdateExclusiveLock for REPACK CONCURRENTLY #8805. The changed C file and editorconfig checks pass.
Fixes #8770