From ac79c72361520a7d9d11882094b6556aece9f467 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 11 May 2018 14:04:02 -0400 Subject: [PATCH v2] PL/pgSQL: Flatten TOAST data in nonatomic context When in a nonatomic execution context, when storing a potentially toasted datum into a PL/pgSQL variable, we need to flatten the datum (i.e., remove any references to TOAST data). Otherwise, a transaction end combined with, say, a concurrent VACUUM, between storing the datum and reading it, could remove the TOAST data, and then the data in the variable would no longer be readable. --- src/backend/utils/adt/expandedrecord.c | 41 ++++- src/include/utils/expandedrecord.h | 10 +- src/pl/plpgsql/src/pl_exec.c | 29 ++-- src/test/isolation/expected/plpgsql-toast.out | 152 ++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/plpgsql-toast.spec | 118 ++++++++++++++ 6 files changed, 326 insertions(+), 25 deletions(-) create mode 100644 src/test/isolation/expected/plpgsql-toast.out create mode 100644 src/test/isolation/specs/plpgsql-toast.spec diff --git a/src/backend/utils/adt/expandedrecord.c b/src/backend/utils/adt/expandedrecord.c index 0bf5fe8cc7..c319cddf65 100644 --- a/src/backend/utils/adt/expandedrecord.c +++ b/src/backend/utils/adt/expandedrecord.c @@ -19,6 +19,7 @@ #include "postgres.h" #include "access/htup_details.h" +#include "access/tuptoaster.h" #include "catalog/heap.h" #include "catalog/pg_type.h" #include "utils/builtins.h" @@ -433,7 +434,8 @@ make_expanded_record_from_exprecord(ExpandedRecordHeader *olderh, void expanded_record_set_tuple(ExpandedRecordHeader *erh, HeapTuple tuple, - bool copy) + bool copy, + bool expand_external) { int oldflags; HeapTuple oldtuple; @@ -482,15 +484,25 @@ expanded_record_set_tuple(ExpandedRecordHeader *erh, */ if (newtuple) { + if (HeapTupleHasExternal(newtuple)) + { + if (expand_external) + { + newtuple = toast_flatten_tuple(newtuple, erh->er_tupdesc); + newflags &= ~ER_FLAG_HAVE_EXTERNAL; + } + else + { + /* Remember if we have any out-of-line field values */ + newflags |= ER_FLAG_HAVE_EXTERNAL; + } + } + /* Save flat representation */ erh->fvalue = newtuple; erh->fstartptr = (char *) newtuple->t_data; erh->fendptr = ((char *) newtuple->t_data) + newtuple->t_len; newflags |= ER_FLAG_FVALUE_VALID; - - /* Remember if we have any out-of-line field values */ - if (HeapTupleHasExternal(newtuple)) - newflags |= ER_FLAG_HAVE_EXTERNAL; } else { @@ -690,6 +702,7 @@ ER_get_flat_size(ExpandedObjectHeader *eohptr) /* ... and we don't need it to recheck domain constraints */ expanded_record_set_field_internal(erh, i + 1, newValue, false, + false, false); /* Might as well free the detoasted value */ pfree(DatumGetPointer(newValue)); @@ -1093,6 +1106,7 @@ expanded_record_fetch_field(ExpandedRecordHeader *erh, int fnumber, void expanded_record_set_field_internal(ExpandedRecordHeader *erh, int fnumber, Datum newValue, bool isnull, + bool expand_external, bool check_constraints) { TupleDesc tupdesc; @@ -1144,7 +1158,12 @@ expanded_record_set_field_internal(ExpandedRecordHeader *erh, int fnumber, */ if (attr->attlen == -1 && VARATT_IS_EXTERNAL(DatumGetPointer(newValue))) - erh->flags |= ER_FLAG_HAVE_EXTERNAL; + { + if (expand_external) + newValue = PointerGetDatum(heap_tuple_fetch_attr((struct varlena *) DatumGetPointer(newValue))); + else + erh->flags |= ER_FLAG_HAVE_EXTERNAL; + } } /* @@ -1200,7 +1219,8 @@ expanded_record_set_field_internal(ExpandedRecordHeader *erh, int fnumber, */ void expanded_record_set_fields(ExpandedRecordHeader *erh, - const Datum *newValues, const bool *isnulls) + const Datum *newValues, const bool *isnulls, + bool expand_external) { TupleDesc tupdesc; Datum *dvalues; @@ -1260,7 +1280,12 @@ expanded_record_set_fields(ExpandedRecordHeader *erh, */ if (attr->attlen == -1 && VARATT_IS_EXTERNAL(DatumGetPointer(newValue))) - erh->flags |= ER_FLAG_HAVE_EXTERNAL; + { + if (expand_external) + newValue = PointerGetDatum(heap_tuple_fetch_attr((struct varlena *) DatumGetPointer(newValue))); + else + erh->flags |= ER_FLAG_HAVE_EXTERNAL; + } } /* diff --git a/src/include/utils/expandedrecord.h b/src/include/utils/expandedrecord.h index a95c9cce22..4f115a940c 100644 --- a/src/include/utils/expandedrecord.h +++ b/src/include/utils/expandedrecord.h @@ -171,7 +171,7 @@ extern ExpandedRecordHeader *make_expanded_record_from_tupdesc(TupleDesc tupdesc extern ExpandedRecordHeader *make_expanded_record_from_exprecord(ExpandedRecordHeader *olderh, MemoryContext parentcontext); extern void expanded_record_set_tuple(ExpandedRecordHeader *erh, - HeapTuple tuple, bool copy); + HeapTuple tuple, bool copy, bool expand_external); extern Datum make_expanded_record_from_datum(Datum recorddatum, MemoryContext parentcontext); extern TupleDesc expanded_record_fetch_tupdesc(ExpandedRecordHeader *erh); @@ -186,13 +186,15 @@ extern Datum expanded_record_fetch_field(ExpandedRecordHeader *erh, int fnumber, extern void expanded_record_set_field_internal(ExpandedRecordHeader *erh, int fnumber, Datum newValue, bool isnull, + bool expand_external, bool check_constraints); extern void expanded_record_set_fields(ExpandedRecordHeader *erh, - const Datum *newValues, const bool *isnulls); + const Datum *newValues, const bool *isnulls, + bool expand_external); /* outside code should never call expanded_record_set_field_internal as such */ -#define expanded_record_set_field(erh, fnumber, newValue, isnull) \ - expanded_record_set_field_internal(erh, fnumber, newValue, isnull, true) +#define expanded_record_set_field(erh, fnumber, newValue, isnull, expand_external) \ + expanded_record_set_field_internal(erh, fnumber, newValue, isnull, expand_external, true) /* * Inline-able fast cases. The expanded_record_fetch_xxx functions above diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 228d1c0d00..3f135f46ab 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -20,6 +20,7 @@ #include "access/htup_details.h" #include "access/transam.h" #include "access/tupconvert.h" +#include "access/tuptoaster.h" #include "catalog/pg_proc.h" #include "catalog/pg_type.h" #include "commands/defrem.h" @@ -912,16 +913,16 @@ plpgsql_exec_trigger(PLpgSQL_function *func, } else if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event)) { - expanded_record_set_tuple(rec_new->erh, trigdata->tg_trigtuple, false); + expanded_record_set_tuple(rec_new->erh, trigdata->tg_trigtuple, false, false); } else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) { - expanded_record_set_tuple(rec_new->erh, trigdata->tg_newtuple, false); - expanded_record_set_tuple(rec_old->erh, trigdata->tg_trigtuple, false); + expanded_record_set_tuple(rec_new->erh, trigdata->tg_newtuple, false, false); + expanded_record_set_tuple(rec_old->erh, trigdata->tg_trigtuple, false, false); } else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event)) { - expanded_record_set_tuple(rec_old->erh, trigdata->tg_trigtuple, false); + expanded_record_set_tuple(rec_old->erh, trigdata->tg_trigtuple, false, false); } else elog(ERROR, "unrecognized trigger action: not INSERT, DELETE, or UPDATE"); @@ -5061,7 +5062,7 @@ exec_assign_value(PLpgSQL_execstate *estate, /* And assign it. */ expanded_record_set_field(erh, recfield->finfo.fnumber, - value, isNull); + value, isNull, !estate->atomic); break; } @@ -5875,7 +5876,7 @@ exec_for_query(PLpgSQL_execstate *estate, PLpgSQL_stmt_forq *stmt, tupdescs_match) { /* Only need to assign a new tuple value */ - expanded_record_set_tuple(rec->erh, tuptab->vals[i], true); + expanded_record_set_tuple(rec->erh, tuptab->vals[i], true, !estate->atomic); } else { @@ -6647,7 +6648,7 @@ exec_move_row(PLpgSQL_execstate *estate, */ newerh = make_expanded_record_for_rec(estate, rec, NULL, rec->erh); - expanded_record_set_tuple(newerh, NULL, false); + expanded_record_set_tuple(newerh, NULL, false, !estate->atomic); assign_record_var(estate, rec, newerh); } else @@ -6689,7 +6690,7 @@ exec_move_row(PLpgSQL_execstate *estate, else { /* No coercion is needed, so just assign the row value */ - expanded_record_set_tuple(newerh, tup, true); + expanded_record_set_tuple(newerh, tup, true, !estate->atomic); } /* Complete the assignment */ @@ -6927,7 +6928,7 @@ exec_move_row_from_fields(PLpgSQL_execstate *estate, } /* Insert the coerced field values into the new expanded record */ - expanded_record_set_fields(newerh, values, nulls); + expanded_record_set_fields(newerh, values, nulls, !estate->atomic); /* Complete the assignment */ assign_record_var(estate, rec, newerh); @@ -7194,7 +7195,7 @@ exec_move_row_from_datum(PLpgSQL_execstate *estate, (erh->er_typmod == rec->erh->er_typmod && erh->er_typmod >= 0))) { - expanded_record_set_tuple(rec->erh, erh->fvalue, true); + expanded_record_set_tuple(rec->erh, erh->fvalue, true, !estate->atomic); return; } @@ -7216,7 +7217,7 @@ exec_move_row_from_datum(PLpgSQL_execstate *estate, (rec->rectypeid == RECORDOID || rec->rectypeid == erh->er_typeid)) { - expanded_record_set_tuple(newerh, erh->fvalue, true); + expanded_record_set_tuple(newerh, erh->fvalue, true, !estate->atomic); assign_record_var(estate, rec, newerh); return; } @@ -7306,7 +7307,7 @@ exec_move_row_from_datum(PLpgSQL_execstate *estate, (tupTypmod == rec->erh->er_typmod && tupTypmod >= 0))) { - expanded_record_set_tuple(rec->erh, &tmptup, true); + expanded_record_set_tuple(rec->erh, &tmptup, true, !estate->atomic); return; } @@ -7323,7 +7324,7 @@ exec_move_row_from_datum(PLpgSQL_execstate *estate, newerh = make_expanded_record_from_typeid(tupType, tupTypmod, mcontext); - expanded_record_set_tuple(newerh, &tmptup, true); + expanded_record_set_tuple(newerh, &tmptup, true, !estate->atomic); assign_record_var(estate, rec, newerh); return; } @@ -8069,6 +8070,8 @@ assign_simple_var(PLpgSQL_execstate *estate, PLpgSQL_var *var, else pfree(DatumGetPointer(var->value)); } + if (!estate->atomic && !isnull && var->datatype->typlen == -1) + newvalue = PointerGetDatum(heap_tuple_fetch_attr((struct varlena *) DatumGetPointer(newvalue))); /* Assign new value to datum */ var->value = newvalue; var->isnull = isnull; diff --git a/src/test/isolation/expected/plpgsql-toast.out b/src/test/isolation/expected/plpgsql-toast.out new file mode 100644 index 0000000000..3abeb259ab --- /dev/null +++ b/src/test/isolation/expected/plpgsql-toast.out @@ -0,0 +1,152 @@ +Parsed test spec with 2 sessions + +starting permutation: lock assign1 vacuum unlock +pg_advisory_unlock_all + + +pg_advisory_unlock_all + + +step lock: + SELECT pg_advisory_lock(1); + +pg_advisory_lock + + +step assign1: +do $$ + declare + x text; + begin + select test1.b into x from test1; + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'x = %', x; + end; +$$; + +step vacuum: + VACUUM test1; + +step unlock: + SELECT pg_advisory_unlock(1); + +pg_advisory_unlock + +t +step assign1: <... completed> + +starting permutation: lock assign2 vacuum unlock +pg_advisory_unlock_all + + +pg_advisory_unlock_all + + +step lock: + SELECT pg_advisory_lock(1); + +pg_advisory_lock + + +step assign2: +do $$ + declare + x text; + begin + x := (select test1.b from test1); + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'x = %', x; + end; +$$; + +step vacuum: + VACUUM test1; + +step unlock: + SELECT pg_advisory_unlock(1); + +pg_advisory_unlock + +t +step assign2: <... completed> + +starting permutation: lock assign3 vacuum unlock +pg_advisory_unlock_all + + +pg_advisory_unlock_all + + +step lock: + SELECT pg_advisory_lock(1); + +pg_advisory_lock + + +step assign3: +do $$ + declare + r record; + begin + select * into r from test1; + r.b := (select test1.b from test1); + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'r = %', r; + end; +$$; + +step vacuum: + VACUUM test1; + +step unlock: + SELECT pg_advisory_unlock(1); + +pg_advisory_unlock + +t +step assign3: <... completed> + +starting permutation: lock assign4 vacuum unlock +pg_advisory_unlock_all + + +pg_advisory_unlock_all + + +step lock: + SELECT pg_advisory_lock(1); + +pg_advisory_lock + + +step assign4: +do $$ + declare + r record; + begin + for r in select test1.b from test1 loop + null; + end loop; + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'r = %', r; + end; +$$; + +step vacuum: + VACUUM test1; + +step unlock: + SELECT pg_advisory_unlock(1); + +pg_advisory_unlock + +t +step assign4: <... completed> diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index b650e467a6..0e997215a8 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -74,3 +74,4 @@ test: predicate-gin-nomatch test: partition-key-update-1 test: partition-key-update-2 test: partition-key-update-3 +test: plpgsql-toast diff --git a/src/test/isolation/specs/plpgsql-toast.spec b/src/test/isolation/specs/plpgsql-toast.spec new file mode 100644 index 0000000000..f05bf41933 --- /dev/null +++ b/src/test/isolation/specs/plpgsql-toast.spec @@ -0,0 +1,118 @@ +# Test TOAST behavior in PL/pgSQL procedures with transaction control. +# +# We need to ensure that values stored in PL/pgSQL variables are free +# of external TOAST references, because those could disappear after a +# transaction is committed (leading to errors "missing chunk number +# ... for toast value ..."). The tests here do this by running VACUUM +# in a second session. Advisory locks are used to have the VACUUM +# kick in at the right time. The different "assign" steps are +# different ways variable assignments are done in PL/pgSQL. + +setup +{ + CREATE TABLE test1 (a int, b text); + ALTER TABLE test1 ALTER COLUMN b SET STORAGE EXTERNAL; + INSERT INTO test1 VALUES (1, repeat('foo', 2000)); +} + +teardown +{ + DROP TABLE test1; +} + +session "s1" + +setup +{ + SELECT pg_advisory_unlock_all(); +} + +# assign_simple_var() +step "assign1" +{ +do $$ + declare + x text; + begin + select test1.b into x from test1; + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'x = %', x; + end; +$$; +} + +# assign_simple_var() +step "assign2" +{ +do $$ + declare + x text; + begin + x := (select test1.b from test1); + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'x = %', x; + end; +$$; +} + +# expanded_record_set_field() +step "assign3" +{ +do $$ + declare + r record; + begin + select * into r from test1; + r.b := (select test1.b from test1); + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'r = %', r; + end; +$$; +} + +# expanded_record_set_tuple() +step "assign4" +{ +do $$ + declare + r record; + begin + for r in select test1.b from test1 loop + null; + end loop; + delete from test1; + commit; + perform pg_advisory_lock(1); + raise notice 'r = %', r; + end; +$$; +} + +session "s2" +setup +{ + SELECT pg_advisory_unlock_all(); +} +step "lock" +{ + SELECT pg_advisory_lock(1); +} +step "vacuum" +{ + VACUUM test1; +} +step "unlock" +{ + SELECT pg_advisory_unlock(1); +} + +permutation "lock" "assign1" "vacuum" "unlock" +permutation "lock" "assign2" "vacuum" "unlock" +permutation "lock" "assign3" "vacuum" "unlock" +permutation "lock" "assign4" "vacuum" "unlock" base-commit: 6186d0bd615ed2eb921ad13ccdf4ceed19d3f7a8 -- 2.17.0