Hi,
On 2019-02-01 08:24:04 -0800, Andres Freund wrote:
> While working on the patch to slotify trigger.c I got somewhat confused
> by the need to expand tuples in trigger.c:
>
> static HeapTuple
> GetTupleForTrigger(EState *estate,
> EPQState *epqstate,
> ResultRelInfo *relinfo,
> ItemPointer tid,
> LockTupleMode lockmode,
> TupleTableSlot **newSlot)
> {
> ...
> if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
> result = heap_expand_tuple(&tuple, relation->rd_att);
> else
> result = heap_copytuple(&tuple);
> ReleaseBuffer(buffer);
>
> There's no explanation why GetTupleForTrigger() needs expanding tuples,
> but most other parts of postgres don't. Normally deforming ought to take
> care of expanding missing attrs.
>
> As far as I can tell, the reason that it's needed is that heap_gettar()
> wasn't updated along the rest of the functions. heap_deform_tuple(),
> heap_attisnull() etc look for missing attrs, but heap_getattr() doesn't.
>
> That, to me, makes no sense. The reason that we see a difference in
> regression test output is that composite_to_json() uses heap_getattr(),
> if it used heap_deform_tuple (which'd be considerably faster), we'd get
> the default value.
>
> Am I missing something?
Indeed, a hacky patch fixing heap_getattr makes the heap_expand_tuple()
in trigger.c unnecessary. Attached. I think we need to do something
along those lines.
Andrew, I think it'd be good to do a ground up review of the fast
defaults patch. There's been a fair number of issues in it, and this is
a another pretty fundamental issue.
Greetings,
Andres Freund
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..b412c81d141 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3412,10 +3412,15 @@ ltrmark:;
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
}
+#ifdef IAM_THE_WRONG_FIX
if (HeapTupleHeaderGetNatts(tuple.t_data) < relation->rd_att->natts)
result = heap_expand_tuple(&tuple, relation->rd_att);
else
result = heap_copytuple(&tuple);
+#else
+ result = heap_copytuple(&tuple);
+#endif
+
ReleaseBuffer(buffer);
return result;
diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h
index 873a3015fc4..81546ab46c7 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -744,6 +744,10 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
#endif /* defined(DISABLE_COMPLEX_MACRO) */
+extern Datum
+getmissingattr(TupleDesc tupleDesc,
+ int attnum, bool *isnull);
+
/* ----------------
* heap_getattr
*
@@ -765,8 +769,7 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
( \
((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
( \
- (*(isnull) = true), \
- (Datum)NULL \
+ getmissingattr(tupleDesc, attnum, isnull) \
) \
: \
fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \
