Don't lose column values on REPACK

Commit 28d534e2ae introduced reform_tuple() with a fast path that
returns the source tuple verbatim when no dropped columns require fixing
up.  I (Álvaro) failed to realize that this broke handling of columns
with a 'missingval' defined: after a VACUUM FULL, CLUSTER, or REPACK
operation, the catalogued missingval is thrown away, so the tuples are
no longer correct.

Fix by forcing the rewrite when the tuple is shorter than the tuple
descriptor.

Author: Satya Narlapuram <satyanarlapuram@gmail.com>
Discussion: https://postgr.es/m/CAHg+QDeoccU5CudrJpmSKZfKZ1gRMNY=5BxSC=JpHgkonzgcOw@mail.gmail.com

contrib/test_decoding/expected/repack.out

@@ -29,3 +29,25 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a
 
 DROP TABLE ptnowner;
 DROP ROLE regress_ptnowner;
+-- Verify that REPACK (CONCURRENTLY) doesn't lose "attmissingval" columns
+CREATE TABLE rpk_missing (id int PRIMARY KEY);
+INSERT INTO rpk_missing SELECT generate_series(1, 3);
+ALTER TABLE rpk_missing ADD COLUMN a int DEFAULT 42;
+SELECT * FROM rpk_missing;
+ id | a  
+----+----
+  1 | 42
+  2 | 42
+  3 | 42
+(3 rows)
+
+REPACK (CONCURRENTLY) rpk_missing;
+SELECT * FROM rpk_missing;
+ id | a  
+----+----
+  1 | 42
+  2 | 42
+  3 | 42
+(3 rows)
+
+DROP TABLE rpk_missing;

contrib/test_decoding/sql/repack.sql

@@ -23,3 +23,12 @@ SELECT a.relname, a.relfilenode=b.relfilenode FROM pg_class a
   JOIN ptnowner_oldnodes b USING (oid) ORDER BY a.relname COLLATE "C";
 DROP TABLE ptnowner;
 DROP ROLE regress_ptnowner;
+
+-- Verify that REPACK (CONCURRENTLY) doesn't lose "attmissingval" columns
+CREATE TABLE rpk_missing (id int PRIMARY KEY);
+INSERT INTO rpk_missing SELECT generate_series(1, 3);
+ALTER TABLE rpk_missing ADD COLUMN a int DEFAULT 42;
+SELECT * FROM rpk_missing;
+REPACK (CONCURRENTLY) rpk_missing;
+SELECT * FROM rpk_missing;
+DROP TABLE rpk_missing;

src/backend/access/heap/heapam_handler.c

