On Mon, Feb 26, 2018 at 1:32 PM, Claudio Freire <klaussfre...@gmail.com> wrote:
> On Mon, Feb 26, 2018 at 11:31 AM, Claudio Freire <klaussfre...@gmail.com>
> wrote:
>> On Mon, Feb 26, 2018 at 6:00 AM, Masahiko Sawada <sawada.m...@gmail.com>
>> wrote:
>>> Here is review comment for v4 patch.
>>>
>>> @@ -1922,6 +1988,8 @@ count_nondeletable_pages(Relation onerel,
>>> LVRelStats *vacrelstats)
>>> * We don't insert a vacuum delay point here, because we
>>> have an
>>> * exclusive lock on the table which we want to hold
>>> for as short a
>>> * time as possible. We still need to check for
>>> interrupts however.
>>> + * We might have to acquire the autovacuum lock,
>>> however, but that
>>> + * shouldn't pose a deadlock risk.
>>> */
>>> CHECK_FOR_INTERRUPTS();
>>>
>>> Is this change necessary?
>>
>> I don't recall doing that change. Maybe a rebase gone wrong.
>>
>>> ----
>>> + /*
>>> + * If there are no indexes then we should periodically
>>> vacuum the FSM
>>> + * on huge relations to make free space visible early.
>>> + */
>>> + if (nindexes == 0 &&
>>> + (vacuumed_pages - vacuumed_pages_at_fsm_vac) >
>>> vacuum_fsm_every_pages)
>>> + {
>>> + /* Vacuum the Free Space Map */
>>> + FreeSpaceMapVacuum(onerel, max_freespace);
>>> + vacuumed_pages_at_fsm_vac = vacuumed_pages;
>>> + max_freespace = 0;
>>> + }
>>>
>>> I think this code block should be executed before we check if the page
>>> is whether new or empty and then do 'continue'. Otherwise we cannot
>>> reach this code if the table has a lot of new or empty pages.
>>
>> In order for the counter (vacuumed_pages) to increase, there have to
>> be plenty of opportunities for this code to run, and I specifically
>> wanted to avoid vacuuming the FSM too often for those cases
>> particularly (when Vacuum scans lots of pages but does no writes).
>>
>>> ----
>>> @@ -663,6 +663,8 @@ fsm_extend(Relation rel, BlockNumber fsm_nblocks)
>>> * If minValue > 0, the updated page is also searched for a page with at
>>> * least minValue of free space. If one is found, its slot number is
>>> * returned, -1 otherwise.
>>> + *
>>> + * If minValue == 0, the value at the root node is returned.
>>> */
>>> static int
>>> fsm_set_and_search(Relation rel, FSMAddress addr, uint16 slot,
>>> @@ -687,6 +689,8 @@ fsm_set_and_search(Relation rel, FSMAddress addr,
>>> uint16 slot,
>>>
>>> addr.level == FSM_BOTTOM_LEVEL,
>>> true);
>>> }
>>> + else
>>> + newslot = fsm_get_avail(page, 0);
>>>
>>> I think it's not good idea to make fsm_set_and_search return either a
>>> slot number or a category value according to the argument. Category
>>> values is actually uint8 but this function returns it as int.
>>
>> Should be fine, uint8 can be contained inside an int in all platforms.
>>
>>> Also we
>>> can call fsm_set_and_search with minValue = 0 at
>>> RecordAndGetPageWithFreeSpace() when search_cat is 0. With you patch,
>>> fsm_set_and_search() then returns the root value but it's not correct.
>>
>> I guess I can add another function to do that.
>>
>>> Also, is this change necessary for this patch? ISTM this change is
>>> used for the change in fsm_update_recursive() as follows but I guess
>>> this change can be a separate patch.
>>>
>>> + new_cat = fsm_set_and_search(rel, parent, parentslot, new_cat, 0);
>>> +
>>> + /*
>>> + * Bubble up, not the value we just set, but the one now in the root
>>> + * node of the just-updated page, which is the page's highest value.
>>> + */
>>
>> I can try to separate them I guess.
>
> Patch set with the requested changes attached.
>
> 2 didn't change AFAIK, it's just rebased on top of the new 1.
Oops. Forgot about the comment changes requested. Fixed those in v6, attached.
From 3bde81cd9a0474e65c487899ddd290ad896b5feb Mon Sep 17 00:00:00 2001
From: Claudio Freire <klaussfre...@gmail.com>
Date: Fri, 28 Jul 2017 21:31:59 -0300
Subject: [PATCH 1/3] Vacuum: Update FSM more frequently
Make Vacuum update the FSM more frequently, to avoid the case where
autovacuum fails to reach the point where it updates the FSM in
highly contended tables.
Introduce a tree pruning threshold to FreeSpaceMapVacuum that avoids
recursing into branches that already contain enough free space, to
avoid having to traverse the whole FSM and thus induce quadratic
costs. Intermediate FSM vacuums are only supposed to make enough
free space visible to avoid extension until the final (non-partial)
FSM vacuum.
Run partial FSM vacuums after each index pass, which is a point
at which whole ranges of the heap have been thorougly cleaned, and
we can expect no further updates to those ranges of the FSM save
for concurrent activity. When there are no indexes, and thus no
index passes, run partial FSM vacuums every 8GB of dirtied pages
or 1/8th of the relation, whichever is highest. This allows some
partial work to be made visible without incurring quadratic cost.
In any case, FSM are small in relation to the table, so even when
quadratic cost is involved, it should not be problematic. Index
passes already incur quadratic cost, and the addition of the FSM
is unlikely to be measurable.
Run a partial FSM vacuum with a low pruning threshold right at
the beginning of the VACUUM to finish up any work left over from
prior canceled vacuum runs, something that is common in highly
contended tables when running only autovacuum.
---
src/backend/access/brin/brin.c | 2 +-
src/backend/access/brin/brin_pageops.c | 10 ++--
src/backend/commands/vacuumlazy.c | 83 ++++++++++++++++++++++++++++---
src/backend/storage/freespace/freespace.c | 27 ++++++++--
src/backend/storage/freespace/indexfsm.c | 2 +-
src/include/storage/freespace.h | 2 +-
6 files changed, 107 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 68b3371..24d6df7 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1461,5 +1461,5 @@ brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy)
* the way to the top.
*/
if (vacuum_fsm)
- FreeSpaceMapVacuum(idxrel);
+ FreeSpaceMapVacuum(idxrel, 0);
}
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
index 60a7025..d4c7a87 100644
--- a/src/backend/access/brin/brin_pageops.c
+++ b/src/backend/access/brin/brin_pageops.c
@@ -136,7 +136,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
brin_initialize_empty_new_buffer(idxrel, newbuf);
UnlockReleaseBuffer(newbuf);
if (extended)
- FreeSpaceMapVacuum(idxrel);
+ FreeSpaceMapVacuum(idxrel, 0);
}
return false;
}
@@ -156,7 +156,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
brin_initialize_empty_new_buffer(idxrel, newbuf);
UnlockReleaseBuffer(newbuf);
if (extended)
- FreeSpaceMapVacuum(idxrel);
+ FreeSpaceMapVacuum(idxrel, 0);
}
return false;
}
@@ -211,7 +211,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
LockBuffer(oldbuf, BUFFER_LOCK_UNLOCK);
if (extended)
- FreeSpaceMapVacuum(idxrel);
+ FreeSpaceMapVacuum(idxrel, 0);
return true;
}
@@ -313,7 +313,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
{
Assert(BlockNumberIsValid(newblk));
RecordPageWithFreeSpace(idxrel, newblk, freespace);
- FreeSpaceMapVacuum(idxrel);
+ FreeSpaceMapVacuum(idxrel, 0);
}
return true;
@@ -457,7 +457,7 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
LockBuffer(revmapbuf, BUFFER_LOCK_UNLOCK);
if (extended)
- FreeSpaceMapVacuum(idxrel);
+ FreeSpaceMapVacuum(idxrel, 0);
return off;
}
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index cf7f5e1..e300d26 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -85,6 +85,20 @@
#define VACUUM_TRUNCATE_LOCK_TIMEOUT 5000 /* ms */
/*
+ * When a table has no indexes, vacuum the FSM at most every 1/Nth
+ * of the relation has been vacuumed to prevent bloat during long-running
+ * vacuums. This specifies the N.
+ */
+#define VACUUM_FSM_EVERY_FRACTION 8
+
+/*
+ * When a table has no indexes, vacuum the FSM at most every this
+ * many dirty pages. With a default page size of 8kb, this value
+ * basically means 8GB of dirtied pages.
+ */
+#define VACUUM_FSM_EVERY_PAGES 1048576
+
+/*
* Guesstimation of number of dead tuples per page. This is used to
* provide an upper limit to memory allocated when vacuuming small
* tables.
@@ -146,7 +160,7 @@ static BufferAccessStrategy vac_strategy;
static void lazy_scan_heap(Relation onerel, int options,
LVRelStats *vacrelstats, Relation *Irel, int nindexes,
bool aggressive);
-static void lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats);
+static Size lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats);
static bool lazy_check_needs_freeze(Buffer buf, bool *hastup);
static void lazy_vacuum_index(Relation indrel,
IndexBulkDeleteResult **stats,
@@ -287,7 +301,7 @@ lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
PROGRESS_VACUUM_PHASE_FINAL_CLEANUP);
/* Vacuum the Free Space Map */
- FreeSpaceMapVacuum(onerel);
+ FreeSpaceMapVacuum(onerel, 0);
/*
* Update statistics in pg_class.
@@ -470,7 +484,9 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
TransactionId relfrozenxid = onerel->rd_rel->relfrozenxid;
TransactionId relminmxid = onerel->rd_rel->relminmxid;
BlockNumber empty_pages,
- vacuumed_pages;
+ vacuumed_pages,
+ fsm_updated_pages,
+ vacuum_fsm_every_pages;
double num_tuples,
tups_vacuumed,
nkeep,
@@ -480,6 +496,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
PGRUsage ru0;
Buffer vmbuffer = InvalidBuffer;
BlockNumber next_unskippable_block;
+ Size max_freespace = 0;
bool skipping_blocks;
xl_heap_freeze_tuple *frozen;
StringInfoData buf;
@@ -504,7 +521,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
get_namespace_name(RelationGetNamespace(onerel)),
relname)));
- empty_pages = vacuumed_pages = 0;
+ empty_pages = vacuumed_pages = fsm_updated_pages = 0;
num_tuples = tups_vacuumed = nkeep = nunused = 0;
indstats = (IndexBulkDeleteResult **)
@@ -517,6 +534,16 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
vacrelstats->nonempty_pages = 0;
vacrelstats->latestRemovedXid = InvalidTransactionId;
+ /*
+ * Vacuum the FSM a few times in the middle if the relation is big
+ * and has no indexes. Once every some amount of dirtied pages, or
+ * fraction of the relation, whatever is bigger, to avoid quadratic cost.
+ * If it has indexes, this is ignored, and the FSM is vacuumed after
+ * each index pass.
+ */
+ vacuum_fsm_every_pages = nblocks / VACUUM_FSM_EVERY_FRACTION;
+ vacuum_fsm_every_pages = Max(vacuum_fsm_every_pages, VACUUM_FSM_EVERY_PAGES);
+
lazy_space_alloc(vacrelstats, nblocks);
frozen = palloc(sizeof(xl_heap_freeze_tuple) * MaxHeapTuplesPerPage);
@@ -746,7 +773,9 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
pgstat_progress_update_multi_param(2, hvp_index, hvp_val);
/* Remove tuples from heap */
- lazy_vacuum_heap(onerel, vacrelstats);
+ freespace = lazy_vacuum_heap(onerel, vacrelstats);
+ if (freespace > max_freespace)
+ max_freespace = freespace;
/*
* Forget the now-vacuumed tuples, and press on, but be careful
@@ -756,12 +785,33 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
vacrelstats->num_dead_tuples = 0;
vacrelstats->num_index_scans++;
+ /*
+ * Vacuum the Free Space Map to make the changes we made visible.
+ * Don't recurse into branches with more than max_freespace,
+ * as we can't set it higher already.
+ */
+ FreeSpaceMapVacuum(onerel, max_freespace);
+ max_freespace = 0;
+
/* Report that we are once again scanning the heap */
pgstat_progress_update_param(PROGRESS_VACUUM_PHASE,
PROGRESS_VACUUM_PHASE_SCAN_HEAP);
}
/*
+ * If there are no indexes then we should periodically vacuum the FSM
+ * on huge relations to make free space visible early.
+ */
+ else if (nindexes == 0 && fsm_updated_pages > vacuum_fsm_every_pages)
+ {
+ /* Vacuum the Free Space Map */
+ FreeSpaceMapVacuum(onerel, max_freespace);
+ fsm_updated_pages = 0;
+ max_freespace = 0;
+ }
+
+
+ /*
* Pin the visibility map page in case we need to mark the page
* all-visible. In most cases this will be very cheap, because we'll
* already have the correct page pinned anyway. However, it's
@@ -877,7 +927,11 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
MarkBufferDirty(buf);
UnlockReleaseBuffer(buf);
+ fsm_updated_pages++;
RecordPageWithFreeSpace(onerel, blkno, freespace);
+ if (freespace > max_freespace)
+ max_freespace = freespace;
+
continue;
}
@@ -913,10 +967,14 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
vmbuffer, InvalidTransactionId,
VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
END_CRIT_SECTION();
+
}
UnlockReleaseBuffer(buf);
+ fsm_updated_pages++;
RecordPageWithFreeSpace(onerel, blkno, freespace);
+ if (freespace > max_freespace)
+ max_freespace = freespace;
continue;
}
@@ -1170,6 +1228,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
*/
vacrelstats->num_dead_tuples = 0;
vacuumed_pages++;
+ fsm_updated_pages++;
}
freespace = PageGetHeapFreeSpace(page);
@@ -1272,7 +1331,11 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
* taken if there are no indexes.)
*/
if (vacrelstats->num_dead_tuples == prev_dead_count)
+ {
RecordPageWithFreeSpace(onerel, blkno, freespace);
+ if (freespace > max_freespace)
+ max_freespace = freespace;
+ }
}
/* report that everything is scanned and vacuumed */
@@ -1392,13 +1455,16 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
* space on their pages. Pages not having dead tuples recorded from
* lazy_scan_heap are not visited at all.
*
+ * Returns the maximum amount of free space on vacuumed pages.
+ *
* Note: the reason for doing this as a second pass is we cannot remove
* the tuples until we've removed their index entries, and we want to
* process index entry removal in batches as large as possible.
*/
-static void
+static Size
lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
{
+ Size max_freespace = 0;
int tupindex;
int npages;
PGRUsage ru0;
@@ -1433,6 +1499,9 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
page = BufferGetPage(buf);
freespace = PageGetHeapFreeSpace(page);
+ if (freespace > max_freespace)
+ max_freespace = freespace;
+
UnlockReleaseBuffer(buf);
RecordPageWithFreeSpace(onerel, tblk, freespace);
npages++;
@@ -1449,6 +1518,8 @@ lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats)
RelationGetRelationName(onerel),
tupindex, npages),
errdetail_internal("%s", pg_rusage_show(&ru0))));
+
+ return max_freespace;
}
/*
diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index dd8ae52..ff79d71 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -108,7 +108,7 @@ static Size fsm_space_cat_to_avail(uint8 cat);
static int fsm_set_and_search(Relation rel, FSMAddress addr, uint16 slot,
uint8 newValue, uint8 minValue);
static BlockNumber fsm_search(Relation rel, uint8 min_cat);
-static uint8 fsm_vacuum_page(Relation rel, FSMAddress addr, bool *eof);
+static uint8 fsm_vacuum_page(Relation rel, FSMAddress addr, uint8 threshold, bool *eof);
static BlockNumber fsm_get_lastblckno(Relation rel, FSMAddress addr);
static void fsm_update_recursive(Relation rel, FSMAddress addr, uint8 new_cat);
@@ -374,9 +374,15 @@ FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks)
/*
* FreeSpaceMapVacuum - scan and fix any inconsistencies in the FSM
+ *
+ * If threshold is nonzero, a partial scan is made, skipping branches
+ * that already contain that much free space. Such a can be faster
+ * if many branches can be skipped, but it can't guarantee all
+ * inconsistencies will be fixed, only those under branches with less
+ * free space recorded than the threshold.
*/
void
-FreeSpaceMapVacuum(Relation rel)
+FreeSpaceMapVacuum(Relation rel, Size threshold)
{
bool dummy;
@@ -384,7 +390,7 @@ FreeSpaceMapVacuum(Relation rel)
* Traverse the tree in depth-first order. The tree is stored physically
* in depth-first order, so this should be pretty I/O efficient.
*/
- fsm_vacuum_page(rel, FSM_ROOT_ADDRESS, &dummy);
+ fsm_vacuum_page(rel, FSM_ROOT_ADDRESS, fsm_space_avail_to_cat(threshold), &dummy);
}
/******** Internal routines ********/
@@ -783,9 +789,12 @@ fsm_search(Relation rel, uint8 min_cat)
/*
* Recursive guts of FreeSpaceMapVacuum
+ *
+ * If threshold is nonzero, a partial scan is made, skipping branches
+ * that already contain that much free space recorded.
*/
static uint8
-fsm_vacuum_page(Relation rel, FSMAddress addr, bool *eof_p)
+fsm_vacuum_page(Relation rel, FSMAddress addr, uint8 threshold, bool *eof_p)
{
Buffer buf;
Page page;
@@ -816,11 +825,19 @@ fsm_vacuum_page(Relation rel, FSMAddress addr, bool *eof_p)
{
int child_avail;
+ /* Tree pruning for partial vacuums */
+ if (threshold)
+ {
+ child_avail = fsm_get_avail(page, slot);
+ if (child_avail >= threshold)
+ continue;
+ }
+
CHECK_FOR_INTERRUPTS();
/* After we hit end-of-file, just clear the rest of the slots */
if (!eof)
- child_avail = fsm_vacuum_page(rel, fsm_get_child(addr, slot), &eof);
+ child_avail = fsm_vacuum_page(rel, fsm_get_child(addr, slot), threshold, &eof);
else
child_avail = 0;
diff --git a/src/backend/storage/freespace/indexfsm.c b/src/backend/storage/freespace/indexfsm.c
index e21047b..dd77a16 100644
--- a/src/backend/storage/freespace/indexfsm.c
+++ b/src/backend/storage/freespace/indexfsm.c
@@ -70,5 +70,5 @@ RecordUsedIndexPage(Relation rel, BlockNumber usedBlock)
void
IndexFreeSpaceMapVacuum(Relation rel)
{
- FreeSpaceMapVacuum(rel);
+ FreeSpaceMapVacuum(rel, 0);
}
diff --git a/src/include/storage/freespace.h b/src/include/storage/freespace.h
index a517d7e..e8bdaa2 100644
--- a/src/include/storage/freespace.h
+++ b/src/include/storage/freespace.h
@@ -31,7 +31,7 @@ extern void XLogRecordPageWithFreeSpace(RelFileNode rnode, BlockNumber heapBlk,
Size spaceAvail);
extern void FreeSpaceMapTruncateRel(Relation rel, BlockNumber nblocks);
-extern void FreeSpaceMapVacuum(Relation rel);
+extern void FreeSpaceMapVacuum(Relation rel, Size threshold);
extern void UpdateFreeSpaceMap(Relation rel,
BlockNumber startBlkNum,
BlockNumber endBlkNum,
--
1.8.4.5
