Fix deletion of speculatively inserted TOAST on conflict

INSERT ..  ON CONFLICT runs a pre-check of the possible conflicting
constraints before performing the actual speculative insertion.  In case
the inserted tuple included TOASTed columns the ON CONFLICT condition
would be handled correctly in case the conflict was caught by the
pre-check, but if two transactions entered the speculative insertion
phase at the same time, one would have to re-try, and the code for
aborting a speculative insertion did not handle deleting the
speculatively inserted TOAST datums correctly.

TOAST deletion would fail with "ERROR: attempted to delete invisible
tuple" as we attempted to remove the TOAST tuples using
simple_heap_delete which reasoned that the given tuples should not be
visible to the command that wrote them.

This commit updates the heap_abort_speculative() function which aborts
the conflicting tuple to use itself, via toast_delete, for deleting
associated TOAST datums.  Like before, the inserted toast rows are not
marked as being speculative.

This commit also adds a isolationtester spec test, exercising the
relevant code path. Unfortunately 9.5 cannot handle two waiting
sessions, and thus cannot execute this test.

Reported-By: Viren Negi, Oskari Saarenmaa
Author: Oskari Saarenmaa, edited a bit by me
Bug: #14150
Discussion: <20160519123338.12513.20271@wrigleys.postgresql.org>
Backpatch: 9.5, where ON CONFLICT was introduced

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 2052362ae65ca4d806466deb0a7b49659ecce229..ee2dd57ec642d1a27370c56ac7aa639a80cabd3d 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3112,7 +3112,7 @@ l1:
        Assert(!HeapTupleHasExternal(&tp));
    }
    else if (HeapTupleHasExternal(&tp))
-       toast_delete(relation, &tp);
+       toast_delete(relation, &tp, false);
 
    /*
     * Mark tuple for invalidation from system caches at next command
@@ -5723,7 +5723,8 @@ heap_finish_speculative(Relation relation, HeapTuple tuple)
  * could deadlock with each other, which would not be acceptable.
  *
  * This is somewhat redundant with heap_delete, but we prefer to have a
- * dedicated routine with stripped down requirements.
+ * dedicated routine with stripped down requirements.  Note that this is also
+ * used to delete the TOAST tuples created during speculative insertion.
  *
  * This routine does not affect logical decoding as it only looks at
  * confirmation records.
@@ -5767,7 +5768,7 @@ heap_abort_speculative(Relation relation, HeapTuple tuple)
     */
    if (tp.t_data->t_choice.t_heap.t_xmin != xid)
        elog(ERROR, "attempted to kill a tuple inserted by another transaction");
-   if (!HeapTupleHeaderIsSpeculative(tp.t_data))
+   if (!(IsToastRelation(relation) || HeapTupleHeaderIsSpeculative(tp.t_data)))
        elog(ERROR, "attempted to kill a non-speculative tuple");
    Assert(!HeapTupleHeaderIsHeapOnly(tp.t_data));
 
@@ -5837,7 +5838,10 @@ heap_abort_speculative(Relation relation, HeapTuple tuple)
    LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 
    if (HeapTupleHasExternal(&tp))
-       toast_delete(relation, &tp);
+   {
+       Assert(!IsToastRelation(relation));
+       toast_delete(relation, &tp, true);
+   }
 
    /*
     * Never need to mark tuple for invalidation, since catalogs don't support

diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index b9691a57bef615c7d32bc1b61e5c3f3fef7bd879..f989f9577d3768f5ce2facf8e52942902433ef3b 100644 (file)
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -66,7 +66,7 @@ typedef struct toast_compress_header
 #define TOAST_COMPRESS_SET_RAWSIZE(ptr, len) \
    (((toast_compress_header *) (ptr))->rawsize = (len))
 
-static void toast_delete_datum(Relation rel, Datum value);
+static void toast_delete_datum(Relation rel, Datum value, bool is_speculative);
 static Datum toast_save_datum(Relation rel, Datum value,
                 struct varlena * oldexternal, int options);
 static bool toastrel_valueid_exists(Relation toastrel, Oid valueid);
@@ -459,7 +459,7 @@ toast_datum_size(Datum value)
  * ----------
  */
 void
-toast_delete(Relation rel, HeapTuple oldtup)
+toast_delete(Relation rel, HeapTuple oldtup, bool is_speculative)
 {
    TupleDesc   tupleDesc;
    Form_pg_attribute *att;
@@ -506,7 +506,7 @@ toast_delete(Relation rel, HeapTuple oldtup)
            if (toast_isnull[i])
                continue;
            else if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
-               toast_delete_datum(rel, value);
+               toast_delete_datum(rel, value, is_speculative);
        }
    }
 }
@@ -1062,7 +1062,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
    if (need_delold)
        for (i = 0; i < numAttrs; i++)
            if (toast_delold[i])
-               toast_delete_datum(rel, toast_oldvalues[i]);
+               toast_delete_datum(rel, toast_oldvalues[i], false);
 
    return result_tuple;
 }
@@ -1654,7 +1654,7 @@ toast_save_datum(Relation rel, Datum value,
  * ----------
  */
 static void
-toast_delete_datum(Relation rel, Datum value)
+toast_delete_datum(Relation rel, Datum value, bool is_speculative)
 {
    struct varlena *attr = (struct varlena *) DatumGetPointer(value);
    struct varatt_external toast_pointer;
@@ -1703,7 +1703,10 @@ toast_delete_datum(Relation rel, Datum value)
        /*
         * Have a chunk, delete it
         */
-       simple_heap_delete(toastrel, &toasttup->t_self);
+       if (is_speculative)
+           heap_abort_speculative(toastrel, toasttup);
+       else
+           simple_heap_delete(toastrel, &toasttup->t_self);
    }
 
    /*

diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 2d449852083e3a9e5a47eba7fcd840f719db2812..9cc780ee10f81159bb6d409d5dd302ab01c709da 100644 (file)
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -408,8 +408,8 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
 
        /*
         * An invalid Xmin can be left behind by a speculative insertion that
-        * is cancelled by super-deleting the tuple.  We shouldn't see any of
-        * those in TOAST tables, but better safe than sorry.
+        * is canceled by super-deleting the tuple.  This also applies to
+        * TOAST tuples created during speculative insertion.
         */
        else if (!TransactionIdIsValid(HeapTupleHeaderGetXmin(tuple)))
            return false;

diff --git a/src/include/access/tuptoaster.h b/src/include/access/tuptoaster.h
index 7d185357714f886df52b3367c8799f9d848af4fa..9e16e256565fccd32b23b38d3e47b8d43311f68e 100644 (file)
--- a/src/include/access/tuptoaster.h
+++ b/src/include/access/tuptoaster.h
@@ -142,7 +142,7 @@ extern HeapTuple toast_insert_or_update(Relation rel,
  * Called by heap_delete().
  * ----------
  */
-extern void toast_delete(Relation rel, HeapTuple oldtup);
+extern void toast_delete(Relation rel, HeapTuple oldtup, bool is_speculative);
 
 /* ----------
  * heap_tuple_fetch_attr() -