fix: four core access-method bugs the missing-test audit found, each red first (#1341)

CHANGELOG.md

@@ -409,6 +409,31 @@ true until the next version shipped.
   only for a role the scan would admit, and gives everyone else the flat estimate, as the
   Parquet wrapper has since #330. `test/fdw_plan_privilege.sh` and
   `test_fdw_plan_privilege.py` pin both wrappers' gates.
+- **A plpgsql EXCEPTION block that rolls back no longer loses the enclosing statement's
+  rows (#1341).** A statement ending inside the block flushed every buffered write,
+  including the enclosing INSERT's or DELETE's, under the block's subtransaction; the
+  rollback then took them away. `INSERT INTO t SELECT f(g) FROM generate_series(1,
+  5000) g`, where `f`'s block wrote a columnar table and raised, kept 1 row. Now a
+  statement's end flushes only its own subtransaction's buffers. The same loss through a
+  READ of that table inside the block is #1360, not yet fixed.
+- **A unique index is no longer violated by an insert racing an uncommitted delete of
+  its key (#1341).** The index fetch answered a row whose delete was still in progress
+  as gone, so the uniqueness check did not wait; when the delete rolled back, the key had
+  two rows. It now reports the row present, as heap does, and the check waits.
+- **An index built after a DELETE serves a transaction whose snapshot began before it
+  (#1341).** The build left deleted rows out, so that transaction's index scan missed a
+  row its table scan returned. Deleted rows are now indexed as not alive, as heap indexes
+  a recently dead tuple; a UNIQUE index still builds over a key whose other holder was
+  deleted.
+- **`INSERT ... ON CONFLICT DO NOTHING` works on a columnar table with a unique index
+  (#1341).** It raised `0A000` ("speculative insert is not supported yet") on any table
+  with a primary key or unique index, which is every table it is used on.
+  `docs/limitations.md` said it worked. Speculative insertion now inserts the row as an
+  ordinary row, and withdraws it again (a delete mark) when a concurrent inserter won the
+  key. `ON CONFLICT DO UPDATE` still refuses when a row conflicts, because updating it
+  takes a row lock; with no conflict it inserts. Tested in `am_contracts`, in both
+  twins, including eight sessions racing on the same keys: 0 duplicate keys, against
+  407 to 507 with the withdrawal disabled.
 - Changing a text column's collation without a rewrite no longer gives wrong
   answers (#1327). `ALTER COLUMN ... TYPE text COLLATE <other>` keeps the
   table's files, so the zone maps kept minima and maxima ordered under the old

docs/limitations.md

@@ -652,8 +652,10 @@ other than where the feature is used:
   partitions. Drop the constraint first if the table must be
   columnar. A conversion of the referencing side is permitted. A conversion of a
   table that no foreign key refers to is also permitted.
-- `INSERT ... ON CONFLICT DO UPDATE` takes a row lock and raises the same error.
-  `ON CONFLICT DO NOTHING` does not, and works.
+- `INSERT ... ON CONFLICT DO UPDATE` raises the same error when a conflicting row
+  exists, because updating it takes a row lock. When no row conflicts, it inserts.
+  `ON CONFLICT DO NOTHING` takes no row lock and works, with or without a conflict
+  target.
 
 `CREATE TABLE` refuses an unlogged columnar table for the same reason. Both cases
 follow one rule. This access method refuses a configuration that it cannot

src/columnar_delete_vector.c

@@ -439,6 +439,25 @@ PgColumnarFlushAllDeleteVectors(void)
 		delete_vector_flush_buffer((DeleteVectorBuffer *) lfirst(lc));
 }
 
+/*
+ * PgColumnarFlushDeleteVectorsOfSubXact
+ *		Flush only the delete buffers the given subtransaction owns, for the reason
+ *		PgColumnarFlushPendingWritesOfSubXact gives (#1341).
+ */
+void
+PgColumnarFlushDeleteVectorsOfSubXact(SubTransactionId subid)
+{
+	ListCell   *lc;
+
+	foreach(lc, PgColumnarDeleteVectorBuffers)
+	{
+		DeleteVectorBuffer *buf = (DeleteVectorBuffer *) lfirst(lc);
+
+		if (buf->subid == subid)
+			delete_vector_flush_buffer(buf);
+	}
+}
+
 /*
  * PgColumnarDiscardAllDeleteVectors
  *		Forget all pending delete buffers (transaction end).

src/columnar_delete_vector.h

@@ -24,6 +24,7 @@ extern void PgColumnarMarkRowDeleted(Relation rel, uint64 rowNumber);
 extern bool PgColumnarDeleteVectorBufferedDeleted(Relation rel, uint64 rowNumber);
 
 extern void PgColumnarFlushAllDeleteVectors(void);
+extern void PgColumnarFlushDeleteVectorsOfSubXact(SubTransactionId subid);
 
 extern void PgColumnarDiscardAllDeleteVectors(void);
 

src/columnar_reader.c

@@ -292,6 +292,9 @@ struct PgColumnarReadState
 	 */
 	char	   *nativeDeleteMask;
 	uint32		nativeDeleteMaskLen;
+	/* return deleted rows too, flagged; the index build asks for this (#1341) */
+	bool		emitDeleted;
+	bool		lastRowDeleted;
 
 	MemoryContext readContext;		/* whole scan */
 	MemoryContext stripeContext;	/* reset per stripe */
@@ -3453,13 +3456,35 @@ pgcolumnar_native_next_row(PgColumnarReadState *rs, Datum *values, bool *nulls,
 		*rowNumber = rs->nativeGroup->firstRowNumber + rs->rowInGroup;
 		rs->rowInGroup++;
 
-		if (deleted)
+		if (deleted && !rs->emitDeleted)
 			continue;			/* row deleted: cursors advanced, do not emit */
 
+		rs->lastRowDeleted = deleted;
 		return true;
 	}
 }
 
+/*
+ * PgColumnarReadSetEmitDeleted
+ *		Return rows the snapshot sees as deleted as well, and let the caller ask
+ *		PgColumnarReadLastRowDeleted which ones they were (#1341). The index build
+ *		needs them: a row deleted after an older snapshot began is still visible to
+ *		that snapshot, so a new index must hold it, as heap's build holds a
+ *		recently-dead tuple. Only the unfiltered path honours this; a filter never
+ *		sees a deleted row.
+ */
+void
+PgColumnarReadSetEmitDeleted(PgColumnarReadState *readState, bool emit)
+{
+	readState->emitDeleted = emit;
+}
+
+bool
+PgColumnarReadLastRowDeleted(PgColumnarReadState *readState)
+{
+	return readState->lastRowDeleted;
+}
+
 bool
 PgColumnarReadNextRow(PgColumnarReadState *readState, Datum *values, bool *nulls,
 					uint64 *rowNumber)

src/columnar_reader.h

@@ -44,6 +44,8 @@ extern bool PgColumnarReadFoldColumn(PgColumnarReadState *readState, int attidx,
 								   const char **validity, const char **packed,
 								   int16 *attlen, const uint32 **vecRawLen);
 
+extern void PgColumnarReadSetEmitDeleted(PgColumnarReadState *readState, bool emit);
+extern bool PgColumnarReadLastRowDeleted(PgColumnarReadState *readState);
 extern void PgColumnarReadSetProjection(PgColumnarReadState *readState,
 										Bitmapset *projectedColumns);
 

src/columnar_tableam.c

@@ -1986,6 +1986,32 @@ pgcolumnar_index_fetch_tuple(struct IndexFetchTableData *scan, ItemPointer tid,
 	else if (PgColumnarBufferedRowByNumber(rel, rowNumber,
 										 slot->tts_values, slot->tts_isnull))
 		ExecStoreVirtualTuple(slot);
+	else if (snapshot != NULL && snapshot->snapshot_type == SNAPSHOT_DIRTY &&
+			 (TransactionIdIsValid(snapshot->xmin) ||
+			  TransactionIdIsValid(snapshot->xmax)) &&
+			 PgColumnarRowIsLive(rel, SnapshotAny, rowNumber))
+	{
+		/*
+		 * A DELETE STILL IN PROGRESS IS NOT A DELETE YET (#1341). The row reads
+		 * as gone only because another transaction's uncommitted delete-vector
+		 * row says so, and the catalog read above recorded that transaction in
+		 * the dirty snapshot. Answering "no such row" let _bt_check_unique insert
+		 * a duplicate without waiting: when the delete then rolled back, the key
+		 * had two live rows. Measured before this change: an INSERT of k racing
+		 * an uncommitted DELETE of k, then a ROLLBACK, committed and left two
+		 * rows with k.
+		 *
+		 * Heap answers such a row as present, with xmax naming the deleter, and
+		 * the caller waits on it and asks again with the outcome known. Report
+		 * the row the same way. The snapshot's xmin/xmax are left as the catalog
+		 * read set them, so the caller waits on that transaction. If the in-progress
+		 * change was not the one that deleted this row, the cost is a wait and a
+		 * re-check, never a wrong answer. The row's values are not needed by the
+		 * uniqueness check, so they are fetched under SnapshotAny, as for a row
+		 * the caller already knows the fate of.
+		 */
+		PgColumnarSlotStoreDeferred(slot, rel, SnapshotAny, rowNumber);
+	}
 	else
 		return false;
 
@@ -2137,20 +2163,54 @@ pgcolumnar_index_delete_tuples(Relation rel, TM_IndexDeleteOp *delstate)
 }
 #endif
 
+/*
+ * Speculative insertion, which INSERT ... ON CONFLICT uses whenever an arbiter
+ * index exists (#1341). docs/limitations.md promised DO NOTHING "works", and it
+ * raised 0A000 on any table with a unique index, which is the only table ON
+ * CONFLICT arbitrates on.
+ *
+ * The executor's pre-check already skips a conflict it can see, without
+ * inserting. What reaches here is a row that looked free: insert it as an
+ * ordinary row, and if the arbiter index then reports a conflict (a concurrent
+ * inserter won), withdraw it again by marking it deleted.
+ *
+ * NO SPECULATIVE TOKEN. Heap stamps the token so a waiter can wait on the
+ * insertion rather than on the whole transaction. Here a waiter waits on the
+ * transaction, which is correct and only coarser. The same unique-key lock the
+ * ordinary insert takes (issue #5) serialises two inserters of one key.
+ *
+ * WITHDRAWN, NOT "SUPER-DELETED". Heap makes an aborted speculative tuple
+ * invisible to everyone at once. A delete mark is transactional, but the row was
+ * never visible outside this transaction, and it commits or aborts together
+ * with the mark. So no other session can ever see it, and this one never does
+ * either, since the executor does not return a withdrawn row.
+ *
+ * DO UPDATE is still refused, and not here: a conflict sends the executor to
+ * lock the existing row, and row locking is unsupported (0A000).
+ */
 static void
 pgcolumnar_tuple_insert_speculative(Relation rel, TupleTableSlot *slot,
 								  CommandId cid, COLUMNAR_TABLE_OPTIONS options,
 								  struct BulkInsertStateData *bistate,
 								  uint32 specToken)
 {
-	COLUMNAR_UNSUPPORTED("speculative insert");
+	pgcolumnar_tuple_insert(rel, slot, cid, options, bistate);
 }
 
 static void
 pgcolumnar_tuple_complete_speculative(Relation rel, TupleTableSlot *slot,
 									uint32 specToken, bool succeeded)
 {
-	COLUMNAR_UNSUPPORTED("speculative insert");
+	if (succeeded)
+		return;
+
+	/*
+	 * The row may still be buffered, and a delete mark needs the row group
+	 * that covers it. Flush first, as the projection back-fill does before it
+	 * reads this transaction's rows.
+	 */
+	PgColumnarFlushWriteStateForRelation(RelationGetRelid(rel));
+	PgColumnarMarkRowDeleted(rel, PgColumnarItemPointerToRowNumber(&slot->tts_tid));
 }
 
 /*
@@ -2363,6 +2423,7 @@ pgcolumnar_index_build_range_scan(Relation table_rel, Relation index_rel,
 	Datum		indexValues[INDEX_MAX_KEYS];
 	bool		indexNulls[INDEX_MAX_KEYS];
 	double		reltuples = 0;
+	bool		alive;
 	uint64		rowNumber;
 
 	if (start_blockno != 0 || numblocks != InvalidBlockNumber)
@@ -2476,6 +2537,9 @@ pgcolumnar_index_build_range_scan(Relation table_rel, Relation index_rel,
 		ownReadState = true;
 	}
 
+	/* deleted rows too, flagged, so the build can index them as not alive (#1341) */
+	PgColumnarReadSetEmitDeleted(readState, true);
+
 	/*
 	 * Say which branch ran and how wide the reader it produced will actually
 	 * read, so a test can assert the projection NARROWED rather than infer it
@@ -2506,8 +2570,10 @@ pgcolumnar_index_build_range_scan(Relation table_rel, Relation index_rel,
 								 &rowNumber))
 			break;
 		ExecStoreVirtualTuple(slot);
+		alive = !PgColumnarReadLastRowDeleted(readState);
 
-		reltuples += 1;
+		if (alive)
+			reltuples += 1;
 
 		MemoryContextReset(econtext->ecxt_per_tuple_memory);
 
@@ -2518,7 +2584,19 @@ pgcolumnar_index_build_range_scan(Relation table_rel, Relation index_rel,
 
 		PgColumnarRowNumberToItemPointer(rowNumber, &slot->tts_tid);
 
-		callback(index_rel, &slot->tts_tid, indexValues, indexNulls, true,
+		/*
+		 * A DELETED ROW IS INDEXED TOO, AS NOT ALIVE (#1341). The build reads
+		 * under a current snapshot, but a transaction whose snapshot began before
+		 * the delete still sees the row, and may use this index. Leaving the row
+		 * out made that transaction's index scan miss a row it can see. Measured
+		 * before this change: REPEATABLE READ, then a committed DELETE of row
+		 * 4242, then CREATE INDEX; the old snapshot's index lookup of 4242 found
+		 * 0 rows, where its scan of the table found 1. Heap indexes a
+		 * recently-dead tuple the same way, as not alive, so a unique index does
+		 * not count it as a duplicate. The fetch still filters by each caller's
+		 * snapshot, so a new snapshot does not see the row by this entry.
+		 */
+		callback(index_rel, &slot->tts_tid, indexValues, indexNulls, alive,
 				 callback_state);
 	}
 
@@ -2781,8 +2859,15 @@ pgcolumnar_executor_end(QueryDesc *queryDesc)
 	else
 		standard_ExecutorEnd(queryDesc);
 
-	PgColumnarFlushAllPendingWrites();
-	PgColumnarFlushAllDeleteVectors();
+	/*
+	 * ONLY THIS SUBTRANSACTION'S BUFFERS (#1341). A statement ending inside a
+	 * plpgsql EXCEPTION block runs here while the block's subtransaction is
+	 * current; flushing the enclosing statement's buffers now would attribute
+	 * them to the block, and its rollback would lose them. They are flushed when
+	 * their own statement ends, back in their own subtransaction.
+	 */
+	PgColumnarFlushPendingWritesOfSubXact(GetCurrentSubTransactionId());
+	PgColumnarFlushDeleteVectorsOfSubXact(GetCurrentSubTransactionId());
 
 	/*
 	 * The fetch cache is scoped to a statement, so release it here rather than

src/columnar_write_state.c

@@ -3864,6 +3864,34 @@ PgColumnarFlushAllPendingWrites(void)
 	}
 }
 
+/*
+ * PgColumnarFlushPendingWritesOfSubXact
+ *		Flush only the write states the given subtransaction owns (#1341).
+ *
+ *		A flush writes catalog rows under the subtransaction that is CURRENT, not
+ *		the one that buffered the rows. So flushing an enclosing statement's buffer
+ *		while a child subtransaction is current -- a statement inside a plpgsql
+ *		EXCEPTION block ends -- attributes the parent's rows to the child, and the
+ *		block's rollback takes them away. Measured before this: an INSERT ... SELECT
+ *		f(g) of 5000 rows, where f's EXCEPTION block wrote a columnar table and rolled
+ *		back, kept one row. A parent's buffer waits for its own statement's end.
+ */
+void
+PgColumnarFlushPendingWritesOfSubXact(SubTransactionId subid)
+{
+	ListCell   *lc;
+
+	foreach(lc, PgColumnarWriteStates)
+	{
+		PgColumnarWriteState *writeState = (PgColumnarWriteState *) lfirst(lc);
+
+		if (writeState->subid != subid)
+			continue;
+		pgcolumnar_flush_row_group(writeState);
+		flush_ws_projections(writeState);
+	}
+}
+
 /*
  * PgColumnarDiscardAllPendingWrites
  *		Forget all pending write states. The backing memory is freed with the

src/columnar_write_state.h

@@ -36,6 +36,7 @@ extern bool PgColumnarBufferedRowByNumber(Relation rel, uint64 rowNumber,
 extern void PgColumnarForgetWriteStateForRelation(Oid relid);
 
 extern void PgColumnarFlushAllPendingWrites(void);
+extern void PgColumnarFlushPendingWritesOfSubXact(SubTransactionId subid);
 
 extern void PgColumnarDiscardAllPendingWrites(void);