[PATCH] plpgsql: detoast a returned record before keeping it in a non-atomic context

Hi,

In a DO block or a procedure that can COMMIT or ROLLBACK, r := f(),
where f is a PL/pgSQL function that returns a record variable, keeps
f's TOAST pointers in r, so once the row is gone r can no longer be
read:

CREATE TABLE big (id int, t text);
ALTER TABLE big ALTER COLUMN t SET STORAGE EXTERNAL;

CREATE FUNCTION get_big(k int) RETURNS big LANGUAGE plpgsql AS $$
DECLARE
r big;
BEGIN
SELECT * INTO r FROM big WHERE id = k;
RETURN r;
END $$;

DO $$
DECLARE
r big;
n bigint;
BEGIN
INSERT INTO big VALUES (1, repeat('x', 10000));
r := get_big(1);
ROLLBACK;
-- any scan of the TOAST table marks the dead chunks as such
EXECUTE format('SELECT count(*) FROM %s',
(SELECT reltoastrelid::regclass FROM pg_class
WHERE oid = 'big'::regclass)) INTO n;
RAISE NOTICE 'length %', length(r.t);
END $$;
ERROR: missing chunk number 0 for toast value 16390 in pg_toast_16384
CONTEXT: PL/pgSQL function inline_code_block line 13 at RAISE

With the attached patch, the NOTICE says length 10000. The failure is
the same on master and on the heads of 14 through 19, in a procedure
too, and after a COMMIT once the row is deleted and another session
has vacuumed the table. A procedure called with such a record as an
argument, as in CALL p(get_big(1)), fails the same way after its own
ROLLBACK.

Since 2efc924180f ("Detoast plpgsql variables if they might live
across a transaction boundary"), PL/pgSQL detoasts the values it
stores into variables in a non-atomic context: assign_simple_var()
for scalars, and expand_external = !estate->atomic wherever it fills
a record variable (expanded_record_set_tuple(),
expanded_record_set_fields(), expanded_record_set_field()).
exec_move_row_from_datum() has one path that doesn't: when the value
is a read/write expanded record of the right type, which is what a
PL/pgSQL function hands back when it returns a record variable, it
just commandeers the object. That record was filled inside the
function, an atomic context, so it may still hold TOAST pointers. A
record from a SQL function, one read with SELECT INTO, and a text
returned by a PL/pgSQL function are all detoasted as expected.

Bug #17669 [1]/messages/by-id/17669-2238da15dce5c53c@postgresql.org is related: there a function lost a TOAST value after
a TRUNCATE, and the suggested workaround was a procedure, since a
non-atomic context detoasts on assignment. With r := get_big(1),
that TRUNCATE case fails in a procedure as well, until this patch.

The attached patch keeps the commandeering, but when we're not atomic
and the record has ER_FLAG_HAVE_EXTERNAL set, it first inlines the
out-of-line fields in place by computing the record's flat size:
ER_get_flat_size() detoasts each external field and clears the flag.
An assertion checks that the flag is clear afterwards.
expandedrecord.c sets that flag whenever it stores an out-of-line
value and clears it only when none is left, so it is safe to rely on.
Atomic contexts, and records with no out-of-line fields, are not
affected, and a loop of 100,000 such assignments leaves the backend's
memory contexts unchanged. Inlining in place keeps the commandeering
as it is, and is cheaper than copying the record.

The patch adds a step to the plpgsql-toast isolation test, which
already has one step for each way PL/pgSQL assigns to a variable: a
DO block assigns a function's record with an out-of-line field,
deletes the row and commits, and another session vacuums the table
before the field is read. Without the fix the step fails with
"missing chunk number 0"; with it, it prints the length.

On master, check-world passes (without the TAP tests), and the PL/pgSQL
tests and the plpgsql-toast isolation test pass with the server running
under Valgrind.

The patch cherry-picks cleanly to REL_14_STABLE through REL_19_STABLE;
on each of them the PL/pgSQL and core regression tests and the
plpgsql-toast isolation test pass with it, and the new step fails
without it. I think it should be back-patched to 14, as f21fadafaf0
("Avoid detoasting failure after COMMIT inside a plpgsql FOR loop"),
the same kind of hole, was back-patched to 11.

[1]: /messages/by-id/17669-2238da15dce5c53c@postgresql.org

Regards,
Muzzammil Sarwar