columnar: serialize CREATE INDEX on columnar tables for PG19 - #8618
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pg19-support #8618 +/- ##
===============================================
Coverage ? 88.94%
===============================================
Files ? 288
Lines ? 64391
Branches ? 8096
===============================================
Hits ? 57275
Misses ? 4782
Partials ? 2334 🚀 New features to boost your workflow:
|
a31f940 to
83042a9
Compare
9d9144a to
3a0a90e
Compare
83042a9 to
59c5dc3
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses PostgreSQL 19’s default parallel CREATE INDEX behavior to ensure columnar TableAM index builds run safely in a serial mode and no longer fail due to missing/unsupported parallel-scan plumbing.
Changes:
- Implement parallel-scan callback support for columnar by delegating initialization/reinitialization to the block-table helper callbacks (to satisfy PG19 callers that size/init parallel scan state unconditionally).
- Ensure columnar index builds ignore any provided parallel scan descriptor and proceed via the serial scan path.
- Force
max_parallel_maintenance_workers=0forCREATE INDEXtargeting columnar relations by temporarily nesting/restoring the GUC aroundPrevProcessUtilityHook.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static Size | ||
| columnar_parallelscan_estimate(Relation rel) | ||
| { | ||
| elog(ERROR, "columnar_parallelscan_estimate not implemented"); | ||
| return sizeof(ParallelBlockTableScanDescData); | ||
| } |
There was a problem hiding this comment.
Done — columnar_parallelscan_estimate now delegates to table_block_parallelscan_estimate(rel) for symmetry with columnar_parallelscan_initialize (which already delegates to table_block_parallelscan_initialize).
One clarification on the under-allocation concern: it doesn't apply today. table_block_parallelscan_estimate() returns exactly sizeof(ParallelBlockTableScanDescData) — the serialized snapshot is sized/allocated separately by the caller (EstimateSnapshotSpace / table_parallelscan_initialize), not into this descriptor — so the returned value is identical to what was here before. Delegating is still the right call for future-proofing in case the AM-level helper ever grows variable-sized state.
3a0a90e to
2ff1cfd
Compare
PG19 enables parallel CREATE INDEX by default (max_parallel_maintenance_workers defaults to 2). Columnar's TableAM is always serial: rs_parallel is stored but never read, and the build path flushes pending writes, which is disallowed inside a parallel operation. Three changes: 1. Provide working parallelscan_estimate/initialize/reinitialize that delegate to the table_block_* helpers, so callers that unconditionally size and initialize a ParallelTableScanDesc (e.g. PG19's parallel btree build) do not abort. Columnar ignores the descriptor, so the state is simply unused. 2. In columnar_index_build_range_scan, accept a parallel scan descriptor by discarding it (scan = NULL) and running the serial path. 3. In ColumnarProcessUtility, when the statement is CREATE INDEX on a columnar AM relation, force max_parallel_maintenance_workers=0 via NewGUCNestLevel/AtEOXact_GUC around PrevProcessUtilityHook so the build runs serially without triggering "cannot update tuples during a parallel operation". DESCRIPTION: Serialize CREATE INDEX on columnar tables on PG19 Closes #8611 Part of #8597
59c5dc3 to
c70d228
Compare
| saveNestLevel = NewGUCNestLevel(); | ||
| (void) set_config_option("max_parallel_maintenance_workers", "0", | ||
| PGC_USERSET, PGC_S_SESSION, | ||
| GUC_ACTION_SAVE, true, 0, false); |
There was a problem hiding this comment.
Should the removal of parallelism also be applied for REINDEX statements ? They have a separate node tag, T_ReindexStmt, which is not directly handled by this switch statement afaict, so falls to the default branch. As is, would a REINDEX of a columnar table with pending writes get parallel mode and hit the failure ?
There was a problem hiding this comment.
Good catch — confirmed this is a real gap. REINDEX (T_ReindexStmt) fell through to the default branch, so a REINDEX of a columnar table with pending writes did enter parallel mode and failed with cannot update tuples during a parallel operation while flushing the pending writes (which updates the stripe reservation entry in columnar.stripe).
Fixed by adding a T_ReindexStmt case that resolves the target relation and sets indexBuildOnColumnar when it's columnar, so the same max_parallel_maintenance_workers = 0 override now covers REINDEX too:
REINDEX_OBJECT_TABLE→ the named relation;REINDEX_OBJECT_INDEX→ itsIndexGetRelation()heap.
REINDEX {SCHEMA,DATABASE,SYSTEM} are intentionally not handled: they can't run inside a transaction block, so there are never pending writes to flush. Added a pending-writes REINDEX regression test (reindex_events) that reproduces the original failure on the unfixed build and passes with the fix.
| if (indexBuildOnColumnar) | ||
| { | ||
| saveNestLevel = NewGUCNestLevel(); | ||
| (void) set_config_option("max_parallel_maintenance_workers", "0", |
There was a problem hiding this comment.
nit: (void) goes against existing calls of set_config_option() (e.g. in PostprocessReassignOwnedStmt(), citus_internal_database_command())
There was a problem hiding this comment.
Done — dropped the (void) cast to match the existing set_config_option() call sites (e.g. PostprocessReassignOwnedStmt(), citus_internal_database_command()).
| AtEOXact_GUC(true, saveNestLevel); | ||
| } | ||
| } | ||
| PG_END_TRY(); |
There was a problem hiding this comment.
Wondering if the current tests in columnar_index.sql are enough to validate the fixes ?
A couple of test suggestions:
- set max_parallel_maintenance_workers to e..g 4 before a
CREATE INDEXon a columnar table, and then show that it is still 4 after theCREATE INDEX. - Similar GUC setting for a REINDEX operation;
SET LOCAL debug_parallel_query = regress;
SET LOCAL max_parallel_workers = 4;
REINDEX TABLE parallel_scan_test;
REINDEX TABLE CONCURRENTLY parallel_scan_test;
SHOW LOCAL max_parallel_workers;
There was a problem hiding this comment.
Added both suggestions. (1) Set max_parallel_maintenance_workers = 4, then SHOW it after both CREATE INDEX and REINDEX on a columnar table to confirm it's restored to 4. (2) A pending-writes REINDEX test mirroring the existing events CREATE INDEX test.
Two deliberate adjustments to the snippet:
- I assert on
max_parallel_maintenance_workersrather thanmax_parallel_workers. The override only touches the former;max_parallel_workersstays 4 regardless of the fix, so asserting on it wouldn't actually catch a regression. - I target a fresh table with actual pending writes inside a txn block instead of
parallel_scan_test— the latter has hadVACUUM FULL, so it has no pending writes and wouldn't exercise the flush-during-reindex path that this fix addresses.
…y cast - columnar_parallelscan_estimate now delegates to table_block_parallelscan_estimate(rel), symmetric with columnar_parallelscan_initialize (Copilot review). - Extend the serial-index-build override to REINDEX: T_ReindexStmt previously fell through to the default branch, so REINDEX TABLE/INDEX on a columnar table with pending writes could enter parallel mode and fail with "cannot update tuples during a parallel operation" while flushing the stripe reservation update. Set indexBuildOnColumnar for REINDEX_OBJECT_TABLE/INDEX on columnar relations. SCHEMA/DATABASE/ SYSTEM cannot run in a transaction block, so they carry no pending writes and need no handling (colm-mchugh review). - Drop the (void) cast on set_config_option to match existing call sites (colm-mchugh nit). - Tests: add a REINDEX-with-pending-writes regression mirroring the existing CREATE INDEX "events" test, and assert that a user-set max_parallel_maintenance_workers is restored after both CREATE INDEX and REINDEX on a columnar table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
colm-mchugh
left a comment
There was a problem hiding this comment.
Lgtm. One nit - is the OidIsValue check in the REINDEX branch strictly necessary ? Wouldn't REINDEX on an invalid relation have errored out at that point?
will check and address with a follow up later |
…8619) Stacked PR (PR3b of the PG19 stack). **Base = `pg19-columnar-index` (#8618)** → `pg19-runtime-fixes` (#8617) → `pg19-ci-test-matrices` (#8616) → `pg19-ruleutils-port` (#8602) → `pg19-support`. Review/merge in stack order. PG19 removed `get_relation_info_hook`, which is where pre-PG19 builds disable parallel query and index-only scans for columnar relations (columnar scans are always serial and cannot return tuples from an index). The Phase-1 build left a `TODO(PG19, #8614)` placeholder; this PR re-implements that suppression inside `ColumnarSetRelPathlistHook`: - forbid future parallel workers (`rel_parallel_workers = 0`, `consider_parallel = false`) and drop any already-generated partial paths; - clear each index's `canreturn` flags and strip any `IndexOnlyScan` paths that were already created (`set_rel_pathlist_hook` runs after path generation). The pre-PG19 `ColumnarGetRelationInfoHook` path is unchanged. Closes #8614 Part of #8597 --------- Co-authored-by: Ibrahim Halatci <ihalatci@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Stacked PR (PR3 of the PG19 stack). Base =
pg19-runtime-fixes(#8617) →pg19-ci-test-matrices(#8616) →pg19-ruleutils-port(#8602) →pg19-support. Review/merge in stack order.PG19 enables parallel
CREATE INDEXby default (max_parallel_maintenance_workersdefaults to 2). Columnar's TableAM is always serial —rs_parallelis stored but never read, and the build path flushes pending writes, which is disallowed inside a parallel operation. This causedCREATE INDEXon columnar tables to fail on PG19.Three changes (all in
columnar_tableam.c):parallelscan_estimate/initialize/reinitializenow delegate to thetable_block_*helpers, so callers that unconditionally size and initialize aParallelTableScanDesc(e.g. PG19's parallel btree build) no longer abort. Columnar ignores the descriptor, so the written state is simply unused.columnar_index_build_range_scanaccepts a parallel scan descriptor by discarding it (scan = NULL) and running the serial path.ColumnarProcessUtilityforcesmax_parallel_maintenance_workers=0(viaNewGUCNestLevel/AtEOXact_GUCaroundPrevProcessUtilityHook) forCREATE INDEXon a columnar AM relation, so the build runs serially.Closes #8611
Part of #8597