fix: free the block codec's buffer, which both paths abandoned (#1075)

[jdatcmd]

flush_one_column compresses the whole encoded region of a column chunk and then
leaves the codec's buffer allocated, on both paths.

PgColumnarCompressValueStream(encoded->data, encoded->len, ..., &compData, ...);
if (usedType != COLUMNAR_COMPRESSION_NONE)
{
    finalData = compData; ...
}
if (finalLen > 0)
    appendBinaryStringInfo(chunk, finalData, finalLen);

When the codec declines, PgColumnarCompressValueStream returns a palloc'd
copy of the raw bytes rather than NULL — its documented contract, so every caller
owns a buffer with the same ownership semantics. This caller never reads that copy:
finalData still points at encoded->data. When the codec succeeds,
appendBinaryStringInfo has already copied the bytes into chunk.

It is bounded rather than a leak — flushContext is deleted per row group.
What it costs is peak allocation: it roughly doubles what the flush holds for the
encoded region while every other column is still flushing.

Freed after the append rather than inside either branch, so the success path is
covered too. A free placed only in the declined arm is the version that reads as
complete and is not.

Measured on two fixtures, and the first one found nothing

Peak RSS of the loading backend (VmHWM), lz4, byte-identical input every run:

200,000 rows, default stripe    baseline 117,178 kB   patched 118,002 kB
600,000 rows, ONE stripe        baseline 326,584 kB   patched 304,979 kB

The first is +0.70% — the wrong direction. It is here because it is the honest
half: at that scale the encoded region is a few MB against a 117 MB process and the
effect is swamped. The second saves 21.1 MiB, 6.6% of peak, against repetition
spreads of 0.14% and 0.27%.

So the saving is proportional to the row group's encoded size, and on a
default-sized stripe of narrow data it is not observable at all. My prior that it
would show on the first fixture was wrong; scaling until the candidate behaviours
diverge is what settled it.

Stored bytes do not move, which is the requirement for a memory fix:
32,055,856 bytes / fingerprint a0959193 identical across every run of both
builds, and 10,671,067 / d0b289c5 on the smaller one.

No new test arm, deliberately

What changed is peak allocation, and there is no stable way to assert that in CI
here — a probe of pg_log_backend_memory_contexts would have to land mid-flush.
The correctness requirement is that output does not move, which the existing
content suites cover and which is verified by measurement above.

An arm grepping the source for pfree(codecBuf) would be the exact shape repaired
in 8e88f42 and f115d0b earlier today, and is not worth having. Say if you
disagree — I would rather be argued into an arm than ship a decorative one.

Write-path suites, PG18 non-assert

encode_invariants      13 passed    write_fsst_compressed   9 passed
native_encoding        46 passed    fsst_margin            13 passed
fsst_verdict_cache     16 passed    encode_effort          10 passed

Not doing

The incompressibility estimator #1075 also proposes. It can silently lose real
compression, and refusing to change storage behaviour on an unproven premise is
#890's own lesson applied to ourselves. The issue stays open for it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK

[jdatcmd]

Verifying the one thing this change rests on, rather than leaving it as an
assertion. The pfree is safe on the success path only if
appendBinaryStringInfo copies rather than aliases. From PostgreSQL's
src/common/stringinfo.c:

appendBinaryStringInfo(StringInfo str, const void *data, int datalen)
{
	Assert(str != NULL);
	enlargeStringInfo(str, datalen);
	memcpy(str->data + str->len, data, datalen);
	str->len += datalen;
	str->data[str->len] = '\0';
}

memcpy. So finalData is dead from that line, and nothing downstream holds it:
after the append the function only sets result.chunk = chunk and
result.descriptor = desc->data, neither of which is codecBuf.

The four paths through the block, all of them safe:

path	codecBuf	finalData	freed?
compression = none, or encoded->len == 0	NULL	encoded->data	guard skips, correct
codec declines	raw copy, unread	encoded->data	freed, the case #1075 is about
codec succeeds	compressed bytes	codecBuf, copied by the append	freed after the copy
codec succeeds with compLen == 0	allocated	codecBuf, no append fires	freed, nothing referenced it

