From 5ad3676bd443bbea0f74b69782d26af4bbef3d8e Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 2 Sep 2026 17:46:16 -0600 Subject: [PATCH 1/2] fix: two silent-correctness defects that block the alpha3 tag (#875, #881) Both make a correct-looking operation produce wrong data with no error. Both were reproduced on main before anything was changed, and each arm goes red when its own fix is reverted. #875 -- A PROJECTION CREATED MID-TRANSACTION MISSED THE WRITES AFTER IT. 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 any projection exists. So a write before add_projection() latched an empty list, the back-fill populated the new projection from the rows that already existed, and every later write in that transaction skipped it in silence. Measured 116 base rows against 105 in the projection. 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: uint64 2^63+5 into bigint -> -9223372036854775803 int64 1,2,3,4 into int -> 0,0,1,2 decimal(10,2) 1.25 into numeric(20,4) -> 0.0125 fixed_size_binary(32) into uuid -> the first 16 bytes The buffer-length check already refused a file whose carrier is NARROWER than the target. These are the same-width-or-wider cases, where the buffer is long enough and nothing complained. Refused now with 42804. Deliberately WITHIN a family: an int64 read into a timestamp as raw microseconds is long-standing accepted behaviour with its own control, and my first attempt broke it. Cross-family behaviour is unchanged. REMOVAL PROOFS, one .so per arm, every mutation asserted applied. #875, in test/projections.sh (71 checks): unmutated 71 + 0 call site removed 69 + 2 function body gutted 69 + 2 #881, in test/arrow_import.sh (72 checks): unmutated 72 + 0 Int check removed 68 + 4 uint64/int64 return 00000 Float check removed 71 + 1 Decimal check removed 71 + 1 FixedSizeBinary check removed 71 + 1 THE FSB ARM EXISTS BECAUSE THE FIRST VERSION OF THIS DID NOT HAVE ONE. Removing that check reddened nothing: the narrowing case an obvious arm would use 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 takes the first half. Also here, both from the same release review: #876 the "does not exist" error now names rebuild_projections(), which recovers it. The declaration is intact and the old message denied it. #877 the CHANGELOG said all three visibility-map clears are held by tests. Two were not until #878. Corrected. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 55 ++++++++++++++++++++- src/columnar.h | 1 + src/columnar_arrow.c | 99 ++++++++++++++++++++++++++++++++++++-- src/columnar_projection.c | 34 +++++++++++-- src/columnar_write_state.c | 32 ++++++++++++ test/arrow_import.sh | 69 ++++++++++++++++++++++++++ test/projections.sh | 79 ++++++++++++++++++++++++++++++ 7 files changed, 361 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f88f82c8..c43f822b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,6 +108,58 @@ true until the next version shipped. ### Fixed +- A projection created mid-transaction receives the writes that follow it + (#875). + + **A write before `pgcolumnar.add_projection()` in the same transaction made + every later write in that transaction skip the new projection.** The rows + landed in the base table and never reached the projection, with no error, and + a covering projection scan then answered as though they had not been inserted. + + BEGIN; + INSERT INTO t ...; -- any write will do + SELECT pgcolumnar.add_projection('t', ...); + INSERT INTO t ...; -- these rows were lost to it + COMMIT; + + Measured: 116 rows in the base table, 105 in the projection. + + `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 the writes that follow rebuild + it from the catalog. + +- An Arrow import reads the width, sign and scale the FILE declares (#881). + + **`imp_apply_field` inspected only `Date`, `Time` and `Timestamp`.** For every + other tag the stride and the interpretation came from the TARGET column, so a + file that declared something else was decoded as though it had not: + + uint64 2^63+5 into bigint -> -9223372036854775803 + int64 1,2,3,4 into int -> 0,0,1,2 + decimal(10,2) 1.25 into numeric(20,4) -> 0.0125 + fixed_size_binary(32) into uuid -> the first 16 bytes + + All four imported without an error. They are refused now with `42804`. + + The buffer-length check already caught the cases where the file's carrier is + NARROWER than the target; these are the ones where it is the same width or + wider, so the buffer is long enough and nothing complained. + + **Scope is within a family.** A tag that does not match the column's family at + all -- an `int64` read into a `timestamp` as raw microseconds -- is + long-standing accepted behaviour with its own test, and is unchanged. + +- `read_projection` explains itself after a rewrite (#876). + + A rewrite mints a new storage id while `pgcolumnar.projection` keeps the old + one, so a projection that is still declared reads as absent. The error said + only `projection "p" does not exist on "t"`, which is not true -- the + declaration is intact. It now carries a hint naming + `pgcolumnar.rebuild_projections()`, which recovers it. The underlying + re-recording is still open as #876. + - `DROP` after `ALTER TABLE ... SET ACCESS METHOD heap` now takes the relid-keyed catalog rows with it, and the hook that does it stays out of the way in databases that have no extension. @@ -169,7 +221,8 @@ true until the next version shipped. index-only scan answers from the index for a row group that is gone. `expire` cleared them; `pgcolumnar.recluster()` and the partial-group rewrite behind `pgcolumnar.compact_rewrite()` did not, and both renumber live rows through - the same retire. All three clear now. The rule is that visibility-map bits go + the same retire. All three clear now, and as of #878 all three are held by + tests; before it, only expire's clear was. The rule is that visibility-map bits go wherever row numbers are reassigned, not only where rows expire. `docs/sql-reference.md` gains the accepted range for `ttl_interval` and the diff --git a/src/columnar.h b/src/columnar.h index 96c440f3..eec6b51a 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -569,6 +569,7 @@ extern void PgColumnarProjectionFanoutRow(Relation rel, PgColumnarWriteState *ba uint64 rowNumber, Datum *values, bool *nulls); extern void PgColumnarFlushWriteStateForRelation(Oid relid); +extern void PgColumnarResetProjectionWritersForRelation(Oid relid); /* ------------------------------------------------------------------------- * delete vector / delete tracking (pgcolumnar_delete_vector.c, spec 7.5, 9) diff --git a/src/columnar_arrow.c b/src/columnar_arrow.c index a6b952aa..4f39b78c 100644 --- a/src/columnar_arrow.c +++ b/src/columnar_arrow.c @@ -1347,6 +1347,7 @@ typedef struct ImpNode Oid typid; int width; /* carrier bytes IN THE FILE, not in PostgreSQL */ int scale; /* decimal scale; not a temporal unit */ + int precision; /* decimal precision; 0 when not a decimal */ int srcUnit; /* Arrow DateUnit/TimeUnit, -1 if the file said nothing */ int32 atttypmod; bool needsInput; @@ -1430,6 +1431,7 @@ imp_build_node(ImpNode *n, Oid typid, int32 typmod, bool *ok) } n->width = width; n->scale = scale; + n->precision = precision; n->needsInput = (n->kind == A_UTF8); if (n->needsInput) { @@ -1555,7 +1557,7 @@ arrow_scale_to_usecs(int unit, int64 v, int64 *out, int64 *nsTrunc) * file cannot drive it deeper than the column's own nesting. */ static void -imp_temporal_mismatch(const char *arrowtype, Oid typid) +imp_field_mismatch(const char *arrowtype, Oid typid) { ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), @@ -1599,7 +1601,7 @@ imp_apply_field(ImpNode *n, const uint8 *meta, uint32 metaLen, uint32 field) { case ARROW_TYPE_Date: if (n->kind != A_DATE32) - imp_temporal_mismatch("a date", n->typid); + imp_field_mismatch("a date", n->typid); pos = pgc_fb_field(meta, metaLen, tt, 0); /* unit (i16) */ n->srcUnit = pos ? fbr_i16(meta, metaLen, pos) : ARROW_DU_MILLI; if (n->srcUnit != ARROW_DU_DAY && n->srcUnit != ARROW_DU_MILLI) @@ -1611,7 +1613,7 @@ imp_apply_field(ImpNode *n, const uint8 *meta, uint32 metaLen, uint32 field) int32 bits; if (n->kind != A_TIME64) - imp_temporal_mismatch("a time", n->typid); + imp_field_mismatch("a time", n->typid); pos = pgc_fb_field(meta, metaLen, tt, 0); /* unit (i16) */ n->srcUnit = pos ? fbr_i16(meta, metaLen, pos) : ARROW_TU_MILLI; pos = pgc_fb_field(meta, metaLen, tt, 1); /* bitWidth (i32) */ @@ -1633,9 +1635,98 @@ imp_apply_field(ImpNode *n, const uint8 *meta, uint32 metaLen, uint32 field) n->width = bits / 8; break; } + case ARROW_TYPE_Int: + { + int32 bits; + bool isSigned; + + /* + * Only when the target is the same FAMILY. A file whose tag + * does not match the column's family at all -- an int64 read + * into a timestamp as raw microseconds -- is long-standing + * accepted behaviour with its own control in + * test/arrow_import.sh, and #881 is about values corrupted + * WITHIN a family, not about changing that. + */ + if (n->kind != A_INT16 && n->kind != A_INT32 && + n->kind != A_INT64) + break; + pos = pgc_fb_field(meta, metaLen, tt, 0); /* bitWidth */ + bits = pos ? fbr_i32(meta, metaLen, pos) : 0; + pos = pgc_fb_field(meta, metaLen, tt, 1); /* is_signed */ + isSigned = pos ? (fbr_u8(meta, metaLen, pos) != 0) : false; + + /* + * The stride comes from the TARGET column, so a file whose + * carrier is a different width is read at the wrong offsets: + * int64 into int returned 0,0,1,2 for 1,2,3,4. And every + * PostgreSQL integer is signed, so a uint64 carrying a value + * above 2^63 reads as negative. Both were silent. + */ + if (bits != n->width * 8) + imp_field_mismatch("an integer of a different width", + n->typid); + if (!isSigned) + imp_field_mismatch("an unsigned integer", n->typid); + break; + } + case ARROW_TYPE_FloatingPoint: + { + int16 prec; + + if (n->kind != A_FLOAT32 && n->kind != A_FLOAT64) + break; /* cross-family: see the Int case */ + pos = pgc_fb_field(meta, metaLen, tt, 0); /* precision */ + prec = pos ? fbr_i16(meta, metaLen, pos) : 0; + /* 0 HALF, 1 SINGLE, 2 DOUBLE; HALF has no PostgreSQL type */ + if (prec != ((n->kind == A_FLOAT32) ? 1 : 2)) + imp_field_mismatch("a float of a different width", + n->typid); + break; + } + case ARROW_TYPE_FixedSizeBinary: + { + int32 bw; + + if (n->kind != A_UUID) + break; /* cross-family: see the Int case */ + pos = pgc_fb_field(meta, metaLen, tt, 0); /* byteWidth */ + bw = pos ? fbr_i32(meta, metaLen, pos) : 0; + if (bw != 16) + imp_field_mismatch("a fixed-size binary of another width", + n->typid); + break; + } + case ARROW_TYPE_Decimal: + { + int32 fprec; + int32 fscale; + + if (n->kind != A_DECIMAL128) + break; /* cross-family: see the Int case */ + pos = pgc_fb_field(meta, metaLen, tt, 0); /* precision */ + fprec = pos ? fbr_i32(meta, metaLen, pos) : 0; + pos = pgc_fb_field(meta, metaLen, tt, 1); /* scale */ + fscale = pos ? fbr_i32(meta, metaLen, pos) : 0; + + /* + * The unscaled integer is read as-is and the TARGET's scale + * is then applied, so a file at another scale is wrong by a + * power of ten: decimal128(10,2) into numeric(20,4) stored + * 0.0125 for 1.25. Precision is checked too, because a file + * that can hold more digits than the column can is not + * representable even when the scales agree. + */ + if (fscale != n->scale) + imp_field_mismatch("a decimal at another scale", n->typid); + if (n->precision > 0 && fprec > n->precision) + imp_field_mismatch("a decimal of greater precision", + n->typid); + break; + } case ARROW_TYPE_Timestamp: if (n->kind != A_TIMESTAMP && n->kind != A_TIMESTAMPTZ) - imp_temporal_mismatch("a timestamp", n->typid); + imp_field_mismatch("a timestamp", n->typid); pos = pgc_fb_field(meta, metaLen, tt, 0); /* unit (i16) */ n->srcUnit = pos ? fbr_i16(meta, metaLen, pos) : ARROW_TU_SECOND; if (n->srcUnit < ARROW_TU_SECOND || n->srcUnit > ARROW_TU_NANO) diff --git a/src/columnar_projection.c b/src/columnar_projection.c index 6051ec80..2c890fbe 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -264,6 +264,19 @@ pgcolumnar_add_projection(PG_FUNCTION_ARGS) sortArr ? sortArr : construct_empty_array(TEXTOID)); + /* + * An open write state on this relation cached its projection-writer list on + * its first row and latched it, including when the list was empty because no + * projection existed yet. The projection set has just changed, so drop that + * cache: without this, every later write in the same transaction skips the + * projection just created and does so silently (#875). + * + * After the back-fill, not before. The back-fill populates the projection + * from the rows that already exist; resetting first would change what it + * sees rather than what follows it. + */ + PgColumnarResetProjectionWritersForRelation(relid); + table_close(rel, ShareLock); PG_RETURN_VOID(); } @@ -328,7 +341,12 @@ pgcolumnar_drop_projection(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("projection \"%s\" does not exist on \"%s\"", - projname, get_rel_name(relid)))); + projname, get_rel_name(relid)), + errhint("A rewrite -- TRUNCATE, vacuum, recluster -- mints a new " + "storage id while the projection rows keep the old one, " + "so a projection that is still declared can read as " + "absent (#876). pgcolumnar.rebuild_projections() " + "re-records them."))); if (targetId == 0) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), @@ -453,7 +471,12 @@ pgcolumnar_read_projection(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("projection \"%s\" does not exist on \"%s\"", - projname, get_rel_name(relid)))); + projname, get_rel_name(relid)), + errhint("A rewrite -- TRUNCATE, vacuum, recluster -- mints a new " + "storage id while the projection rows keep the old one, " + "so a projection that is still declared can read as " + "absent (#876). pgcolumnar.rebuild_projections() " + "re-records them."))); ncols = proj->columnsLen; @@ -639,7 +662,12 @@ pgcolumnar_reconstruct_via_projection(PG_FUNCTION_ARGS) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_OBJECT), errmsg("projection \"%s\" does not exist on \"%s\"", - projname, get_rel_name(relid)))); + projname, get_rel_name(relid)), + errhint("A rewrite -- TRUNCATE, vacuum, recluster -- mints a new " + "storage id while the projection rows keep the old one, " + "so a projection that is still declared can read as " + "absent (#876). pgcolumnar.rebuild_projections() " + "re-records them."))); ncols = proj->columnsLen; diff --git a/src/columnar_write_state.c b/src/columnar_write_state.c index 097c6b7e..4a9e9228 100644 --- a/src/columnar_write_state.c +++ b/src/columnar_write_state.c @@ -3349,6 +3349,38 @@ flush_ws_projections(PgColumnarWriteState *ws) table_close(rel, RowExclusiveLock); } +/* + * PgColumnarResetProjectionWritersForRelation + * Drop the cached projection-writer list for `relid` so the next write + * rebuilds it from the catalog. + * + * PgColumnarProjectionFanoutRow builds that list on first use and latches it, + * including when it comes back EMPTY. That is correct while the projection set + * cannot change under an open write state, and add_projection() is exactly the + * operation that changes it: a write before it latches an empty list, the + * back-fill then populates the new projection from the rows that already exist, + * and every later write in the same transaction skips it silently (#875). + * + * Buffered projection rows are flushed before the list is dropped, or they would + * be lost with the writers that hold them. + */ +void +PgColumnarResetProjectionWritersForRelation(Oid relid) +{ + ListCell *lc; + + foreach(lc, PgColumnarWriteStates) + { + PgColumnarWriteState *writeState = (PgColumnarWriteState *) lfirst(lc); + + if (writeState->relid != relid) + continue; + flush_ws_projections(writeState); + writeState->projWriters = NIL; + writeState->projInited = false; + } +} + /* * PgColumnarFlushWriteStateForRelation * Flush any pending partial stripe for a single relation. Used at scan diff --git a/test/arrow_import.sh b/test/arrow_import.sh index 2ca09070..afee56bf 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -306,6 +306,29 @@ cases = { # a temporal carrier whose tag no other temporal target can hold 'tu_t64_plain': (pa.time64('us'), NOON * 10**6), } +# #881 fixtures. These go through pa.array rather than the raw-byte packing +# below, because that builder derives its struct format from bit_width and can +# express neither a uint64 above 2^63, nor a float, nor a 128-bit decimal. +import decimal +extra = { + 'tu_u64': pa.array([2**63 + 5], pa.uint64()), + 'tu_i32': pa.array([7], pa.int32()), + 'tu_i64': pa.array([7], pa.int64()), + 'tu_f32': pa.array([1.5], pa.float32()), + 'tu_f64': pa.array([1.5], pa.float64()), + 'tu_d102': pa.array([decimal.Decimal('1.25')], pa.decimal128(10, 2)), + 'tu_d204': pa.array([decimal.Decimal('1.25')], pa.decimal128(20, 4)), + # A WIDER fixed-size binary is the case the buffer-length check cannot see: + # 32 bytes per row is more than uuid's 16, so the buffer is long enough and + # the reader would take the first half of each value. + 'tu_fsb32': pa.array([b'0123456789abcdef0123456789abcdef'], pa.binary(32)), + 'tu_fsb16': pa.array([b'0123456789abcdef'], pa.binary(16)), +} +for name, arr in extra.items(): + tab = pa.table({'v': arr}) + with ipc.new_stream(pa.OSFile(os.path.join(out, name + '.arrows'), 'wb'), tab.schema) as w: + w.write_table(tab) + for name, (typ, v) in cases.items(): raw = struct.pack(' -9223372036854775803 + # int64 1,2,3,4 into int -> 0,0,1,2 + # decimal(10,2) 1.25 into numeric(20,4) -> 0.0125 + # + # Every one imported without an error. They are refused now. The controls + # below are the matching files, which must still import, or the refusal is + # just "no Arrow file works". + check "a uint64 file is refused for bigint (#881)" \ + "$(tu_import_state tu_u64 bigint)" "42804" + check "an int64 file is refused for a 4-byte int (#881)" \ + "$(tu_import_state tu_i64 int)" "42804" + check "an int32 file is refused for bigint (#881)" \ + "$(tu_import_state tu_i32 bigint)" "42804" + check "a float32 file is refused for float8 (#881)" \ + "$(tu_import_state tu_f32 float8)" "42804" + check "control: a matching int64 file imports into bigint" \ + "$(tu_import_state tu_i64 bigint)" "00000" + check "control: a matching int32 file imports into int" \ + "$(tu_import_state tu_i32 int)" "00000" + check "control: a matching float64 file imports into float8" \ + "$(tu_import_state tu_f64 float8)" "00000" + check "control: a matching float32 file imports into float4" \ + "$(tu_import_state tu_f32 float4)" "00000" + # A NARROWER carrier is already caught by the buffer-length check, so it + # cannot tell this guard from its absence. A WIDER one can: the buffer is + # long enough and the reader would silently take the first 16 bytes. + check "a wider fixed-size binary is refused for uuid (#881)" \ + "$(tu_import_state tu_fsb32 uuid)" "42804" + check "control: a 16-byte fixed-size binary imports into uuid" \ + "$(tu_import_state tu_fsb16 uuid)" "00000" + check "a decimal at another scale is refused (#881)" \ + "$(tu_import_state tu_d102 'numeric(20,4)')" "42804" + check "control: a matching decimal imports" \ + "$(tu_import_state tu_d204 'numeric(20,4)')" "00000" + + # A refusal must leave nothing behind: an arm that only reads the SQLSTATE + # would pass for a fix that errored after writing the row. + tu_import_state tu_u64 bigint >/dev/null + check "and a refused import leaves the target empty" \ + "$(q "SELECT count(*) FROM tu_t;")" "0" + # A NON-temporal tag is still left alone: an int64 file keeps importing into # a timestamp column as raw microseconds, exactly as it did before. check "control: a non-temporal tag is not caught by the temporal refusal" \ diff --git a/test/projections.sh b/test/projections.sh index 90e095e6..badfaf62 100755 --- a/test/projections.sh +++ b/test/projections.sh @@ -326,4 +326,83 @@ check "and it removed the orphan" \ "$(q "SELECT count(*) FROM pgcolumnar.projection_declaration WHERE name = 'ghost';")" "0" psql_run "DROP TABLE IF EXISTS od2;" >/dev/null 2>&1 +# --------------------------------------------------------------------------- +# A projection created mid-transaction must receive the writes that follow it +# (#875). +# +# PgColumnarProjectionFanoutRow builds the write state's projection-writer list +# on first use and latches it -- INCLUDING when the list comes back empty. So a +# write before add_projection() latches an empty list, add_projection() then +# back-fills the rows that already existed, and every later write in that +# transaction skips the projection with no error. The rows are in the base table +# and absent from the projection, and a covering projection scan answers as if +# they were never inserted. +# +# The leading write is the whole trigger, so the control is the same transaction +# without it: that path already worked, and an arm that only ran the broken +# shape could not tell a fix from a change that broke both. +# --------------------------------------------------------------------------- +echo "-- a projection added mid-transaction receives later writes (#875)" + +# This suite had no SQLSTATE helper; same form as test/export_sink.sh's. Assert +# the code, not the message: "does not exist" is also what a typo in the table +# name produces. +proj_sqlstate() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -qtA 2>&1 < Date: Wed, 2 Sep 2026 17:58:16 -0600 Subject: [PATCH 2/2] fix: drop_projection has the same latched cache, in the other direction (#875) Found in review of this PR, on the sibling function. drop_projection deletes the projection row, its metadata and its declaration, and left the write state's cached writer pointing at what it had just removed. Writes after it in the same transaction kept appending: the rows land in a projection storage whose catalog rows are gone, and the transaction commits with an orphan. Reproduced before fixing, and the control is what identifies the cause: drop in its OWN transaction 0 orphan storage ids drop MID-transaction 1 orphan storage id Same fix, already in this branch and callable. MY FIRST ATTEMPT TO REPRODUCE THIS SAID "CANNOT REPRODUCE", and the reason is worth the comment it now carries in the suite. My orphan query excluded row-group storage ids present in pgcolumnar.storage -- but a projection's storage is registered there too, so the filter hid the exact row the arm exists to find. Two fixtures, both 0, before the catalog dump showed the row sitting there. A reviewer's finding I could not reproduce turned out to be my instrument. The count is also scoped to ONE relation. A database-wide count is not independent: the first arm's orphan is still present when the control runs, so the control failed for the previous arm's reason. That version read as a stronger removal proof than it was -- two arms red instead of one. Removal proof, one .so per arm, mutation asserted applied: unmutated 73 + 0 drop_projection's reset removed 72 + 1 the mid-transaction arm only The own-transaction control stays green under the mutation, which is what pins this to the cache rather than to drop_projection's own cleanup. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 6 ++++++ src/columnar_projection.c | 11 +++++++++++ test/projections.sh | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c43f822b..ea78dd23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,6 +130,12 @@ true until the next version shipped. now drops that cache, after the back-fill, so the writes that follow rebuild it from the catalog. + **`drop_projection` had the same defect in the other direction.** A writer + cached before the drop kept taking rows, which landed in a projection storage + whose catalog rows were already deleted and committed as an orphan: one orphan + storage id when the drop happened mid-transaction, none when it had the + transaction to itself. It drops the cache too. + - An Arrow import reads the width, sign and scale the FILE declares (#881). **`imp_apply_field` inspected only `Date`, `Time` and `Timestamp`.** For every diff --git a/src/columnar_projection.c b/src/columnar_projection.c index 2c890fbe..67e55f3a 100644 --- a/src/columnar_projection.c +++ b/src/columnar_projection.c @@ -368,6 +368,17 @@ pgcolumnar_drop_projection(PG_FUNCTION_ARGS) /* and forget the declaration, so a later rebuild does not resurrect it (#266) */ PgColumnarDeleteProjectionDeclaration(relid, projname); + /* + * The same latched cache as add_projection's, in the other direction. A + * write earlier in this transaction cached a writer for the projection just + * deleted, and without this the writes that follow keep appending to it: the + * rows land in a projection storage whose catalog rows are already gone, and + * the transaction commits with an orphan. Measured 1 orphan storage id when + * the drop happens mid-transaction, 0 when it has the transaction to itself, + * which is what pins it to the cache rather than to the deletes above. + */ + PgColumnarResetProjectionWritersForRelation(relid); + table_close(rel, ShareUpdateExclusiveLock); PG_RETURN_VOID(); } diff --git a/test/projections.sh b/test/projections.sh index badfaf62..21518cb2 100755 --- a/test/projections.sh +++ b/test/projections.sh @@ -405,4 +405,36 @@ check "and the base table still took every row" \ check "a projection dropped mid-transaction is gone, not still being written" \ "$(proj_sqlstate "SELECT pgcolumnar.read_projection('dt','dp')")" "42704" +# The arm above only proves the DECLARATION went. The writes are the other half: +# a writer cached before the drop keeps taking rows, which land in a projection +# storage whose catalog rows are already deleted and commit as an orphan. Count +# row-group storage ids that no projection row names. +# +# Do NOT also exclude ids present in pgcolumnar.storage: a projection's storage +# is registered there too, so that filter hides exactly the row this arm is for. +# It cost me a "cannot reproduce" before the control below showed otherwise. +# Scoped to ONE relation. A database-wide count is not independent: the first +# arm's orphan is still there when the control runs, so the control would fail +# for the previous arm's reason and read as if it had caught its own. +proj_orphans() { + q "SELECT count(*) FROM (SELECT DISTINCT rg.storage_id + FROM pgcolumnar.row_group rg + JOIN pgcolumnar.storage s ON s.storage_id = rg.storage_id + WHERE s.relation_oid = '$1'::regclass) x + WHERE NOT EXISTS (SELECT 1 FROM pgcolumnar.projection p + WHERE p.proj_storage_id = x.storage_id);" +} +check "a mid-transaction drop leaves no orphan projection storage" \ + "$(proj_orphans dt)" "0" + +# The control that pins it to the CACHE rather than to drop_projection's own +# cleanup: the same drop with the transaction to itself was always 0. +psql_run "CREATE TABLE dt2 (a int, c int) USING pgcolumnar; + SELECT pgcolumnar.add_projection('dt2', 'dp2', ARRAY['a','c'], ARRAY['c']); + INSERT INTO dt2 SELECT g, g FROM generate_series(1, 50) g;" +psql_run "SELECT pgcolumnar.drop_projection('dt2', 'dp2');" +psql_run "INSERT INTO dt2 SELECT g, g FROM generate_series(200, 300) g;" +check "control: a drop in its own transaction never left one" \ + "$(proj_orphans dt2)" "0" + pgc_summary