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 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[email protected]
Backpatch: 11, where fast defaults were introduced

diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 5f34b26a2ce3d4618fe7f8b92a4eb0dba4f6323b..1d03b7dd136e516fcca787246c6de913e7c411fe 100644 (file)
--- 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
index a0cc82c2a36544355e9e0d432b1744aaed2dea93..d7ffc9c3e2bbf826f6a1a28414013c400c475b34 100644 (file)
--- 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
index 1867a70f6f3a029c4f15c69c8fea6ace8667c2b1..bc61bc8a8e998b8c9bc4f7fef32f47020700399e 100644 (file)
--- 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
index 40a15bd2d61c7a491b723cb324d537a8a3660691..10bc5ff757c211c4fa67f4bede754d5cb744c19a 100644 (file)
--- 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 0f65a79c7fd391a9e3e64ee1fff74c54ef684f60..4589b9e58d1708fb26acbd15e1d428776aaf97f4 100644 (file)
--- 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;