@@ -2396,10 +2396,10 @@ heap_insert_for_repack(HeapTuple tuple, Relation OldHeap, Relation NewHeap,
 /*
  * Subroutine for reform_and_rewrite_tuple and heap_insert_for_repack.
  *
- * Deform the given tuple, set values of dropped columns to NULL, form a new
- * tuple and return it.  If no attributes need to be changed in this way, a
- * copy of the original tuple is returned.  Caller is responsible for freeing
- * the returned tuple.
+ * Deform the given tuple, set values of dropped columns to NULL, and fill in
+ * any values from attmissingval; then form a new tuple and return it.  If no
+ * attributes need to be changed, a copy of the original tuple is returned.
+ * Caller is responsible for freeing the returned tuple.
  *
  * XXX this coding assumes that both relations have the same tupledesc.
  */
@@ -2411,13 +2411,33 @@ reform_tuple(HeapTuple tuple, Relation OldHeap, Relation NewHeap,
 	TupleDesc	newTupDesc = RelationGetDescr(NewHeap);
 	bool		needs_reform = false;
 
-	/* Skip work if the tuple doesn't need any attributes changed */
-	for (int i = 0; i < newTupDesc->natts; i++)
+	/*
+	 * A short tuple might require values from attmissing val, so activate the
+	 * coding unconditionally in that case.  The value might legitimally be
+	 * NULL otherwise, so this is slightly wasteful, but it probably beats
+	 * having to test each attribute for presence of attmissingval each time.
+	 */
+	if (HeapTupleHeaderGetNatts(tuple->t_data) < newTupDesc->natts)
+		needs_reform = true;
+
+	/*
+	 * If the column has been dropped but a value is still present, we can
+	 * optimize storage now by getting rid of it.
+	 */
+	if (!needs_reform)
 	{
-		if (TupleDescCompactAttr(newTupDesc, i)->attisdropped &&
-			!heap_attisnull(tuple, i + 1, newTupDesc))
-			needs_reform = true;
+		for (int i = 0; i < newTupDesc->natts; i++)
+		{
+			if (TupleDescCompactAttr(newTupDesc, i)->attisdropped &&
+				!heap_attisnull(tuple, i + 1, newTupDesc))
+			{
+				needs_reform = true;
+				break;
+			}
+		}
 	}
+
+	/* Skip work if no changes are needed */
 	if (!needs_reform)
 		return heap_copytuple(tuple);
 

src/test/regress/expected/fast_default.out

@@ -969,6 +969,42 @@ SELECT count(*)
      0
 (1 row)
 
+-- Verify that table-rewriting maintenance commands preserve attmissingval
+-- columns.
+CREATE TABLE t (id int PRIMARY KEY);
+INSERT INTO t SELECT generate_series(1, 3);
+ALTER TABLE t ADD COLUMN a int DEFAULT 42;
+ALTER TABLE t ADD COLUMN b int NOT NULL DEFAULT 7 CHECK (b > 0);
+VACUUM FULL t;
+SELECT * FROM t ORDER BY id;
+ id | a  | b 
+----+----+---
+  1 | 42 | 7
+  2 | 42 | 7
+  3 | 42 | 7
+(3 rows)
+
+ALTER TABLE t ADD COLUMN c text DEFAULT 'hello';
+CLUSTER t USING t_pkey;
+SELECT * FROM t ORDER BY id;
+ id | a  | b |   c   
+----+----+---+-------
+  1 | 42 | 7 | hello
+  2 | 42 | 7 | hello
+  3 | 42 | 7 | hello
+(3 rows)
+
+ALTER TABLE t ADD COLUMN d int DEFAULT 99;
+REPACK t;
+SELECT * FROM t ORDER BY id;
+ id | a  | b |   c   | d  
+----+----+---+-------+----
+  1 | 42 | 7 | hello | 99
+  2 | 42 | 7 | hello | 99
+  3 | 42 | 7 | hello | 99
+(3 rows)
+
+DROP TABLE t;
 -- cleanup
 DROP FOREIGN TABLE ft1;
 DROP SERVER s0;

src/test/regress/sql/fast_default.sql

@@ -653,6 +653,22 @@ SELECT count(*)
   WHERE attrelid = 'ft1'::regclass AND
     (attmissingval IS NOT NULL OR atthasmissing);
 
+-- Verify that table-rewriting maintenance commands preserve attmissingval
+-- columns.
+CREATE TABLE t (id int PRIMARY KEY);
+INSERT INTO t SELECT generate_series(1, 3);
+ALTER TABLE t ADD COLUMN a int DEFAULT 42;
+ALTER TABLE t ADD COLUMN b int NOT NULL DEFAULT 7 CHECK (b > 0);
+VACUUM FULL t;
+SELECT * FROM t ORDER BY id;
+ALTER TABLE t ADD COLUMN c text DEFAULT 'hello';
+CLUSTER t USING t_pkey;
+SELECT * FROM t ORDER BY id;
+ALTER TABLE t ADD COLUMN d int DEFAULT 99;
+REPACK t;
+SELECT * FROM t ORDER BY id;
+DROP TABLE t;
+
 -- cleanup
 DROP FOREIGN TABLE ft1;
 DROP SERVER s0;