8000 Fix deletion of speculatively inserted TOAST on conflict · linearregression/postgres@07ef035 · GitHub
[go: up one dir, main page]
Skip to content {"props":{"docsUrl":"https://docs.github.com/get-started/accessibility/keyboard-shortcuts"}}

Commit 07ef035

Browse files
anarazelanarazel
committed
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
1 parent cf9b0fe commit 07ef035

File tree

7 files changed

+87
-13
lines changed

7 files changed

+87
-13
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3335,7 +3335,7 @@ heap_delete(Relation relation, ItemPointer tid,
33353335
Assert(!HeapTupleHasExternal(&tp));
33363336
}
33373337
else if (HeapTupleHasExternal(&tp))
3338-
toast_delete(relation, &tp);
3338+
toast_delete(relation, &tp, false);
33393339

33403340
/*
33413341
* Mark tuple for invalidation from system caches at next command
@@ -6057,7 +6057,8 @@ heap_finish_speculative(Relation relation, HeapTuple tuple)
60576057
* could deadlock with each other, which would not be acceptable.
60586058
*
60596059
* This is somewhat redundant with heap_delete, but we prefer to have a
6060-
* dedicated routine with stripped down requirements.
6060+
* dedicated routine with stripped down requirements. Note that this is also
6061+
* used to delete the TOAST tuples created during speculative insertion.
60616062
*
60626063
* This routine does not affect logical decoding as it only looks at
60636064
* confirmation records.
@@ -6101,7 +6102,7 @@ heap_abort_speculative(Relation relation, HeapTuple tuple)
61016102
*/
61026103
if (tp.t_data->t_choice.t_heap.t_xmin != xid)
61036104
elog(ERROR, "attempted to kill a tuple inserted by another transaction");
6104-
if (!HeapTupleHeaderIsSpeculative(tp.t_data))
6105+
if (!(IsToastRelation(relation) || HeapTupleHeaderIsSpeculative(tp.t_data)))
61056106
elog(ERROR, "attempted to kill a non-speculative tuple");
61066107
Assert(!HeapTupleHeaderIsHeapOnly(tp.t_data));
61076108

@@ -6171,7 +6172,10 @@ heap_abort_speculative(Relation relation, HeapTuple tuple)
61716172
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
61726173

61736174
if (HeapTupleHasExternal(&tp))
6174-
toast_delete(relation, &tp);
6175+
{
6176+
Assert(!IsToastRelation(relation));
6177+
toast_delete(relation, &tp, true);
6178+
}
61756179

61766180
/*
61776181
* Never need to mark tuple for invalidation, since catalogs don't support

‎src/backend/access/heap/tuptoaster.c

Lines changed: 9 additions & 6 deletions
Original fil 8000 e line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ typedef struct toast_compress_header
6767
#define TOAST_COMPRESS_SET_RAWSIZE(ptr, len) \
6868
(((toast_compress_header *) (ptr))->rawsize = (len))
6969

70-
static void toast_delete_datum(Relation rel, Datum value);
70+
static void toast_delete_datum(Relation rel, Datum value, bool is_speculative);
7171
static Datum toast_save_datum(Relation rel, Datum value,
7272
struct varlena * oldexternal, int options);
7373
static bool toastrel_valueid_exists(Relation toastrel, Oid valueid);
@@ -461,7 +461,7 @@ toast_datum_size(Datum value)
461461
* ----------
462462
*/
463463
void
464-
toast_delete(Relation rel, HeapTuple oldtup)
464+
toast_delete(Relation rel, HeapTuple oldtup, bool is_speculative)
465465
{
466466
TupleDesc tupleDesc;
467467
Form_pg_attribute *att;
@@ -508,7 +508,7 @@ toast_delete(Relation rel, HeapTuple oldtup)
508508
if (toast_isnull[i])
509509
continue;
510510
else if (VARATT_IS_EXTERNAL_ONDISK(PointerGetDatum(value)))
511-
toast_delete_datum(rel, value);
511+
toast_delete_datum(rel, value, is_speculative);
512512
}
513513
}
514514
}
@@ -1064,7 +1064,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
10641064
if (need_delold)
10651065
for (i = 0; i < numAttrs; i++)
10661066
if (toast_delold[i])
1067-
toast_delete_datum(rel, toast_oldvalues[i]);
1067+
toast_delete_datum(rel, toast_oldvalues[i], false);
10681068

10691069
return result_tuple;
10701070
}
@@ -1656,7 +1656,7 @@ toast_save_datum(Relation rel, Datum value,
16561656
* ----------
16571657
*/
16581658
static void
1659-
toast_delete_datum(Relation rel, Datum value)
1659+
toast_delete_datum(Relation rel, Datum value, bool is_speculative)
16601660
{
16611661
struct varlena *attr = (struct varlena *) DatumGetPointer(value);
16621662
struct varatt_external toast_pointer;
@@ -1707,7 +1707,10 @@ toast_delete_datum(Relation rel, Datum value)
17071707
/*
17081708
* Have a chunk, delete it
17091709
*/
1710-
simple_heap_delete(toastrel, &toasttup->t_self);
1710+
if (is_speculative)
1711+
heap_abort_speculative(toastrel, toasttup);
1712+
else
1713+
simple_heap_delete(toastrel, &toasttup->t_self);
17111714
}
17121715

17131716
/*

‎src/backend/utils/time/tqual.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -418,8 +418,8 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
418418

419419
/*
420420
* An invalid Xmin can be left behind by a speculative insertion that
421-
* is canceled by super-deleting the tuple. We shouldn't see any of
422-
* those in TOAST tables, but better safe than sorry.
421+
* is canceled by super-deleting the tuple. This also applies to
422+
* TOAST tuples created during speculative insertion.
423423
*/
424424
else if (!TransactionIdIsValid(HeapTupleHeaderGetXmin(tuple)))
425425
return false;

‎src/include/access/tuptoaster.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ extern HeapTuple toast_insert_or_update(Relation rel,
142142
* Called by heap_delete().
143143
* ----------
144144
*/
145-
extern void toast_delete(Relation rel, HeapTuple oldtup);
145+
extern void toast_delete(Relation rel, HeapTuple oldtup, bool is_speculative);
146146

147147
/* ----------
148148
* heap_tuple_fetch_attr() -

‎src/test/isolation/expected/insert-conflict-toast.out

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
Parsed test spec with 3 sessions
2+
3+
starting permutation: s2insert s3insert s1commit
4+
pg_advisory_xact_lock
5+
6+
7+
step s2insert:
8+
INSERT INTO ctoast (key, val) VALUES (1, ctoast_large_val()) ON CONFLICT DO NOTHING;
9+
<waiting ...>
10+
step s3insert:
11+
INSERT INTO ctoast (key, val) VALUES (1, ctoast_large_val()) ON CONFLICT DO NOTHING;
12+
<waiting ...>
13+
step s1commit: COMMIT;
14+
step s2insert: <... completed>
15+
step s3insert: <... completed>

‎src/test/isolation/isolation_schedule

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ test: insert-conflict-do-nothing
2828
test: insert-conflict-do-update
2929
test: insert-conflict-do-update-2
3030
test: insert-conflict-do-update-3
31+
test: insert-conflict-toast
3132
test: delete-abort-savept
3233
test: delete-abort-savept-2
3334
test: aborted-keyrevoke

‎src/test/isolation/specs/insert-conflict-toast.spec

Lines changed: 51 additions & 0 deletions
< 40BE /tr>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# INSERT...ON CONFLICT test on table with TOAST
2+
#
3+
# This test verifies that speculatively inserted toast rows do not
4+
# cause conflicts. It does so by using expression index over a
5+
# function which acquires an advisory lock, triggering two index
6+
# insertions to happen almost at the same time. This is not guaranteed
7+
# to lead to a failed speculative insertion, but makes one quite
8+
# likely.
9+
10+
setup
11+
{
12+
CREATE TABLE ctoast (key int primary key, val text);
13+
CREATE OR REPLACE FUNCTION ctoast_lock_func(int) RETURNS INT IMMUTABLE LANGUAGE SQL AS 'select pg_advisory_xact_lock_shared(1); select $1;';
14+
CREATE OR REPLACE FUNCTION ctoast_large_val() RETURNS TEXT LANGUAGE SQL AS 'select array_agg(md5(g::text))::text from generate_series(1, 256) g';
15+
CREATE UNIQUE INDEX ctoast_lock_idx ON ctoast (ctoast_lock_func(key));
16+
}
17+
18+
teardown
19+
{
20+
DROP TABLE ctoast;
21+
DROP FUNCTION ctoast_lock_func(int);
22+
DROP FUNCTION ctoast_large_val();
23+
}
24+
25+
session "s1"
26+
setup
27+
{
28+
BEGIN ISOLATION LEVEL READ COMMITTED;
29+
SELECT pg_advisory_xact_lock(1);
30+
}
31+
step "s1commit" { COMMIT; }
32+
33+
session "s2"
34+
setup
35+
{
36+
SET default_transaction_isolation = 'read committed';
37+
}
38+
step "s2insert" {
39+
INSERT INTO ctoast (key, val) VALUES (1, ctoast_large_val()) ON CONFLICT DO NOTHING;
40+
}
41+
42+
session "s3"
43+
setup
44+
{
45+
SET default_transaction_isolation = 'read committed';
46+
}
47+
step "s3insert" {
48+
INSERT INTO ctoast (key, val) VALUES (1, ctoast_large_val()) ON CONFLICT DO NOTHING;
49+
}
50+
51+
permutation "s2insert" "s3insert" "s1commit"

0 commit comments

Comments
 (0)
0