From b15db4fb408eb3e642b123e5aaf64f7d8a269a23 Mon Sep 17 00:00:00 2001 From: John Naylor Date: Wed, 17 Mar 2021 16:48:17 -0400 Subject: [PATCH v4] Fix bug in heap space management that was overly cautious about fillfactor Previously, if there were leftover ununused line pointers in a page, a table with a low fill factor would not be available for new tuples that were large enough to exceed fillfactor, even though the page contained no tuples at all. This lead to unnecessary relation extension. Fix by allowing some slack space equal to 1/8 the maximum space that could be taken up by line pointers. This is somewhat arbitrary, but should allow many cases to succeed where they didn't before. Per report from Floris van Nee. --- src/backend/access/heap/hio.c | 40 +++++++++++++++++++--------- src/test/regress/expected/insert.out | 22 +++++++++++++++ src/test/regress/sql/insert.sql | 23 ++++++++++++++++ 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c index 75223c9bea..ff2890e9e1 100644 --- a/src/backend/access/heap/hio.c +++ b/src/backend/access/heap/hio.c @@ -335,11 +335,21 @@ RelationGetBufferForTuple(Relation relation, Size len, Buffer buffer = InvalidBuffer; Page page; Size pageFreeSpace = 0, - saveFreeSpace = 0; + saveFreeSpace = 0, + targetFreeSpace = 0; BlockNumber targetBlock, otherBlock; bool needLock; + /* + * The minimum space on a page for it to be considered "empty" regardless + * of fillfactor. We base this on MaxHeapTupleSize, minus a small amount + * of slack. We allow slack equal to 1/8 the maximum space that could be + * taken by line pointers, which is somewhat arbitrary. + */ + const Size maxPaddedFsmRequest = MaxHeapTupleSize - + (MaxHeapTuplesPerPage / 8 * sizeof(ItemIdData)); + len = MAXALIGN(len); /* be conservative */ /* Bulk insert is not supported for updates, only inserts. */ @@ -358,6 +368,18 @@ RelationGetBufferForTuple(Relation relation, Size len, saveFreeSpace = RelationGetTargetPageFreeSpace(relation, HEAP_DEFAULT_FILLFACTOR); + /* + * We want to allow inserting a large tuple into an empty page even if + * that would violate the fillfactor. Otherwise, we would unnecessarily + * extend the relation. Instead, ask the FSM for maxPaddedFsmRequest + * bytes. This will allow it to return a page that is not quite empty + * because of unused line pointers. + */ + if (len + saveFreeSpace > maxPaddedFsmRequest) + targetFreeSpace = Max(len, maxPaddedFsmRequest); + else + targetFreeSpace = len + saveFreeSpace; + if (otherBuffer != InvalidBuffer) otherBlock = BufferGetBlockNumber(otherBuffer); else @@ -376,13 +398,7 @@ RelationGetBufferForTuple(Relation relation, Size len, * When use_fsm is false, we either put the tuple onto the existing target * page or extend the relation. */ - if (len + saveFreeSpace > MaxHeapTupleSize) - { - /* can't fit, don't bother asking FSM */ - targetBlock = InvalidBlockNumber; - use_fsm = false; - } - else if (bistate && bistate->current_buf != InvalidBuffer) + if (bistate && bistate->current_buf != InvalidBuffer) targetBlock = BufferGetBlockNumber(bistate->current_buf); else targetBlock = RelationGetTargetBlock(relation); @@ -393,7 +409,7 @@ RelationGetBufferForTuple(Relation relation, Size len, * We have no cached target page, so ask the FSM for an initial * target. */ - targetBlock = GetPageWithFreeSpace(relation, len + saveFreeSpace); + targetBlock = GetPageWithFreeSpace(relation, targetFreeSpace); } /* @@ -517,7 +533,7 @@ loop: } pageFreeSpace = PageGetHeapFreeSpace(page); - if (len + saveFreeSpace <= pageFreeSpace) + if (targetFreeSpace <= pageFreeSpace) { /* use this page as future insert target, too */ RelationSetTargetBlock(relation, targetBlock); @@ -550,7 +566,7 @@ loop: targetBlock = RecordAndGetPageWithFreeSpace(relation, targetBlock, pageFreeSpace, - len + saveFreeSpace); + targetFreeSpace); } /* @@ -582,7 +598,7 @@ loop: * Check if some other backend has extended a block for us while * we were waiting on the lock. */ - targetBlock = GetPageWithFreeSpace(relation, len + saveFreeSpace); + targetBlock = GetPageWithFreeSpace(relation, targetFreeSpace); /* * If some other waiter has already extended the relation, we diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index da50ee3b67..400a45f799 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -82,6 +82,28 @@ select col1, col2, char_length(col3) from inserttest; drop table inserttest; -- +-- test using FSM with large tuples and low fillfactor +-- +CREATE TABLE large_tuple_test (a int, b text) WITH (fillfactor = 10); +ALTER TABLE large_tuple_test ALTER COLUMN b SET STORAGE plain; +-- simulate having a few line pointers on the page so that the +-- page has free space slightly less than MaxHeapTupleSize +INSERT INTO large_tuple_test (select 1, NULL); +-- should still fit on the page +INSERT INTO large_tuple_test (select 2, repeat('a', 1000)); +SELECT pg_size_pretty(pg_relation_size('large_tuple_test'::regclass, 'main')); + pg_size_pretty +---------------- + 8192 bytes +(1 row) + +-- add small record to the second page +INSERT INTO large_tuple_test (select 3, NULL); +-- now this tuple won't fit on the second page, but the insert should +-- still succeed by extending the relation +INSERT INTO large_tuple_test (select 4, repeat('a', 8126)); +DROP TABLE large_tuple_test; +-- -- check indirection (field/array assignment), cf bug #14265 -- -- these tests are aware that transformInsertStmt has 3 separate code paths diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 963faa1614..560ccbc652 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -37,6 +37,29 @@ select col1, col2, char_length(col3) from inserttest; drop table inserttest; +-- +-- test using FSM with large tuples and low fillfactor +-- +CREATE TABLE large_tuple_test (a int, b text) WITH (fillfactor = 10); +ALTER TABLE large_tuple_test ALTER COLUMN b SET STORAGE plain; + +-- simulate having a few line pointers on the page so that the +-- page has free space slightly less than MaxHeapTupleSize +INSERT INTO large_tuple_test (select 1, NULL); + +-- should still fit on the page +INSERT INTO large_tuple_test (select 2, repeat('a', 1000)); +SELECT pg_size_pretty(pg_relation_size('large_tuple_test'::regclass, 'main')); + +-- add small record to the second page +INSERT INTO large_tuple_test (select 3, NULL); + +-- now this tuple won't fit on the second page, but the insert should +-- still succeed by extending the relation +INSERT INTO large_tuple_test (select 4, repeat('a', 8126)); + +DROP TABLE large_tuple_test; + -- -- check indirection (field/array assignment), cf bug #14265 -- -- 2.22.0