Concurrent REPACK: Clear out dropped-column values from all tuples

Commit e5d25959cf made REPACK null-out values of dropped columns in
the NEW tuple of an UPDATE.  However, a concurrent INSERT can carry one
too, for example when a trigger returns a copy of an existing row.
REPACK (CONCURRENTLY) then kept the values for such columns, which is
wasteful -- or worse, failed with "row is too big" if the new heap
happens not to have a TOAST table.

Move their clearing from prepare_concurrent_update() to restore_tuple()
so that it affects all tuples processed, not just the UPDATE ones.

Author: Shihao Zhong <zhong950419@gmail.com>
Backpatch-through: 19
Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com

src/backend/commands/repack.c

@@ -2819,7 +2819,7 @@ apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt)
 			/*
 			 * Adjust spilled_tuple so that it can be used as the new tuple in
 			 * the update that we're about to replay.  This fixes TOAST
-			 * pointers as well as remove useless values from dropped columns.
+			 * pointers.
 			 */
 			prepare_concurrent_update(spilled_tuple, ondisk_tuple);
 
@@ -2948,6 +2948,7 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
 {
 	uint32		t_len;
 	HeapTuple	tup;
+	TupleDesc	desc = slot->tts_tupleDescriptor;
 	int			natt_ext;
 
 	/* Read the tuple. */
@@ -2965,6 +2966,16 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
 	 */
 	ExecForceStoreHeapTuple(tup, slot, false);
 
+	/*
+	 * Dropped columns can still have values in some tuples.  Null them out.
+	 * This saves space, and the new heap might not even have a TOAST table.
+	 */
+	for (int i = 0; i < desc->natts; i++)
+	{
+		if (TupleDescCompactAttr(desc, i)->attisdropped)
+			slot->tts_isnull[i] = true;
+	}
+
 	/*
 	 * Next, read any attributes we stored separately into the tts_values
 	 * array elements expecting them, if any.  This matches
@@ -2973,8 +2984,6 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
 	BufFileReadExact(file, &natt_ext, sizeof(natt_ext));
 	if (natt_ext > 0)
 	{
-		TupleDesc	desc = slot->tts_tupleDescriptor;
-
 		for (int i = 0; i < desc->natts; i++)
 		{
 			CompactAttribute *attr = TupleDescCompactAttr(desc, i);
@@ -3017,13 +3026,10 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot)
  * update that we're about to replay.
  *
  * We perform the following critical change:
- * - Any EXTERNAL_ONDISK toast pointers so that it points to the corresponding
- *   toast value in 'src' (the transient table) instead.  The TOAST storage for
- *   'dest' is going to be dropped, so these values cannot be used any longer.
- *
- * We also apply the following optimization:
- * - If any columns are dropped but the slot still contains values, mark them
- *   as null to avoid uselessly wasting space in the new relation.
+ * - Update any EXTERNAL_ONDISK toast pointers so that they point to the
+ *   corresponding toast values in 'src' (the transient table) instead.
+ *   The TOAST storage for 'dest' is going to be dropped, so these values
+ *   cannot be used any longer.
  */
 static void
 prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src)
@@ -3036,14 +3042,7 @@ prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src)
 		varlena    *varlena_dst;
 
 		if (attr->attisdropped)
-		{
-			if (!slot_attisnull(dest, i + 1))
-			{
-				slot_getsomeattrs(dest, i + 1);
-				dest->tts_isnull[i] = true;
-			}
 			continue;
-		}
 		if (attr->attlen != -1)
 			continue;
 		if (slot_attisnull(dest, i + 1))

src/test/modules/injection_points/expected/repack_dropped.out

@@ -1,6 +1,6 @@
 Parsed test spec with 3 sessions
 
-starting permutation: s1_size s2_repack s3_updates s1_unlock s2_noop s1_size
+starting permutation: s1_size s2_repack s3_updates s3_insert s1_unlock s2_noop s1_size
 injection_points_attach
 -----------------------
                        
@@ -23,6 +23,9 @@ step s2_repack:
 step s3_updates: 
 	UPDATE repack_dropped SET a = a || a;
 
+step s3_insert: 
+	INSERT INTO repack_dropped (id, a) VALUES (3, 'three');
+
 step s1_unlock: 
 	SELECT injection_points_wakeup('repack-concurrently-before-lock');
 
@@ -43,5 +46,6 @@ id|fits_in_one_block
 --+-----------------
  1|t                
  2|t                
-(2 rows)
+ 3|t                
+(3 rows)
 

src/test/modules/injection_points/specs/repack_dropped.spec

@@ -13,11 +13,18 @@ setup {
 	INSERT INTO repack_dropped (id, a, b) VALUES (2, 'two',
 		repeat(encode(sha256('2'), 'hex'), current_setting('block_size')::int / 32));
 	CREATE FUNCTION repack_dropped_f() RETURNS trigger LANGUAGE plpgsql AS
-		$$ BEGIN
+		$$ DECLARE r repack_dropped;
+		BEGIN
+			IF TG_OP = 'INSERT' THEN
+				r := (SELECT t FROM repack_dropped t WHERE id = 1);
+				r.id := NEW.id;
+				RETURN r;
+			END IF;
 			IF NEW.id = 1 THEN return OLD; END IF;
 			RETURN NEW;
 		END $$;
 	CREATE TRIGGER repack_dropped_t BEFORE UPDATE ON repack_dropped FOR EACH ROW EXECUTE FUNCTION repack_dropped_f();
+	CREATE TRIGGER repack_dropped_ins_t BEFORE INSERT ON repack_dropped FOR EACH ROW EXECUTE FUNCTION repack_dropped_f();
 	ALTER TABLE repack_dropped DROP COLUMN b;
 }
 
@@ -63,10 +70,17 @@ step s3_updates
 	UPDATE repack_dropped SET a = a || a;
 }
 
+# The trigger makes tuple 3 a copy of tuple 1, dropped column included.
+step s3_insert
+{
+	INSERT INTO repack_dropped (id, a) VALUES (3, 'three');
+}
+
 permutation
 	s1_size
 	s2_repack
 	s3_updates
+	s3_insert
 	s1_unlock
 	s2_noop
 	s1_size