On 2020-Nov-04, Tomas Vondra wrote: > The first test is fairly trivial - it simply builds index on toasted data > and then shows how an insert and select fail. There's a caveat, that this > requires a DELETE + VACUUM, and the VACUUM actually has to cleanup the rows. > So there must be no concurrent transactions that might need the rows, which > is unlikely in regression tests. So this requires waiting for all running > transactions to finish - I did that by building an index concurrently. It's > a bit strange, but it's better than any other solution I could think of > (timeout or some custom wait for xacts).
There are recent changes in vacuum for temp tables (commit 94bc27b57680?) that would maybe make this stable enough, without having to have the CIC there. At least, I tried it locally a few times and it appears to work well. This won't work for older releases though, just master. This is patch 0001 attached here. > The second test is a bit redundant - it merely checks that both CREATE INDEX > and INSERT INTO fail the same way when the index tuple gets too large. > Before the fix there were some inconsistencies - the CREATE INDEX succeeded > because it used TOASTed data. So ultimately this tests the same thing, but > from a different perspective. Hmm. This one shows page size in the error messages, so it'll fail on nonstandard builds. I think we try to stay away from introducing those, so I'd leave this test out. The code fix looks all right -- I'd just move the #include lines to their place. Patch 0002. You add this comment: > + /* > + * Do nothing if value is not of varlena type. We don't > need to > + * care about NULL values here, thanks to bv_allnulls > above. > + * > + * If value is stored EXTERNAL, must fetch it so we are > not > + * depending on outside storage. > + * > + * XXX Is this actually true? Could it be that the > summary is > + * NULL even for range with non-NULL data? E.g. > degenerate bloom > + * filter may be thrown away, etc. > + */ I think the XXX comment points to a bug that we don't have right now, since neither minmax nor inclusion can end up with a NULL summary starting from non-NULL data. But if the comment about bloom is correct, then surely it'll become a bug when bloom is added. I don't think we need the second part of this comment: > +/* > + * This enables de-toasting of index entries. Needed until VACUUM is > + * smart enough to rebuild indexes from scratch. > + */ ... because, surely, we're now never working on having VACUUM rebuild indexes from scratch. In fact, I wonder if we need the #define at all. I propose to remove all those #ifdef lines in your patch. The fix looks good to me. I just added a comment in 0003.
>From 101638065fb13e540f58f96ff839af982d7c5344 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Thu, 5 Nov 2020 14:12:38 -0300 Subject: [PATCH 1/3] Simplify test to use a temp table rather than vacuum. --- src/test/regress/expected/brin.out | 7 +------ src/test/regress/sql/brin.sql | 8 +------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out index 79ec028f69..0650e24a31 100644 --- a/src/test/regress/expected/brin.out +++ b/src/test/regress/expected/brin.out @@ -527,7 +527,7 @@ EXPLAIN (COSTS OFF) SELECT * FROM brin_test WHERE b = 1; (2 rows) -- make sure data are properly de-toasted in BRIN index -CREATE TABLE brintest_3 (a text, b text, c text, d text); +CREATE TEMP TABLE brintest_3 (a text, b text, c text, d text); -- long random strings (~2000 chars each, so ~6kB for min/max on two -- columns) to trigger toasting WITH rand_value AS (SELECT string_agg(md5(i::text),'') AS val FROM generate_series(1,60) s(i)) @@ -535,11 +535,6 @@ INSERT INTO brintest_3 SELECT val, val, val, val FROM rand_value; CREATE INDEX brin_test_toast_idx ON brintest_3 USING brin (b, c); DELETE FROM brintest_3; --- We need to wait a bit for all transactions to complete, so that the --- vacuum actually removes the TOAST rows. Creating an index concurrently --- is a one way to achieve that, because it does exactly such wait. -CREATE INDEX CONCURRENTLY brin_test_temp_idx ON brintest_3(a); -DROP INDEX brin_test_temp_idx; -- vacuum the table, to discard TOAST data VACUUM brintest_3; -- retry insert with a different random-looking (but deterministic) value diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql index f1b510fb0b..9b284738dd 100644 --- a/src/test/regress/sql/brin.sql +++ b/src/test/regress/sql/brin.sql @@ -472,7 +472,7 @@ EXPLAIN (COSTS OFF) SELECT * FROM brin_test WHERE a = 1; EXPLAIN (COSTS OFF) SELECT * FROM brin_test WHERE b = 1; -- make sure data are properly de-toasted in BRIN index -CREATE TABLE brintest_3 (a text, b text, c text, d text); +CREATE TEMP TABLE brintest_3 (a text, b text, c text, d text); -- long random strings (~2000 chars each, so ~6kB for min/max on two -- columns) to trigger toasting @@ -483,12 +483,6 @@ SELECT val, val, val, val FROM rand_value; CREATE INDEX brin_test_toast_idx ON brintest_3 USING brin (b, c); DELETE FROM brintest_3; --- We need to wait a bit for all transactions to complete, so that the --- vacuum actually removes the TOAST rows. Creating an index concurrently --- is a one way to achieve that, because it does exactly such wait. -CREATE INDEX CONCURRENTLY brin_test_temp_idx ON brintest_3(a); -DROP INDEX brin_test_temp_idx; - -- vacuum the table, to discard TOAST data VACUUM brintest_3; -- 2.20.1
>From 3e7dfad4e8d83633cb311281a1eb77f62dcc5566 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Thu, 5 Nov 2020 14:14:15 -0300 Subject: [PATCH 2/3] Move includes to right place --- src/backend/access/brin/brin_tuple.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c index 96eab9e87c..e3d5bde51b 100644 --- a/src/backend/access/brin/brin_tuple.c +++ b/src/backend/access/brin/brin_tuple.c @@ -32,15 +32,15 @@ #include "postgres.h" #include "access/brin_tuple.h" +#include "access/detoast.h" +#include "access/heaptoast.h" #include "access/htup_details.h" +#include "access/toast_internals.h" #include "access/tupdesc.h" #include "access/tupmacs.h" #include "utils/datum.h" #include "utils/memutils.h" -#include "access/detoast.h" -#include "access/heaptoast.h" -#include "access/toast_internals.h" /* * This enables de-toasting of index entries. Needed until VACUUM is -- 2.20.1
>From cd94b204749da311506155141cd3d2eb179e28c2 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Thu, 5 Nov 2020 14:14:31 -0300 Subject: [PATCH 3/3] Add comment --- src/backend/access/brin/brin_tuple.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c index e3d5bde51b..6774f597a4 100644 --- a/src/backend/access/brin/brin_tuple.c +++ b/src/backend/access/brin/brin_tuple.c @@ -159,6 +159,12 @@ brin_form_tuple(BrinDesc *brdesc, BlockNumber blkno, BrinMemTuple *tuple, if (tuple->bt_columns[keyno].bv_hasnulls) anynulls = true; + /* + * Now obtain the values of each stored datum. Note that some values + * might be toasted, and we cannot rely on the original heap values + * sticking around forever, so we must detoast them. Also try to + * compress them. + */ for (datumno = 0; datumno < brdesc->bd_info[keyno]->oi_nstored; datumno++) -- 2.20.1
