fix: Avoid retaining buffer for latest parse in reader#3533
Merged
Conversation
The buffer can be arbitrarily large, and the parser shouldn’t keep it around while waiting on (and potentially also buffering) the next complete packet.
charmander
force-pushed
the
parser-reader-cleanup
branch
from
August 22, 2025 02:44
3206c54 to
715c9dc
Compare
Owner
|
commin' in a bit late here, but appreicate it! |
stephenh
added a commit
to stephenh/node-postgres
that referenced
this pull request
Jul 26, 2026
## Summary Currently, `parseDataRowMessage` eagerly materializes a JS string for every non-null cell of every row at parse time. For a consumer that reads every cell that is exactly the right amount of work; for a consumer that reads some of them, or none of them, it is pure overhead — and on large result sets it is the dominant cost of a query. This PR makes `DataRowMessage` carry the raw payload range (`bytes` + `offset`) and decode cells lazily in a memoized `fields` getter. Existing consumers see identical values from an identical `fields` array; consumers that want to decode on their own terms can now read the payload directly instead of paying for 40 strings per row to use two of them. Two parts: 1. **`DataRowMessage` decodes lazily.** `fields` becomes a memoized getter over `bytes`/`offset`. 2. **The parser reassembles only the message that straddles a chunk, not the whole chunk.** This is what makes the lazily-decoded views safe to hold past the parse tick, and it also drops a full-chunk `memcpy` per socket read for everyone, including pg itself (that copy was how a straddling message's two halves were made contiguous: every chunk was appended to one scratch buffer and all messages were parsed out of there. Only the straddling message ever needs bytes from two chunks, though, so now only it is reassembled — every other message is already contiguous in the chunk it arrived in, and is parsed where it landed). The second part could be dropped if you'd rather land just the lazy getter, but the two together are what make the payload range useful to a consumer. ## Motivation Measured against a local PostgreSQL 17 (node v26.4.0), `select * from <40 column table>` with 100k rows: server-side execution is ~5–10 ms, but `client.query` takes ~630 ms. Roughly, that splits into ~215 ms of the parser building a string per cell (4M cell strings for this one result), ~50 ms of socket transfer, ~285 ms of work downstream of the parser — pg-types converting those strings into values (`timestamptz` into `Date`, and so on) and `Result.parseRow` building 100k 40-property row objects — and ~80 ms of accumulating the rows. Building the strings is the largest single term, and it is the only one this PR touches. Consumers that don't want those strings already exist: - ORMs that decode lazily on field access. A Joist prototype measures 2.2–2.55× faster on 100k–1M row reads — a 100k-row find that reads one column of a 40 column table goes 378 ms → 148 ms, with GC-traced heap for the held result dropping ~3.4×. - Cursor and stream consumers, which hand rows straight to a caller that may only touch a few columns. - Byte-ish workloads — see brianc#2240, where large cell allocation was enough of a problem to be worth its own fix — and brianc#2093, asking directly how to avoid materializing results. This layer is also demonstrably performance-sensitive and actively maintained: brianc#2151 (parser speed improvements), brianc#2240 (the buffer management this PR reworks), and brianc#3533 (Dec 2025, avoiding retention of the last parse buffer). The change here is in that same vein: less copying, less allocation, no behavior change. ## Why now The whole-chunk copy is not a decision anyone made about straddling messages; it is the residue of a simpler design that was optimized twice without its premise being revisited. - brianc#2155 (Apr 2020), the original TypeScript parser: on any read with a leftover, it allocated a fresh `leftover + chunk` buffer and copied both halves into it. Nothing about that targets straddling messages — contiguity is simply what the message loop wants to walk. - brianc#2240 (Jun 2020): the cost being attacked was that per-read allocation. It kept the copy and removed the allocation, giving us today's single scratch buffer that doubles, is appended into, and is compacted in place when it runs out of room. Later commits in the same PR renamed the fields and extracted `mergeBuffer`; the algorithm was already what it is now. So both rounds optimized _how_ to get a contiguous buffer, not _whether_ everything had to live in one. Under eager decoding that premise costs nothing: once `parse()` returns, every message is plain JS strings, nothing points into the bytes any more, and recycling one buffer forever is strictly the best thing to do. A 64 KiB memcpy also does not stand out next to 4M string allocations in a profile. Deferring the cell decoding is what changes the calculus. The moment a message is a view over the bytes, recycling stops being free, and it becomes worth asking which bytes actually have to move. The answer — one message per read, rather than one chunk per read — was always true; it just never mattered before. ## What changed **`messages.ts`** — `DataRowMessage` stores `bytes: Buffer` and `offset: number` (the payload start, pointing at the `int16` field count), reads `fieldCount` in the constructor, and exposes `fields` as a memoized getter that walks the length-prefixed cells (`int32 len`, then `len` bytes; `len === -1` is SQL NULL), decoding each with `bytes.toString('utf-8', …)` — byte for byte what `BufferReader.string` did. Callers that read `fields` see no behavior change at all: the same array of the same values, built by the same UTF-8 decoding of the same bytes, just on first read rather than at parse time. The only difference visible to them is one property read per row. **`parser.ts`** — the `DataRow` case constructs the message from `(bytes, offset)`, which `handlePacket` already has, so `parseDataRowMessage` goes away entirely. And `mergeBuffer` is replaced: - Before: when handling split messages (very common b/c a 64 KiB read typically has many complete messages/rows — ~89 rows of the 737-byte shape in the benchmark below — and then a fraction of one more, since where messages end has nothing to do with where reads end), on main the _entire_ incoming chunk is copied into a scratch buffer, which grows by doubling and is compacted in place whenever the existing buffer can be reused, overwriting what earlier messages had been parsed out of. Those compacted-over bytes are what a lazily decoded `DataRow` still needs, meaning any "lazy parsing" would be forced to happen immediately/before the scratch buffer was reused. Our observation is that the trailing fragment is the only part of a chunk that needs bytes from later chunks: the parser consumes messages from the front, so everything ahead of that fragment is already complete where it landed. - After: the chunk is always parsed in place, and only the one message per chunk that straddles a read/chunk boundary is reassembled into a buffer of its own, which is handed to that message and never touched again. This sounds like "an extra buffer" and so a higher cost to all consumers (eager or lazy), and it is worth being precise that per read it is one. On main, the scratch buffer is allocated once — the first straddle doubles 64 KiB into 128 KiB, and every top-up after that hits the in-place compaction branch, which allocates nothing — so its cost amortizes to zero across the result, at the price of copying 64 KiB into it every read. Here, each straddling message gets its own buffer, so a 1000-read result makes 1000 of them rather than reusing one. Measured, that is +934 bytes per 64 KiB read: one message's worth, which under 4 KiB comes out of node's buffer pool (a pointer bump, ~11 messages per 8 KiB slab) and is young-generation garbage that dies with the message — for `Result.parseRow`, in the same tick. Total allocated bytes over a whole result therefore crosses main's fixed 128 KiB at ~140 reads (~9 MB of rows) and grows from there: ~1 MB for the 70 MiB benchmark below. In exchange, the 64 KiB copy per read is gone, and so is the retained buffer: between reads the parser now holds at most one message's bytes, rather than 128 KiB for the life of the query. For a consumer that reads `fields` (eager parsing), the added allocation is ~1% of the ~70 KiB of cell strings that same read already allocates — total allocation per read measures 70.0 KiB after vs 71.6 KiB before, and the benchmark below shows no change in GC counts or pause time. The one thing to know is the granularity for consumers that _do_ retain: holding one row keeps its whole ~64 KiB read alive, so a consumer holding a few scattered rows should copy out instead. Because that copy ran on essentially every read, a streaming result spent nearly its whole life in the recycled scratch buffer. Concretely, in the benchmark below the old code copies 64 KiB per read (70 MB in total); the new code copies one ~737-byte row per read. Two smaller consequences worth noting: - Every message — not just `DataRow` — is now a view over memory nothing will rewrite, so `copyData` chunks and `authenticationMD5Password` salts (both `Buffer` slices of the parse buffer today) become safe to retain as well. - Between reads the parser now retains at most one message's bytes. On main it holds the scratch buffer — 128 KiB for 64 KiB reads, more if a burst contains large messages — for as long as a response is streaming, releasing it only on a read that ends exactly on a message boundary, i.e. in practice the read that ends the response (or never, while queries are pipelined back to back). That is the same class of problem brianc#3533 addressed. A straddling message's buffer is sized from its header, but only up to 1 MiB up front (`MAX_EAGER_MESSAGE_LENGTH`); beyond that it grows as bytes actually arrive, so a bogus length in a corrupt stream can't ask for a huge allocation. ## Benchmarks `packages/pg-protocol/src/b-data-row.ts` (new, alongside the existing `b.ts`): 100k `DataRow` frames of 40 text cells = 70.3 MiB, delivered in 1125 × 64 KiB chunks, node v26.4.0, best of 5 runs after a warm-up. | consumer | master | this PR | | ---------------------------------- | ----------------- | ---------------------- | | never reads a cell | 213 ms, 64 gc/run | **2.1 ms**, 2.6 gc/run | | reads `fields` once (what pg does) | 216 ms | **205 ms** | | reads every cell | 218 ms | **210 ms** | So: ~100× less time and ~25× fewer collections for a consumer that decodes on its own, and no regression — a small improvement, from the dropped chunk copy — for pg's own eager path. End to end against a local PostgreSQL 17, `select * from wide` (100k rows × 40 columns, 66 MB), best of 7: | consumer | master | this PR | | ----------------------------------------------- | ------ | ---------- | | `client.query`, rows accumulated as objects | 631 ms | 625 ms | | `row` events, rows dropped | 549 ms | 549 ms | | needs 2 of 40 columns, reading `fields` | 266 ms | 264 ms | | needs 2 of 40 columns, reading `bytes`/`offset` | — | **158 ms** | The last row is the point of the change: same query, same 100k rows, same checksum, 1.7× less wall time because 38 of 40 cells per row are never turned into strings. ## Compatibility - `name`, `length`, `fieldCount` and `fields` behave exactly as before for every existing consumer — pg's `Result.parseRow(msg.fields)` reads the getter once per row, one extra property read, same values. - `fields` memoizes, so repeated reads decode once. - The old `new DataRowMessage(length, fields)` constructor still works (an array second argument takes the pre-decoded path), so anything constructing these directly keeps compiling. It is marked `@deprecated` and can be dropped in a major if you'd rather not carry it. - `fields` is now typed `(string | null)[]` rather than `any[]`. - `bytes`, `offset` and `fieldCount` are public, which is what lets a consumer skip `fields` entirely. ## Tests - `inbound-parser.test.ts`: DataRow cases for multi-byte UTF-8, an empty-string cell (`len === 0`) and a NULL cell (`len === -1`); that `fieldCount` is available without decoding; that `fields` is memoized; that the payload range is readable; and that the legacy constructor still works. - New `chunked parsing` block for the buffer rework: a mixed message stream parsed whole, split at every single byte offset, in fixed-size chunks of 1/2/3/4/5/7/13/64/128 bytes (so headers split, bodies split, several messages per chunk, and messages spanning many chunks), a 3 MiB message delivered in 64 KiB chunks, and a check that messages held until after the whole stream is parsed still decode correctly. That last one fails against the old buffer management (the lazy getter reads recycled bytes and throws) and passes with the new. - One test that compared a whole `dataRow` message with `deepEqual` now asserts its shape explicitly, since `fields` is a getter. - pg-protocol 86 passing; pg 270 unit + 218 integration; pg-cursor 37; pg-query-stream 40; pg-pool 86 — all against PostgreSQL 17 on node v26. ## Follow-up Nothing here depends on it, but the natural next step is to let `fields` be decoded per cell rather than per row: with `bytes`/`offset` public, a consumer can already do that, but pg's own `Result.parseRow` still decodes all 40 cells to build a row object. Making that lazy too (a row proxy, or decoding on first property access) would push the same win into `pool.query` itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The buffer can be arbitrarily large, and the parser shouldn’t keep it around while waiting on (and potentially also buffering) the next complete packet.