From 5fef71e1ffa0ba6a12ca9db6e286221750f53dc2 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 4 Feb 2019 07:10:12 -0800
Subject: [PATCH] Fix heap_getattr() handling of fast defaults.

Previously heap_getattr() returned NULL for attributes with a fast
default value (c.f. 16828d5c0273), as it had no handling whatsoever
for that case.

A previous fix, 7636e5c60f, attempted to fix issues caused by this
oversight, but just expanding OLD tuples for triggers doesn't actually
solve the actual 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.

Fix by handling 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
---
 src/backend/access/common/heaptuple.c      |  4 +---
 src/backend/commands/trigger.c             |  5 +---
 src/include/access/htup_details.h          |  7 +++---
 src/test/regress/expected/fast_default.out | 27 ++++++++++++++++++++++
 src/test/regress/sql/fast_default.sql      | 20 ++++++++++++++++
 5 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index ed4549ca579..783b04a3cb9 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -71,8 +71,6 @@
 #define VARLENA_ATT_IS_PACKABLE(att) \
 	((att)->attstorage != 'p')
 
-static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
-
 
 /* ----------------------------------------------------------------
  *						misc support routines
@@ -82,7 +80,7 @@ static Datum getmissingattr(TupleDesc tupleDesc, int attnum, bool *isnull);
 /*
  * 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
index 7b5896b98f9..0b245a613e0 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3412,10 +3412,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
index 873a3015fc4..d2b46456f1c 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -764,10 +764,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)) \
 		) \
@@ -788,6 +785,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
index 40a15bd2d61..10bc5ff757c 100644
--- 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
index 0f65a79c7fd..4589b9e58d1 100644
--- 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;
-- 
2.18.0.rc2.dirty

