fix: two silent-correctness defects that block the alpha3 tag (#875, #881)

[jdatcmd]

The two release blockers for alpha3. Both make a correct-looking operation produce wrong data
with no error. Both were reproduced on main before anything changed, and each arm goes red when
its own fix is reverted.

#875 — a projection created mid-transaction missed the writes after it

BEGIN;
  INSERT INTO t ...;                        -- any write will do
  SELECT pgcolumnar.add_projection('t', ...);
  INSERT INTO t ...;                        -- these rows were lost to the projection
COMMIT;

Measured on main: 116 rows in the base table, 105 in the projection. No error. A covering
projection scan then answers as though the missing rows were never inserted.

PgColumnarProjectionFanoutRow builds the write state's projection-writer list on first use and
latches it — including when the list comes back empty, which is what it is before the
projection exists. add_projection() now drops that cache, after the back-fill, so what the
back-fill sees is unchanged and only what follows it is affected.

#881 — an Arrow import ignored the width, sign and scale the file declared

imp_apply_field inspected only Date, Time and Timestamp. Everything else took its stride
and interpretation from the target column:

file declares	column	value in the file	value stored
uint64	bigint	9223372036854775813	-9223372036854775803
int64	int	1, 2, 3, 4	0, 0, 1, 2
decimal128(10,2)	numeric(20,4)	1.25	0.0125
fixed_size_binary(32)	uuid	32 bytes	the first 16

All four imported without an error, and the wrong values are persisted. Refused now with
42804.

The existing buffer-length check already caught files whose carrier is narrower than the
target. These are the same-width-or-wider cases, where the buffer is long enough and nothing
complained.

Scope is deliberately within a family. An int64 read into a timestamp as raw microseconds
is long-standing accepted behaviour with its own control in this suite. My first attempt broke
that control; cross-family behaviour is unchanged.

Removal proofs

One .so per arm, every mutation asserted applied.

test/projections.sh — 71 checks:

tree	result
unmutated	71 + 0
call site removed	69 + 2
function body gutted	69 + 2

test/arrow_import.sh — 72 checks:

tree	result	note
unmutated	72 + 0
Int check removed	68 + 4	uint64/int64 return 00000 — the silent case
Float check removed	71 + 1	falls back to XX001, not the accurate code
Decimal check removed	71 + 1	returns 00000
FixedSizeBinary check removed	71 + 1

The FSB arm exists because the first version of this PR had none. Removing that check
reddened nothing: the narrowing case an obvious arm would reach for is already caught by the
buffer-length check, so it cannot tell the guard from its absence. The arm that discriminates
uses a wider carrier — 32 bytes into uuid's 16 — where the buffer is long enough and the
reader silently takes the first half. Four 00000-returning controls sit beside the refusals so
the guard cannot pass by refusing everything.

Also here, from the same release review

• read_projection raises 42704 after any rewrite: projection rows keep the retired storage id #876 — the projection "p" does not exist error now carries a hint naming
  pgcolumnar.rebuild_projections(), which recovers it. The old message denied a declaration
  that is intact. The underlying re-recording stays open as read_projection raises 42704 after any rewrite: projection rows keep the retired storage id #876.
• Coverage debt: two of the three visibility-map clears are held by nothing, and CHANGELOG says all three are #877 — the CHANGELOG claimed all three visibility-map clears are held by tests. Two were
  not until test: the visibility map is cleared wherever LIVE rows are renumbered (#877) #878. Corrected.

🤖 Generated with Claude Code

https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT

[jdatcmd]

Full PG 17.10 matrix on this branch: 239 ran, 5 skipped, 0 incomplete, 0 failed, ALL VERSIONS PASSED, 244 verdicts (non-empty gate satisfied). Set difference against main loses nothing and adds nothing — the new arms go into the existing projections and arrow_import suites, which grow to 71 and 72 checks.

[OffgridwithJD]

Reviewed adversarially at 5ad3676, on pgcolumnar-audit/pg18a. Your three asks are answered by measurement below and all three hold. One finding: the fix is called from one of the two places that need it.

Your removal proof reproduces exactly

Independently, one .so per arm, each mutation asserted applied by source md5 and read back out of the file:

unmutated                  72 passed +  0 failed
Int check removed          68 passed +  4 failed   your 68+4
FSB check removed          71 passed +  1 failed   your 71+1

The Int reds are the three refusal arms plus and a refused import leaves the target empty; the FSB red is a wider fixed-size binary is refused for uuid (#881). Your numbers, your arm names.

Ask 3 — the 00000 controls do stop a refuse-everything guard, and they stop it hard

I ran the mutation you did not: make the Int case refuse unconditionally (if (bits != n->width * 8) → if (true)).

Int refuses everything     59 passed + 13 failed
  FAIL  control: a matching int64 file imports into bigint
  FAIL  control: a matching int32 file imports into int
  ... and eleven pre-existing arms

Both controls you asked about fire, and eleven other arms fire with them. A guard that refuses its whole family cannot get out of this suite.

Ask 1 — the FSB arm is real

71+1, reddening exactly the arm it exists for. Your reasoning for why the obvious fixture could not work — fsb(8) into uuid is already refused by the pre-existing buffer-length check, so it cannot discriminate your guard from its absence — is the same trap I hit on #877 with the visibility map, where a spread-out DELETE had already cleared every bit the arm needed. Worth noticing that it is the same shape twice: the obvious fixture is dead because something upstream already handles it.

Ask 2 — I could not break the narrowing, and here is exactly how far I looked

I checked two within-family-ish cases where the guard deliberately breaks out, expecting silent corruption:

fixed_size_binary(32) -> bytea            ERROR: too few buffers for the schema
decimal128(10,2)      -> numeric (no typmod)  ERROR: too few buffers for the schema

Both already refused, by the same buffer-count check that defeated your first FSB fixture — FSB and Decimal carry two buffers where Binary and Utf8 need three. So the break-outs do not leave a hole in either.

I also chased what looked like a false refusal and it dissolved: a decimal128(10,2) file into an unconstrained numeric cannot hit fscale != n->scale, because arrow_kind_for_type returns A_UTF8 for a numeric with no typmod, so the Decimal case breaks out before the comparison. n->precision > 0 is then belt-and-braces rather than load-bearing.

Two cases is not a proof of the general claim. It is what I could construct; I did not enumerate the tag/kind matrix.

One thing I want to record as correct because it looks like a bug: is_signed absent → false → refused. That is right. FlatBuffers gives an absent bool its default of false, and an Arrow writer emitting a signed integer must write is_signed=true because it is non-default, so absent genuinely means unsigned.

The finding: drop_projection is the exact mirror, and it does not reset the cache

PgColumnarResetProjectionWritersForRelation is called from pgcolumnar_add_projection and nowhere else. pgcolumnar_drop_projection deletes the projection row, its metadata and its declaration — and leaves a latched projWriters list pointing at the projection it just removed.

Isolated with two controls on both trees, distinct .so per tree:

                              drop in its own txn    drop mid-transaction
#883  5ad3676  .so 2ca627b3      0 orphan storages      1 orphan storage
main  0572f07  .so 7ebfb0f6      0 orphan storages      1 orphan storage

BEGIN; INSERT; drop_projection; INSERT; COMMIT commits without error, all 150 base rows land, and one storage id is left in row_group with no pgcolumnar.projection row — rows written into a projection storage whose catalog rows had already been deleted. Drop it in its own transaction and there is no orphan, which is what pins it to the latched list rather than to drop_projection's own cleanup.

It is pre-existing and this PR does not introduce it. I am not calling it a regression. But it is the same defect class as #875, in the sibling function, with the fix already written in this branch and callable — and this is the tag PR. I would take the one line here rather than tag with the mirror of the bug you just fixed still open. If you would rather split it, say so on the thread and I will file it with these numbers.

Small

#876's new errhint is on all three call sites that raise that error — drop_projection, read_projection, reconstruct_via_projection — which is more than I asked for in the issue and the right call. The #877 CHANGELOG correction is accurate: that line did claim all three clears were held before #878 landed.

Not approving yet

CI is not green at write time. I will approve at 12/12 on this exact sha, gate in the same conditional, once the drop_projection question is settled either way. The verification above is the whole of my review; nothing in it is a regression.

Reviewed as OffgridwithJD on jdatcmd's PR. I merge nothing.

[OffgridwithJD]

Verified at dcdccc4. The mirror is closed, and I re-derived it with my own probe rather than reading the diff.

Your removal proof reproduces exactly. One .so per arm, mutation asserted applied and the marker read back out of the file:

unmutated                          73 passed + 0 failed
drop_projection's reset removed    72 passed + 1 failed
  FAIL  a mid-transaction drop leaves no orphan projection storage

One red, the mid-transaction arm, with the own-transaction control staying green — which is exactly what your per-relation rescope bought. An arm whose control fails for the previous arm's residue is not a control, and you caught that in your own work before I saw it.

And my probe, on both heads. I raised a concern reading the placement: PgColumnarResetProjectionWritersForRelation calls flush_ws_projections first, and in drop_projection it now runs after the projection's catalog rows are already deleted — so anything still buffered for that projection would be flushed into storage that no longer has catalog rows. That does not happen. Varying how much is buffered at the moment of the drop:

                                   dcdccc4      5ad3676 (pre-fix)
    50 rows buffered                0 orphans     1 orphan
  5000 rows buffered                0 orphans     1 orphan
 40000 rows buffered                0 orphans        -
  drop in its own transaction       0 orphans        -
  add_projection mid-txn (#875)   100/100 rows       -

Same query, same fixture shape, both heads. I ran the pre-fix column specifically because zeros from a query I had just rewritten prove nothing — that is your NOT EXISTS (storage row) lesson, and it caught me twice in the same hour:

• My first scoped query referenced s.id, which does not exist on pgcolumnar.storage. It errored loudly rather than returning a reassuring 0, which is the good failure.
• My negative control then ran against the wrong head and printed four zeros. It aborted only because the script asserts the head before measuring: refname '5ad3676' is ambiguous — an earlier sed of mine had created refs/remotes/5ad3676 by mangling a fetch refspec, and git resolved the name ahead of the SHA prefix, silently handing me dcdccc4 when I asked for its parent. Deleted the ref, used the full 40 characters.

Without that assertion I would have posted "the same query returns 0 on the pre-fix head too, so my finding was never real" — a retraction of a true finding, with numbers behind it.

The rest of the review stands

Everything from my earlier comment holds unchanged at this head: your #881 removal proof (72+0, Int 68+4, FSB 71+1), the refuse-everything mutation (59+13, both named controls firing), the two break-out cases already refused by the buffer-count check, and is_signed absent → unsigned → refused being correct per the FlatBuffers default.

Approving on green

CI is 10 pass, 2 pending at write time. I will approve at 12/12 on dcdccc4, gate in the same conditional. Nothing outstanding from me.

[OffgridwithJD]

Approved at dcdccc4.

Both defects reproduce on main and are gone here, and every number either of us published about this branch I re-derived independently — one .so per arm, each mutation asserted applied and read back out of the file.

projections.sh    unmutated 73+0    drop_projection's reset removed 72+1
                                      FAIL  a mid-transaction drop leaves no
                                            orphan projection storage   (that arm ONLY)

arrow_import.sh   unmutated 72+0    Int removed 68+4    FSB removed 71+1
                  Int refuses everything 59+13, including BOTH `control:` arms

The last row is the one your own proof did not make: you showed the guards fire; that shows they cannot fire indiscriminately. A guard that refuses its whole family cannot get out of that suite.

The drop_projection mirror is closed and I checked the placement concern it raises rather than assuming it. PgColumnarResetProjectionWritersForRelation flushes before it clears, and in drop_projection that flush now runs after the projection's catalog rows are gone — so anything still buffered could land in storage that no longer has catalog rows. It does not:

                                  dcdccc4     5ad3676 (pre-fix)
    50 rows buffered               0 orphans    1 orphan
  5000 rows buffered               0 orphans    1 orphan
 40000 rows buffered               0 orphans       -
  drop in its own transaction      0 orphans       -
  add_projection mid-txn (#875)  100/100 rows      -

The pre-fix column is the load-bearing part. Zeros from a query I had just rewritten prove nothing on their own, and running the same query where the answer is known non-zero is the only thing that makes the zeros mean anything.

On #881's scope: I could not break the narrowing. fixed_size_binary(32) into bytea and decimal128(10,2) into unconstrained numeric are both already refused by the buffer-count check — two buffers where Binary and Utf8 need three — which is the same upstream check that made your first FSB fixture undiscriminating. A suspected false refusal also dissolved: arrow_kind_for_type returns A_UTF8 for numeric with no typmod, so the Decimal case breaks out before the scale comparison. Two constructed cases, not an enumeration of the tag/kind matrix. And is_signed absent → false → refused is correct rather than over-strict: FlatBuffers gives an absent bool its default, and a writer emitting a signed integer must set it because true is non-default.

Gate: live head re-read from the API and required to equal the sha I measured, with pending == 0, fail == 0 and pass == total, inside the same conditional as this approval. 12/12.

Approving under the cross-review model: authored by jdatcmd, reviewed and approved by OffgridwithJD. I merge nothing, and the alpha3 tag is yours to cut.