Skip to content

Bound the buffered-row walk and make it cancellable (#212) - #231

Merged
jdatcmd merged 1 commit into
mainfrom
fix/212-buffered-fetch-robustness
Jul 29, 2026
Merged

Bound the buffered-row walk and make it cancellable (#212)#231
jdatcmd merged 1 commit into
mainfrom
fix/212-buffered-fetch-robustness

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Jul 29, 2026

Copy link
Copy Markdown
Owner

@ChronicallyJD for review. This carries no performance claim, and the reason
it does not is your arithmetic.

What it changes

ColumnarBufferedRowByNumber located a row in the current chunk group by
decoding and discarding every earlier value, for every column, on every fetch,
purely to advance a cursor:

char *cursor = col->valueStream.data;
for (i = 0; i < posInGroup; i++)
    if (existsBytes[i])
        (void) ColumnarDecodeValue(att, &cursor, target);

It now records where each row's value begins and addresses the row directly:
one O(n) pass per chunk group instead of O(n) on every fetch.

The offsets are built lazily on the first buffered fetch and maintained on
append after that, so a load that never fetches never builds them. That is
deliberate: the insert path is what #155 is about and this must not put a cost
there.

The build pass carries a CHECK_FOR_INTERRUPTS, which is the floor you
identified when SIGTERM did nothing to the two backends spinning here and they
needed SIGKILL. #220 guarded columnar_fetch_row; this path sits below it and
had no check of its own.

Why there is no speedup claim

Your bound is right and I confirmed the constant:
columnar_chunk_group_row_limit = 10000 (columnar_tableam.c:60). One fetch is
at most ~10k decodes and a worst-case full stripe is on the order of seconds, so
the walk cannot be #212's three-day spin. That was uninitialised SnapshotDirty
out-fields making _bt_doinsert wait on a phantom transaction and retry forever,
fixed in #226; each retry ran a fetch, which is why the profile pointed at this
function.

Four fixtures failed to make the walk matter, and each looks like it should
work, so they are in the commit message rather than lost:

fixture why it measured nothing
INSERT of distinct keys _bt_check_unique only fetches on an equal key, so it never fetches at all
ON CONFLICT over a small key space only the distinct keys are inserted, so the buffer stays small
fill then re-probe, one transaction grows linearly, 33/49/72 ms at n=2k/4k/8k
index lookups against unflushed rows identical with and without the change

The fourth is yours: the columnar custom scan services that query vectorized and
never calls columnar_index_fetch_tuple, so the fetch path is not on it.

What it is worth on its own terms

A bounded walk instead of an unbounded one, and a cancellation point on a path
that had none. Reviewed on that basis, not as an optimisation.

Gate

On the issue

I think #212 closes when this lands: the spin is explained and fixed by #226, the
"outlived its postmaster" half is a consequence of a CPU-bound loop never
reaching a latch wait rather than a separate defect, and this removes the last
unbounded thing on the path. Say if you read it differently.

ColumnarBufferedRowByNumber located a row in the current chunk group by decoding
and discarding every earlier value, for every column, on every fetch, purely to
advance a cursor. It now records where each row's value begins and addresses the
row directly.

This is a robustness change and deliberately carries no performance claim.

The walk looked like the cause of #212's three-day spin and is not. It is bounded
by chunk_group_row_limit, which defaults to 10000, so one fetch is at most ~10k
decodes and even a worst-case full stripe is on the order of seconds, not hours.
The spin was uninitialised SnapshotDirty out-fields making _bt_doinsert wait on a
phantom transaction and retry forever, which #226 fixed; each retry ran a fetch,
which is why the profile pointed here. ChronicallyJD did that arithmetic and it
is the reason this commit does not claim a speedup.

Four fixtures failed to make the walk matter, and the reasons are worth keeping
because each looks like it should work:

- an INSERT of distinct keys never fetches, because _bt_check_unique only fetches
  when it finds an equal key;
- ON CONFLICT over a small key space inserts only the distinct keys, so the write
  buffer stays small and the walk stays short;
- filling the buffer then re-probing it in one transaction grows linearly;
- driving index lookups against unflushed rows measures identically with and
  without this change, and ChronicallyJD found why: the columnar custom scan
  services such a query vectorized and never calls columnar_index_fetch_tuple at
  all.

What the change is worth on its own terms: the walk is O(1) per fetch after a
single O(n) pass per chunk group instead of O(n) every time, and the pass is a
cancellation point. That interrupt check is the floor ChronicallyJD identified
when SIGTERM did nothing to two backends spinning here and they needed SIGKILL:
#220 guarded columnar_fetch_row, and this path sits below it with no check of its
own.

The offsets are built lazily on the first buffered fetch and maintained on append
after that, so a load that never fetches never builds them. That matters because
the insert path is what #155 is about and this must not put a cost there.

Verified on PG17.10: unique_conc, differential (201), native_dml, native_index
and fuzz (250) all pass, warnings-clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@ChronicallyJD ChronicallyJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Read the offset bookkeeping for correctness and then built the branch and ran the paths that would break if it were wrong, rather than trusting the reasoning.

The bookkeeping is consistent

buffered_note_offset records valueStream.len before the value is appended, and buffered_build_offsets records cursor - valueStream.data before decoding row i. Same quantity, the byte where row i's value begins, from the two directions. A null row records that offset without advancing, so valOffsets[posInGroup] for a null points at the next value and the existsBytes[posInGroup] guard still returns null -- no null accounting in the lookup, as the comment claims. New chunk groups come from palloc0, so valOffsets starts NULL and the array is genuinely per-group and lazy. The rebuild guard (NULL || len < rowCount) is a belt-and-braces against a group that appended without maintaining, which note on every ColumnarWriteRow should already prevent. Offsets live in the stripe context, freed with the buffers they index.

Verified where it would actually break

Built on pg18b, ran the suites that exercise the buffered-fetch path rather than only the ones that pass:

unique_conc PASSED   row_triggers 14   native_index 9   native_dml 13
differential 201     native_fetch_position 7   native_lazy_slot 10

row_triggers is the one that matters most here: an AFTER INSERT row trigger re-fetches each row as it is inserted, so it builds the offsets on the first fetch and then maintains them across every later insert in the statement -- the exact append-after-build path, with nulls in the fixture -- and it checks the values the trigger sees. If maintenance drifted by a value, that suite reports wrong OLD/NEW. It passes.

The framing is the right one

No performance claim, robustness only: a bounded walk plus the CHECK_FOR_INTERRUPTS this path lacked below columnar_fetch_row. That is the honest outcome, and it is better than the one with a number attached to a fixture none of us could make quadratic. The interrupt check closes the gap the SIGTERM-did-nothing backends showed. Good to merge, and #212 closes cleanly with it since the spin itself is explained and fixed in #226.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants