Modify tuptoaster's API so that it does not try to modify the passed
authorTom Lane <tgl@sss.pgh.pa.us>
Sun, 20 Nov 2005 18:38:20 +0000 (18:38 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Sun, 20 Nov 2005 18:38:20 +0000 (18:38 +0000)
tuple in-place, but instead passes back an all-new tuple structure if
any changes are needed.  This is a much cleaner and more robust solution
for the bug discovered by Alexey Beschiokov; accordingly, revert the
quick hack I installed yesterday.
With this change, HeapTupleData.t_datamcxt is no longer needed; will
remove it in a separate commit in HEAD only.

src/backend/access/heap/heapam.c
src/backend/access/heap/tuptoaster.c
src/backend/executor/execMain.c
src/include/access/tuptoaster.h

index 6c669ed62b476d2dcd3430f426bdba1b14b2f0b0..fc750884c75647bc3ed249276d70ae535e75bf6c 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.200 2005/10/15 02:49:08 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.201 2005/11/20 18:38:20 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1085,12 +1085,19 @@ heap_get_latest_tid(Relation relation,
  *
  * use_fsm is passed directly to RelationGetBufferForTuple, which see for
  * more info.
+ *
+ * The return value is the OID assigned to the tuple (either here or by the
+ * caller), or InvalidOid if no OID.  The header fields of *tup are updated
+ * to match the stored tuple; in particular tup->t_self receives the actual
+ * TID where the tuple was stored.  But note that any toasting of fields
+ * within the tuple data is NOT reflected into *tup.
  */
 Oid
 heap_insert(Relation relation, HeapTuple tup, CommandId cid,
                        bool use_wal, bool use_fsm)
 {
        TransactionId xid = GetCurrentTransactionId();
+       HeapTuple       heaptup;
        Buffer          buffer;
 
        if (relation->rd_rel->relhasoids)
@@ -1128,19 +1135,24 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
        /*
         * If the new tuple is too big for storage or contains already toasted
         * out-of-line attributes from some other relation, invoke the toaster.
+        *
+        * Note: below this point, heaptup is the data we actually intend to
+        * store into the relation; tup is the caller's original untoasted data.
         */
        if (HeapTupleHasExternal(tup) ||
                (MAXALIGN(tup->t_len) > TOAST_TUPLE_THRESHOLD))
-               heap_tuple_toast_attrs(relation, tup, NULL);
+               heaptup = toast_insert_or_update(relation, tup, NULL);
+       else
+               heaptup = tup;
 
        /* Find buffer to insert this tuple into */
-       buffer = RelationGetBufferForTuple(relation, tup->t_len,
+       buffer = RelationGetBufferForTuple(relation, heaptup->t_len,
                                                                           InvalidBuffer, use_fsm);
 
        /* NO EREPORT(ERROR) from here till changes are logged */
        START_CRIT_SECTION();
 
-       RelationPutHeapTuple(relation, buffer, tup);
+       RelationPutHeapTuple(relation, buffer, heaptup);
 
        /* XLOG stuff */
        if (relation->rd_istemp)
@@ -1158,15 +1170,15 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
                uint8           info = XLOG_HEAP_INSERT;
 
                xlrec.target.node = relation->rd_node;
-               xlrec.target.tid = tup->t_self;
+               xlrec.target.tid = heaptup->t_self;
                rdata[0].data = (char *) &xlrec;
                rdata[0].len = SizeOfHeapInsert;
                rdata[0].buffer = InvalidBuffer;
                rdata[0].next = &(rdata[1]);
 
-               xlhdr.t_natts = tup->t_data->t_natts;
-               xlhdr.t_infomask = tup->t_data->t_infomask;
-               xlhdr.t_hoff = tup->t_data->t_hoff;
+               xlhdr.t_natts = heaptup->t_data->t_natts;
+               xlhdr.t_infomask = heaptup->t_data->t_infomask;
+               xlhdr.t_hoff = heaptup->t_data->t_hoff;
 
                /*
                 * note we mark rdata[1] as belonging to buffer; if XLogInsert decides
@@ -1180,8 +1192,8 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
                rdata[1].next = &(rdata[2]);
 
                /* PG73FORMAT: write bitmap [+ padding] [+ oid] + data */
-               rdata[2].data = (char *) tup->t_data + offsetof(HeapTupleHeaderData, t_bits);
-               rdata[2].len = tup->t_len - offsetof(HeapTupleHeaderData, t_bits);
+               rdata[2].data = (char *) heaptup->t_data + offsetof(HeapTupleHeaderData, t_bits);
+               rdata[2].len = heaptup->t_len - offsetof(HeapTupleHeaderData, t_bits);
                rdata[2].buffer = buffer;
                rdata[2].buffer_std = true;
                rdata[2].next = NULL;
@@ -1191,7 +1203,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
                 * page instead of restoring the whole thing.  Set flag, and hide
                 * buffer references from XLogInsert.
                 */
-               if (ItemPointerGetOffsetNumber(&(tup->t_self)) == FirstOffsetNumber &&
+               if (ItemPointerGetOffsetNumber(&(heaptup->t_self)) == FirstOffsetNumber &&
                        PageGetMaxOffsetNumber(page) == FirstOffsetNumber)
                {
                        info |= XLOG_HEAP_INIT_PAGE;
@@ -1212,13 +1224,23 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
        /*
         * If tuple is cachable, mark it for invalidation from the caches in case
         * we abort.  Note it is OK to do this after WriteBuffer releases the
-        * buffer, because the "tup" data structure is all in local memory, not in
-        * the shared buffer.
+        * buffer, because the heaptup data structure is all in local memory,
+        * not in the shared buffer.
         */
-       CacheInvalidateHeapTuple(relation, tup);
+       CacheInvalidateHeapTuple(relation, heaptup);
 
        pgstat_count_heap_insert(&relation->pgstat_info);
 
+       /*
+        * If heaptup is a private copy, release it.  Don't forget to copy t_self
+        * back to the caller's image, too.
+        */
+       if (heaptup != tup)
+       {
+               tup->t_self = heaptup->t_self;
+               heap_freetuple(heaptup);
+       }
+
        return HeapTupleGetOid(tup);
 }
 
@@ -1469,7 +1491,7 @@ l1:
         * context lock on the buffer first.
         */
        if (HeapTupleHasExternal(&tp))
-               heap_tuple_toast_attrs(relation, NULL, &tp);
+               toast_delete(relation, &tp);
 
        /*
         * Mark tuple for invalidation from system caches at next command
@@ -1553,8 +1575,10 @@ simple_heap_delete(Relation relation, ItemPointer tid)
  * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
  * (the last only possible if wait == false).
  *
- * On success, newtup->t_self is set to the TID where the new tuple
- * was inserted.
+ * On success, the header fields of *newtup are updated to match the new
+ * stored tuple; in particular, newtup->t_self is set to the TID where the
+ * new tuple was inserted.  However, any TOAST changes in the new tuple's
+ * data are not reflected into *newtup.
  *
  * In the failure cases, the routine returns the tuple's t_ctid and t_xmax.
  * If t_ctid is the same as otid, the tuple was deleted; if different, the
@@ -1570,6 +1594,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
        TransactionId xid = GetCurrentTransactionId();
        ItemId          lp;
        HeapTupleData oldtup;
+       HeapTuple       heaptup;
        PageHeader      dp;
        Buffer          buffer,
                                newbuf;
@@ -1760,11 +1785,12 @@ l2:
         * We need to invoke the toaster if there are already any out-of-line toasted
         * values present, or if the new tuple is over-threshold.
         */
+       newtupsize = MAXALIGN(newtup->t_len);
+
        need_toast = (HeapTupleHasExternal(&oldtup) ||
                                  HeapTupleHasExternal(newtup) ||
-                                 (MAXALIGN(newtup->t_len) > TOAST_TUPLE_THRESHOLD));
+                                 newtupsize > TOAST_TUPLE_THRESHOLD);
 
-       newtupsize = MAXALIGN(newtup->t_len);
        pagefree = PageGetFreeSpace((Page) dp);
 
        if (need_toast || newtupsize > pagefree)
@@ -1776,15 +1802,25 @@ l2:
                                                                           HEAP_MOVED);
                HeapTupleHeaderSetXmax(oldtup.t_data, xid);
                HeapTupleHeaderSetCmax(oldtup.t_data, cid);
+               /* temporarily make it look not-updated */
+               oldtup.t_data->t_ctid = oldtup.t_self;
                already_marked = true;
                LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
-               /* Let the toaster do its thing */
+               /*
+                * Let the toaster do its thing, if needed.
+                *
+                * Note: below this point, heaptup is the data we actually intend to
+                * store into the relation; newtup is the caller's original untoasted
+                * data.
+                */
                if (need_toast)
                {
-                       heap_tuple_toast_attrs(relation, newtup, &oldtup);
-                       newtupsize = MAXALIGN(newtup->t_len);
+                       heaptup = toast_insert_or_update(relation, newtup, &oldtup);
+                       newtupsize = MAXALIGN(heaptup->t_len);
                }
+               else
+                       heaptup = newtup;
 
                /*
                 * Now, do we need a new page for the tuple, or not?  This is a bit
@@ -1805,8 +1841,8 @@ l2:
                 */
                if (newtupsize > pagefree)
                {
-                       /* Assume there's no chance to put newtup on same page. */
-                       newbuf = RelationGetBufferForTuple(relation, newtup->t_len,
+                       /* Assume there's no chance to put heaptup on same page. */
+                       newbuf = RelationGetBufferForTuple(relation, heaptup->t_len,
                                                                                           buffer, true);
                }
                else
@@ -1823,7 +1859,7 @@ l2:
                                 * seldom be taken.
                                 */
                                LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
-                               newbuf = RelationGetBufferForTuple(relation, newtup->t_len,
+                               newbuf = RelationGetBufferForTuple(relation, heaptup->t_len,
                                                                                                   buffer, true);
                        }
                        else
@@ -1838,6 +1874,7 @@ l2:
                /* No TOAST work needed, and it'll fit on same page */
                already_marked = false;
                newbuf = buffer;
+               heaptup = newtup;
        }
 
        /*
@@ -1849,7 +1886,7 @@ l2:
        /* NO EREPORT(ERROR) from here till changes are logged */
        START_CRIT_SECTION();
 
-       RelationPutHeapTuple(relation, newbuf, newtup);         /* insert new tuple */
+       RelationPutHeapTuple(relation, newbuf, heaptup); /* insert new tuple */
 
        if (!already_marked)
        {
@@ -1863,13 +1900,13 @@ l2:
        }
 
        /* record address of new tuple in t_ctid of old one */
-       oldtup.t_data->t_ctid = newtup->t_self;
+       oldtup.t_data->t_ctid = heaptup->t_self;
 
        /* XLOG stuff */
        if (!relation->rd_istemp)
        {
                XLogRecPtr      recptr = log_heap_update(relation, buffer, oldtup.t_self,
-                                                                                        newbuf, newtup, false);
+                                                                                        newbuf, heaptup, false);
 
                if (newbuf != buffer)
                {
@@ -1905,10 +1942,10 @@ l2:
        /*
         * If new tuple is cachable, mark it for invalidation from the caches in
         * case we abort.  Note it is OK to do this after WriteBuffer releases the
-        * buffer, because the "newtup" data structure is all in local memory, not
+        * buffer, because the heaptup data structure is all in local memory, not
         * in the shared buffer.
         */
-       CacheInvalidateHeapTuple(relation, newtup);
+       CacheInvalidateHeapTuple(relation, heaptup);
 
        /*
         * Release the lmgr tuple lock, if we had it.
@@ -1918,6 +1955,16 @@ l2:
 
        pgstat_count_heap_update(&relation->pgstat_info);
 
+       /*
+        * If heaptup is a private copy, release it.  Don't forget to copy t_self
+        * back to the caller's image, too.
+        */
+       if (heaptup != newtup)
+       {
+               newtup->t_self = heaptup->t_self;
+               heap_freetuple(heaptup);
+       }
+
        return HeapTupleMayBeUpdated;
 }
 
index fd20f111b809ed090b516370927fbb6c43766d01..99d725c27ccd48543952fb4ff8562f28eba3b6bb 100644 (file)
@@ -8,14 +8,17 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/access/heap/tuptoaster.c,v 1.53 2005/10/15 02:49:09 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/access/heap/tuptoaster.c,v 1.54 2005/11/20 18:38:20 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
- *             heap_tuple_toast_attrs -
+ *             toast_insert_or_update -
  *                     Try to make a given tuple fit into one page by compressing
  *                     or moving off attributes
  *
+ *             toast_delete -
+ *                     Reclaim toast storage when a tuple is deleted
+ *
  *             heap_tuple_untoast_attr -
  *                     Fetch back a given value from the "secondary" relation
  *
 
 #undef TOAST_DEBUG
 
-static void toast_delete(Relation rel, HeapTuple oldtup);
 static void toast_delete_datum(Relation rel, Datum value);
-static void toast_insert_or_update(Relation rel, HeapTuple newtup,
-                                          HeapTuple oldtup);
 static Datum toast_save_datum(Relation rel, Datum value);
 static varattrib *toast_fetch_datum(varattrib *attr);
 static varattrib *toast_fetch_datum_slice(varattrib *attr,
                                                int32 sliceoffset, int32 length);
 
 
-/* ----------
- * heap_tuple_toast_attrs -
- *
- *     This is the central public entry point for toasting from heapam.
- *
- *     Calls the appropriate event specific action.
- * ----------
- */
-void
-heap_tuple_toast_attrs(Relation rel, HeapTuple newtup, HeapTuple oldtup)
-{
-       if (newtup == NULL)
-               toast_delete(rel, oldtup);
-       else
-               toast_insert_or_update(rel, newtup, oldtup);
-}
-
-
 /* ----------
  * heap_tuple_fetch_attr -
  *
@@ -305,7 +287,7 @@ toast_datum_size(Datum value)
  *     Cascaded delete toast-entries on DELETE
  * ----------
  */
-static void
+void
 toast_delete(Relation rel, HeapTuple oldtup)
 {
        TupleDesc       tupleDesc;
@@ -355,11 +337,22 @@ toast_delete(Relation rel, HeapTuple oldtup)
  *
  *     Delete no-longer-used toast-entries and create new ones to
  *     make the new tuple fit on INSERT or UPDATE
+ *
+ * Inputs:
+ *     newtup: the candidate new tuple to be inserted
+ *     oldtup: the old row version for UPDATE, or NULL for INSERT
+ * Result:
+ *     either newtup if no toasting is needed, or a palloc'd modified tuple
+ *     that is what should actually get stored
+ *
+ * NOTE: neither newtup nor oldtup will be modified.  This is a change
+ * from the pre-8.1 API of this routine.
  * ----------
  */
-static void
+HeapTuple
 toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
 {
+       HeapTuple       result_tuple;
        TupleDesc       tupleDesc;
        Form_pg_attribute *att;
        int                     numAttrs;
@@ -757,7 +750,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
        if (need_change)
        {
                HeapTupleHeader olddata = newtup->t_data;
-               char       *new_data;
+               HeapTupleHeader new_data;
                int32           new_len;
 
                /*
@@ -775,14 +768,18 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
                                                                                  toast_values, toast_isnull);
 
                /*
-                * Allocate new tuple in same context as old one.
+                * Allocate and zero the space needed, and fill HeapTupleData fields.
                 */
-               new_data = (char *) MemoryContextAlloc(newtup->t_datamcxt, new_len);
-               newtup->t_data = (HeapTupleHeader) new_data;
-               newtup->t_len = new_len;
+               result_tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + new_len);
+               result_tuple->t_len = new_len;
+               result_tuple->t_self = newtup->t_self;
+               result_tuple->t_tableOid = newtup->t_tableOid;
+               result_tuple->t_datamcxt = CurrentMemoryContext;
+               new_data = (HeapTupleHeader) ((char *) result_tuple + HEAPTUPLESIZE);
+               result_tuple->t_data = new_data;
 
                /*
-                * Put the tuple header and the changed values into place
+                * Put the existing tuple header and the changed values into place
                 */
                memcpy(new_data, olddata, olddata->t_hoff);
 
@@ -790,16 +787,11 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
                                                toast_values,
                                                toast_isnull,
                                                (char *) new_data + olddata->t_hoff,
-                                               &(newtup->t_data->t_infomask),
-                                               has_nulls ? newtup->t_data->t_bits : NULL);
-
-               /*
-                * In the case we modified a previously modified tuple again, free the
-                * memory from the previous run
-                */
-               if ((char *) olddata != ((char *) newtup + HEAPTUPLESIZE))
-                       pfree(olddata);
+                                               &(new_data->t_infomask),
+                                               has_nulls ? new_data->t_bits : NULL);
        }
+       else
+               result_tuple = newtup;
 
        /*
         * Free allocated temp values
@@ -816,6 +808,8 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup)
                for (i = 0; i < numAttrs; i++)
                        if (toast_delold[i])
                                toast_delete_datum(rel, toast_oldvalues[i]);
+
+       return result_tuple;
 }
 
 
index 6f79c4d58b00a364fa6c6eb5ff60591d7c4a04cf..c0c6cfbe5000f282e58bdb98754a62fc9abd11b9 100644 (file)
@@ -26,7 +26,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.259 2005/11/19 20:57:44 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/execMain.c,v 1.260 2005/11/20 18:38:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1445,16 +1445,6 @@ ExecInsert(TupleTableSlot *slot,
        estate->es_lastoid = newId;
        setLastTid(&(tuple->t_self));
 
-       /*
-        * KLUGE SOLUTION for bug found post 8.1 release: if the tuple toaster
-        * fired on the tuple then it changed the physical tuple inside the
-        * tuple slot, leaving any extracted information invalid.  Mark the
-        * extracted state invalid just in case.  Need to fix things so that
-        * the toaster gets to run against the tuple before we materialize it,
-        * but that's way too invasive for a stable branch.
-        */
-       slot->tts_nvalid = 0;
-
        /*
         * insert index entries for tuple
         */
@@ -1709,16 +1699,6 @@ lreplace:;
        IncrReplaced();
        (estate->es_processed)++;
 
-       /*
-        * KLUGE SOLUTION for bug found post 8.1 release: if the tuple toaster
-        * fired on the tuple then it changed the physical tuple inside the
-        * tuple slot, leaving any extracted information invalid.  Mark the
-        * extracted state invalid just in case.  Need to fix things so that
-        * the toaster gets to run against the tuple before we materialize it,
-        * but that's way too invasive for a stable branch.
-        */
-       slot->tts_nvalid = 0;
-
        /*
         * Note: instead of having to update the old index tuples associated with
         * the heap tuple, all we do is form and insert new index tuples. This is
index b45df277b1e79526c0fa60bbe388c119cbf06956..b5870e69dcbb88854d1270f83c6e705ff4789acc 100644 (file)
@@ -6,7 +6,7 @@
  *
  * Copyright (c) 2000-2005, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/include/access/tuptoaster.h,v 1.23 2005/07/06 19:02:53 momjian Exp $
+ * $PostgreSQL: pgsql/src/include/access/tuptoaster.h,v 1.24 2005/11/20 18:38:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 
 
 /* ----------
- * heap_tuple_toast_attrs() -
+ * toast_insert_or_update -
  *
- *             Called by heap_insert(), heap_update() and heap_delete().
- *             Outdates any no-longer-needed toast entries referenced
- *             by oldtup and creates new ones until newtup is no more than
- *             TOAST_TUPLE_TARGET (or we run out of toastable values).
- *             Possibly modifies newtup by replacing the t_data part!
+ *     Called by heap_insert() and heap_update().
+ * ----------
+ */
+extern HeapTuple toast_insert_or_update(Relation rel,
+                                                                               HeapTuple newtup, HeapTuple oldtup);
+
+/* ----------
+ * toast_delete -
  *
- *             oldtup is NULL if insert, newtup is NULL if delete.
+ *     Called by heap_delete().
  * ----------
  */
-extern void heap_tuple_toast_attrs(Relation rel,
-                                          HeapTuple newtup, HeapTuple oldtup);
+extern void toast_delete(Relation rel, HeapTuple oldtup);
 
 /* ----------
  * heap_tuple_fetch_attr() -