From df613d175d8172c75d3a99a2d3a7b4a363992eca Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 3 Aug 2026 03:59:02 -0600 Subject: [PATCH] fix: a rewrite's own projection ids are not foreign reservations (#345) pgcolumnar.sort_status reported ~90% decay on a table that had just been fully reclustered, whenever the table had a projection. The rows were correctly ordered; only the reporting was wrong. record_online_sorted_extent walks the rewrite's own reserved stripe ids and stops at the first gap, on the reasoning that a gap means another session took that id. That reasoning is false within the rewrite's own transaction. A projection writes through its own inner write state but calls ColumnarWriteRow with the BASE relation, so it reserves from this relation's stripe counter; its groups are then recorded under the projection's own storage id and never appear in the base relation's group list. ColumnarWriteStateStripeIds returned only the base write state's reservations, so base and projection draws alternated, ours came back as 17, 19, 21, ... and the run walk broke at the first step. The rewrite was competing with itself. The ids are now unioned with the projection fan-out's, taking only those at or above the rewrite's own first id, so anything drawn before it began is still excluded. A projection id inside the run is harmless to sort_status: it counts base-relation groups, and a projection's group number is not one. Reproduced with a control, which is what establishes the projection as the cause rather than assuming it. Both arms are identical except for add_projection: noproj sorted 200000, appended 0 (correct) withproj sorted 20000, appended 180000 (wrong) Removal proof: with the union reverted, the withproj arm fails at 20000/180000 against 200000/0 while the noproj control stays green, so the test is pinned to this mechanism and not to breakage in general. This is the opposite error to #342 in the same function. #342 was the mark claiming groups it should not; this is the mark refusing groups it should. The range fix in #344 addressed the first and left runEnd computed by the same consecutive-run walk, so this survived it. Direction of error here is the safe one -- more decay reported than exists, so at worst an unnecessary re-sort -- but a table reporting 90% decay right after a successful full recluster makes sort_status useless for deciding when a re-sort is worth its cost. Full 15-19 matrix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011miCFRSatixeNRw3w5yNq8 --- src/columnar.h | 1 + src/columnar_vacuum.c | 36 +++++++++++++++++++++++- src/columnar_write_state.c | 57 ++++++++++++++++++++++++++++++++++++++ test/recluster_extent.sh | 30 ++++++++++++++++++++ 4 files changed, 123 insertions(+), 1 deletion(-) diff --git a/src/columnar.h b/src/columnar.h index d01c377..c6e6146 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -517,6 +517,7 @@ typedef struct ColumnarWriteState ColumnarWriteState; extern ColumnarWriteState *ColumnarGetWriteState(Relation rel); extern int ColumnarWriteStateStripeCount(ColumnarWriteState *ws); extern uint64 *ColumnarWriteStateStripeIds(ColumnarWriteState *ws, int *n); +extern uint64 *ColumnarWriteStateProjStripeIds(ColumnarWriteState *ws, int *n); extern uint64 ColumnarWriteRow(ColumnarWriteState *writeState, Relation rel, Datum *values, bool *nulls); extern void ColumnarProjectionFanoutRow(Relation rel, ColumnarWriteState *baseWs, diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index fb4e97b..ee84d7c 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -336,11 +336,45 @@ record_online_sorted_extent(Relation rel, uint64 storageId, if (nOurs <= 0) return; /* this rewrite reserved nothing */ - /* our own reservations, ascending */ + /* + * Our own reservations, ascending -- including the ones our projection + * fan-out drew (#345). A projection writes through its own write state but + * reserves from this relation's stripe counter, so its ids interleave with + * ours and would otherwise read as foreign reservations, truncating the run + * at the first projection flush. They are ours: the same transaction drew + * them. Only ids at or above our own first one are taken, so anything drawn + * before this rewrite began is still excluded. + */ ours = (uint64 *) palloc(sizeof(uint64) * nOurs); memcpy(ours, all + stripeMark, sizeof(uint64) * nOurs); qsort(ours, nOurs, sizeof(uint64), uint64_cmp); + { + int nProj = 0; + uint64 *projIds = ColumnarWriteStateProjStripeIds(writeState, &nProj); + + if (nProj > 0) + { + uint64 lo = ours[0]; + uint64 *merged = (uint64 *) palloc(sizeof(uint64) * (nOurs + nProj)); + int m = nOurs; + int j; + + memcpy(merged, ours, sizeof(uint64) * nOurs); + for (j = 0; j < nProj; j++) + { + if (projIds[j] >= lo) + merged[m++] = projIds[j]; + } + pfree(ours); + ours = merged; + nOurs = m; + qsort(ours, nOurs, sizeof(uint64), uint64_cmp); + } + if (projIds != NULL) + pfree(projIds); + } + /* the consecutive run of our own ids, starting at the lowest */ runEnd = ours[0]; for (i = 1; i < nOurs; i++) diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index b16f6d6..c94e4eb 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -1457,6 +1457,63 @@ typedef struct ColumnarProjWriter MemoryContext rowCtx; /* reset after each stripe flush: row datums */ ColumnarWriteState *innerWs; /* reused stripe encoder for this projection */ } ColumnarProjWriter; +/* + * ColumnarWriteStateProjStripeIds + * The stripe ids this write state's projection fan-out drew (#345). + * + * A projection writes through its own inner write state but reserves from + * the BASE relation's stripe counter, because ColumnarWriteRow is called + * with the base relation (see flush_proj_writer). Its groups are recorded + * under the projection's own storage id, so they never appear in the base + * relation's row group list. + * + * That combination is why the caller needs these separately. To + * record_online_sorted_extent, an id drawn by its own projection fan-out is + * indistinguishable from one taken by another session: both leave a gap in + * the base write state's ids. Treating the former as foreign truncated the + * ordered run at the first projection flush, so a fully reclustered table + * with a projection reported almost all of itself as decayed. + * + * Returns a palloc'd array in the caller's context, or NULL when this write + * state has no projection writers. + */ +uint64 * +ColumnarWriteStateProjStripeIds(ColumnarWriteState *ws, int *n) +{ + ListCell *lc; + uint64 *ids = NULL; + int total = 0; + int k = 0; + + *n = 0; + if (ws->projWriters == NIL) + return NULL; + + foreach(lc, ws->projWriters) + { + ColumnarProjWriter *w = (ColumnarProjWriter *) lfirst(lc); + + if (w->innerWs != NULL) + total += w->innerWs->nReservedStripeIds; + } + if (total == 0) + return NULL; + + ids = (uint64 *) palloc(sizeof(uint64) * total); + foreach(lc, ws->projWriters) + { + ColumnarProjWriter *w = (ColumnarProjWriter *) lfirst(lc); + int i; + + if (w->innerWs == NULL) + continue; + for (i = 0; i < w->innerWs->nReservedStripeIds; i++) + ids[k++] = w->innerWs->reservedStripeIds[i]; + } + *n = k; + return ids; +} + /* * columnar_build_write_state diff --git a/test/recluster_extent.sh b/test/recluster_extent.sh index ee41762..c78c13b 100755 --- a/test/recluster_extent.sh +++ b/test/recluster_extent.sh @@ -144,6 +144,36 @@ check "the id-drawn-below counts still cover every stored row" \ "$(raw "SELECT (sorted_rows + appended_rows = (SELECT sum(rowcount) FROM pgcolumnar.stats('d')))::text FROM pgcolumnar.sort_status('d');")" "true" +# ------------------------------- a projection must not read as foreign (#345) + +# The rewrite's projection fan-out writes through its own write state but draws +# stripe ids from THIS relation's counter, and records its groups under the +# projection's own storage id. So its ids interleave with the rewrite's own and +# are invisible in the base relation's group list -- exactly what a foreign +# reservation looks like. Treating them as foreign truncated the ordered run at +# the first projection flush, so a table that had just been fully reclustered +# reported almost all of itself as decayed. +# +# Both arms are identical except for the projection, so the projection is +# established as the cause rather than assumed. +for arm in noproj withproj; do + raw "DROP TABLE IF EXISTS pr_$arm;" >/dev/null + raw "CREATE TABLE pr_$arm (id int, k int, v text) USING pgcolumnar;" >/dev/null + raw "SELECT pgcolumnar.set_options('pr_$arm', stripe_row_limit => 20000);" >/dev/null + raw "INSERT INTO pr_$arm SELECT g, ((g::bigint * 7919) % 100000)::int, 'v' || g + FROM generate_series(1, 200000) g;" >/dev/null + if [ "$arm" = withproj ]; then + E="$(raw "SELECT pgcolumnar.add_projection('pr_$arm', 'p_$arm', ARRAY['k','id'], ARRAY['k']);")" + case "$E" in *ERROR*) echo " add_projection failed: $E";; esac + fi + raw "SELECT pgcolumnar.recluster('pr_$arm', 'id', 'k');" >/dev/null + S="$(raw "SELECT sorted_rows FROM pgcolumnar.sort_status('pr_$arm');")" + A="$(raw "SELECT appended_rows FROM pgcolumnar.sort_status('pr_$arm');")" + echo " $arm: sorted $S, appended $A" + check "$arm: a full recluster claims every row" "$S" "200000" + check "$arm: a full recluster leaves nothing appended" "$A" "0" +done + # ---------------------------------------------------- the uncontended case # With no concurrent writer the rewrite orders everything, so the run covers the