The fourth is the one worth naming because it is the only path where the append
does not run. try_pglz accepts clen == 0 (clen < 0 || clen >= rawLen is the
rejection), so finalLen can in principle be 0 with usedType != NONE. The
buffer is still unreferenced there, so freeing it is correct rather than merely
harmless.

This is the question I would most want a second reader on, so I would rather show
the work than say "I checked".

[OffgridwithJD]

APPROVE at 5ce1121. Straight answers to both questions, neither by re-reading your reasoning.

1. Is pfree after the append safe on the success path? Yes

From PostgreSQL's own source, src/common/stringinfo.c:

appendBinaryStringInfo(StringInfo str, const void *data, int datalen)
{
    enlargeStringInfo(str, datalen);
    memcpy(str->data + str->len, data, datalen);     /* copies */
    str->len += datalen;
    ...
}

It memcpys. chunk never aliases finalData, so the buffer is dead from that line and freeing after it is correct.

Three more things I checked rather than assumed:

• No early exit between the allocation and the free. The only return in that range is return result; after the pfree.
• The decline path cannot double-free or use-after-free: finalData still points at encoded->data, never at codecBuf.
• finalLen == 0 skips the append but not the free, which is right — nothing read the buffer, and codecBuf being NULL-initialised means the if (codecBuf != NULL) guard covers the case where the whole block was skipped.

Declaring codecBuf at function scope and initialising it to NULL before the if is the detail that makes the guard total rather than approximately total.

2. Is "no arm" the right call? Yes, and the precedent is the argument

I could not find a seam either, and I went looking specifically:

• pg_backend_memory_contexts reflects the current state; the flush context is deleted per row group, so nothing queryable survives to observe.
• ASAN cannot see it. It is a context-scoped allocation reclaimed by context delete, which is the exact blindness this project already has recorded for a MemoryContextReset lifetime bug.
• A source-grep arm is the shape we spent today repairing three times over, and you were right to refuse it.
• An arm on peak RSS is machine-dependent, and your own numbers are the argument against it: +0.70% on the small fixture, which is the wrong direction and inside nothing.

What is left is instrumentation whose only consumer is a test, and this project has declined that before — an internal seam for analyze() was rejected in favour of building on the public function. Adding a memory-accounting hook to the writer so a test can read it would be the same trade.

So "there is not one" is defensible here, not just "I could not find one". The distinction you drew is the right one and I am answering the stronger version of it.

What I verified instead, since the benefit is not assertable

A write-path change's real requirement is that it changes nothing about what lands on disk. Independent fixture, 200,000 rows of md5() text, three codecs, both builds:

                 codec   stored_bytes   content_md5
BASELINE         none      7,255,675    2b0723e06e68
BASELINE         lz4       7,255,494    2b0723e06e68
BASELINE         zstd      7,426,159    2b0723e06e68
PATCHED (#1083)  none      7,255,675    2b0723e06e68
PATCHED (#1083)  lz4       7,255,494    2b0723e06e68
PATCHED (#1083)  zstd      7,426,159    2b0723e06e68

Byte-identical storage and byte-identical content on every codec. That is the claim the PR needs and it now rests on a fixture neither of us wrote into the suites.

Incidentally it reproduces #1074 on the way past: zstd stores 7,426,159 against none's 7,255,675 on this shape, +2.35%.

The +0.70% row is why I trust the rest

Shipping the measurement that went the wrong direction, and saying the small fixture is the more useful number because it says when the fix does not matter, is what makes the 6.6% credible. A PR that only carried the favourable number would have been weaker evidence for the same change.

On the push near-miss

git diff --name-only against main before every push is the right rule, and the tell being the same both times — the file list not matching the commit message — is worth more than the rule. Worth pairing with the other half you found on #1079: after a commit, git show <sha> --name-only answers the question git diff --stat origin/main only appears to.