Re: Detoast a column once per row instead of once per reference

Hi,

I tested v2 on an assert build with the injection points from 0004,
comparing detoast_reuse on and off in the same binary. I think three
changes are needed.

1. Do not make the copy for functions that read only a slice or the
size.

Some functions read only part of a TOASTed value: "=" on text and
bytea compares the lengths first, octet_length() reads only the
size, substr()/left()/starts_with() fetch the first chunks. With the
patch, once the column is marked, the first reference makes a full
copy, so these functions now read the whole value. The patch has a
list of such functions, but only the planner uses it.

200 rows, txt about 48 kB, STORAGE EXTERNAL. Shared buffer hits in
WHERE, lower is better:

off on
txt = 'x' OR txt = 'y' 3 1843
octet_length(txt) < 10, then two full reads 3 1843
left(txt, 3) = 'zzz', then two full reads 643 1843
two full reads on every row (control) 3643 1843

"then" is CASE WHEN <test> THEN length(txt) > 0 AND
position('q' in txt) > 0 ELSE false END, so the test runs first.

So a function missing from the list costs more than a lost
optimization, and extension functions are in no list. These
functions should use the copy if it already exists, and get the
stored datum otherwise.

2. Check the stored form in one place for every function call.

pg_column_size() and friends must see the stored datum. The patch
checks this for ordinary calls, but not for a call in FROM, which
gets the copy. With the default settings the answer is wrong:

CREATE TABLE z (txt text);
ALTER TABLE z ALTER txt SET COMPRESSION pglz;
INSERT INTO z VALUES (repeat('abcdefgh', 40000));

SELECT pg_column_size(txt) FROM z; -- 3679
SELECT s FROM z, LATERAL pg_column_size(z.txt) s; -- 320004
SELECT s FROM z, LATERAL pg_column_compression(z.txt) s; -- NULL

One check used by every place that compiles function arguments
would fix this, and a pg_proc flag or support request (your first
open question) could later replace the hard-coded list there.

3. Check that the copy still belongs to the row (extensions).

The patch relies on every refill going through ExecClearTuple() or
ExecStore*(). Extensions do not always do that: some refill a
virtual slot in place and evaluate again. That was harmless before;
now the next rows get the first row's copy. An extension's custom
scan that does this and calls ExecQual() per row gave a wrong count
in a release build (529 instead of 500) and hit the Assert in
slot_detoast_attr() in an assert build.

tts_detoast_src already records which datum each copy was made from,
but only in assert builds. Keeping it in all builds and making a new
copy when the datum has changed costs one pointer compare.

Also, the executor does not check that a marked column is varlena,
so a plan changed after set_plan_references(), as some extensions
do, can leave a bit on an int column. A typlen check is cheap.

Best regards,
Denis Smirnov