Fix heap_getattr() handling of fast defaults.

anarazel · anarazel · commit 297d627e074a · 2019-02-06T01:09:42.000-08:00
Previously heap_getattr() returned NULL for attributes with a fast default value (c.f. 16828d5), as it had no handling whatsoever for that case. A previous fix, 7636e5c, attempted to fix issues caused by this oversight, but just expanding OLD tuples for triggers doesn't actually solve the underlying issue. One known consequence of this bug is that the check for HOT updates can return the wrong result, when a previously fast-default'ed column is set to NULL. Which in turn means that an index over a column with fast default'ed columns might be corrupt if the underlying column(s) allow NULLs. Fix by handling fast default columns in heap_getattr(), remove now superfluous expansion in GetTupleForTrigger(). Author: Andres Freund Discussion: https://postgr.es/m/20190201162404.onngi77f26baem4g@alap3.anarazel.de Backpatch: 11, where fast defaults were introduced
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
@@ -80,7 +80,7 @@
 /*
  * Return the missing value of an attribute, or NULL if there isn't one.
  */
-static Datum
+Datum
 getmissingattr(TupleDesc tupleDesc,
 			   int attnum, bool *isnull)
 {
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
@@ -3396,10 +3396,7 @@ ltrmark:;
 		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 	}
 
-	if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
-		result = heap_expand_tuple(&tuple, relation->rd_att);
-	else
-		result = heap_copytuple(&tuple);
+	result = heap_copytuple(&tuple);
 	ReleaseBuffer(buffer);
 
 	return result;
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
@@ -783,10 +783,7 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
 		((attnum) > 0) ? \
 		( \
 			((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
-			( \
-				(*(isnull) = true), \
-				(Datum)NULL \
-			) \
+				getmissingattr((tupleDesc), (attnum), (isnull)) \
 			: \
 				fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
 		) \
@@ -807,6 +804,8 @@ extern Datum nocachegetattr(HeapTuple tup, int attnum,
 			   TupleDesc att);
 extern Datum heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
 				bool *isnull);
+extern Datum getmissingattr(TupleDesc tupleDesc,
+			   int attnum, bool *isnull);
 extern HeapTuple heap_copytuple(HeapTuple tuple);
 extern void heap_copytuple_with_tuple(HeapTuple src, HeapTuple dest);
 extern Datum heap_copy_tuple_as_datum(HeapTuple tuple, TupleDesc tupleDesc);
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
@@ -770,6 +770,33 @@ SELECT * FROM vtype2;
  2 | yyy
 (2 rows)
 
+-- Ensure that defaults are checked when evaluating whether HOT update
+-- is possible, this was broken for a while:
+-- https://postgr.es/m/20190202133521.ylauh3ckqa7colzj%40alap3.anarazel.de
+BEGIN;
+CREATE TABLE t();
+INSERT INTO t DEFAULT VALUES;
+ALTER TABLE t ADD COLUMN a int DEFAULT 1;
+CREATE INDEX ON t(a);
+-- set column with a default 1 to NULL, due to a bug that wasn't
+-- noticed has heap_getattr buggily returned NULL for default columns
+UPDATE t SET a = NULL;
+-- verify that index and non-index scans show the same result
+SET LOCAL enable_seqscan = true;
+SELECT * FROM t WHERE a IS NULL;
+ a 
+---
+  
+(1 row)
+
+SET LOCAL enable_seqscan = false;
+SELECT * FROM t WHERE a IS NULL;
+ a 
+---
+  
+(1 row)
+
+ROLLBACK;
 -- cleanup
 DROP TABLE vtype;
 DROP TABLE vtype2;
diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql
@@ -505,6 +505,26 @@ ALTER TABLE vtype2 ALTER COLUMN b TYPE varchar(20) USING b::varchar(20);
 SELECT * FROM vtype2;
 
 
+-- Ensure that defaults are checked when evaluating whether HOT update
+-- is possible, this was broken for a while:
+-- https://postgr.es/m/20190202133521.ylauh3ckqa7colzj%40alap3.anarazel.de
+BEGIN;
+CREATE TABLE t();
+INSERT INTO t DEFAULT VALUES;
+ALTER TABLE t ADD COLUMN a int DEFAULT 1;
+CREATE INDEX ON t(a);
+-- set column with a default 1 to NULL, due to a bug that wasn't
+-- noticed has heap_getattr buggily returned NULL for default columns
+UPDATE t SET a = NULL;
+
+-- verify that index and non-index scans show the same result
+SET LOCAL enable_seqscan = true;
+SELECT * FROM t WHERE a IS NULL;
+SET LOCAL enable_seqscan = false;
+SELECT * FROM t WHERE a IS NULL;
+ROLLBACK;
+
+
 -- cleanup
 DROP TABLE vtype;
 DROP TABLE vtype2;

Original file line number	Diff line number	Diff line change
`@@ -80,7 +80,7 @@`
`80`	`80`	`/*`
`81`	`81`	`* Return the missing value of an attribute, or NULL if there isn't one.`
`82`	`82`	`*/`
`83`		`-static Datum`
	`83`	`+Datum`
`84`	`84`	`getmissingattr(TupleDesc tupleDesc,`
`85`	`85`	`int attnum, bool *isnull)`
`86`	`86`	`{`