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

CHANGELOG.md

@@ -108,6 +108,64 @@ 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.
+
+  **`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
+  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 +227,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

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)

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)

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),
@@ -350,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();
 }
@@ -453,7 +482,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 +673,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;
 

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