Re: Fwd: [HACKERS] Avoid endless futile table locks in vacuuming.
Jeff Janes writes: > So like the attached, although it is a bit weird to call > lazy_check_needs_freeze if , under !scan_all, we don't actually care > about whether it needs freezing but only the hastup. I think this misses unpinning the buffer in the added code path. I rearranged to avoid that, did some other cosmetic work, and committed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Fwd: [HACKERS] Avoid endless futile table locks in vacuuming.
Jeff Janes writes: > On Dec 29, 2015 4:47 PM, "Tom Lane" wrote: >> Uh, isn't that what my patch is doing? > My reading was it does that only if there are no tuples that could be > frozen. If there are tuples that could be frozen, it actually does > the freezing, even though that is not necessary unless scan_all is > true. Ah, now I see. > So like the attached, although it is a bit weird to call > lazy_check_needs_freeze if , under !scan_all, we don't actually care > about whether it needs freezing but only the hastup. True, but this is such a corner case that it doesn't seem worth expending additional code to have a special-purpose page scan for it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Fwd: [HACKERS] Avoid endless futile table locks in vacuuming.
Forgetting to CC the list is starting to become a bad habit, sorry.
forwarding to list:
-- Forwarded message --
From: Jeff Janes
Date: Wed, Dec 30, 2015 at 10:03 AM
Subject: Re: [HACKERS] Avoid endless futile table locks in vacuuming.
To: Tom Lane
On Dec 29, 2015 4:47 PM, "Tom Lane" wrote:
>
> Jeff Janes writes:
> > If we are not doing a scan_all and we fail to acquire a clean-up lock on
> > the last block, and the last block reports that it needs freezing, then we
> > continue on to wait for the clean-up lock. But there is no need, we don't
> > really need to freeze the block, and we already know whether it has tuples
> > or not without the clean up lock. Couldn't we just set the flag based on
> > hastup, then 'continue'?
>
> Uh, isn't that what my patch is doing?
My reading was it does that only if there are no tuples that could be
frozen. If there are tuples that could be frozen, it actually does
the freezing, even though that is not necessary unless scan_all is
true.
So like the attached, although it is a bit weird to call
lazy_check_needs_freeze if , under !scan_all, we don't actually care
about whether it needs freezing but only the hastup.
Cheers,
Jeff
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index 2429889..e3cfa59
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*** static BufferAccessStrategy vac_strategy
*** 138,144
static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
Relation *Irel, int nindexes, bool scan_all);
static void lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats);
! static bool lazy_check_needs_freeze(Buffer buf);
static void lazy_vacuum_index(Relation indrel,
IndexBulkDeleteResult **stats,
LVRelStats *vacrelstats);
--- 138,144
static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
Relation *Irel, int nindexes, bool scan_all);
static void 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,
LVRelStats *vacrelstats);
*** static void lazy_cleanup_index(Relation
*** 147,152
--- 147,153
LVRelStats *vacrelstats);
static int lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer);
+ static bool should_attempt_truncation(LVRelStats *vacrelstats);
static void lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats);
static BlockNumber count_nondeletable_pages(Relation onerel,
LVRelStats *vacrelstats);
*** lazy_vacuum_rel(Relation onerel, int opt
*** 175,181
LVRelStats *vacrelstats;
Relation *Irel;
int nindexes;
- BlockNumber possibly_freeable;
PGRUsage ru0;
TimestampTz starttime = 0;
long secs;
--- 176,181
*** lazy_vacuum_rel(Relation onerel, int opt
*** 263,276
/*
* Optionally truncate the relation.
- *
- * Don't even think about it unless we have a shot at releasing a goodly
- * number of pages. Otherwise, the time taken isn't worth it.
*/
! possibly_freeable = vacrelstats->rel_pages - vacrelstats->nonempty_pages;
! if (possibly_freeable > 0 &&
! (possibly_freeable >= REL_TRUNCATE_MINIMUM ||
! possibly_freeable >= vacrelstats->rel_pages / REL_TRUNCATE_FRACTION))
lazy_truncate_heap(onerel, vacrelstats);
/* Vacuum the Free Space Map */
--- 263,270
/*
* Optionally truncate the relation.
*/
! if (should_attempt_truncation(vacrelstats))
lazy_truncate_heap(onerel, vacrelstats);
/* Vacuum the Free Space Map */
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 498,503
--- 492,504
* started skipping blocks, we may as well skip everything up to the next
* not-all-visible block.
*
+ * We don't skip the last page, unless we've already determined that it's
+ * not worth trying to truncate the table. This avoids having every
+ * vacuum take access exclusive lock on the table to attempt a truncation
+ * that just fails immediately (if there are tuples in the last page).
+ * This is worth avoiding mainly because such a lock must be replayed on
+ * any hot standby, where it can be disruptive.
+ *
* Note: if scan_all is true, we won't actually skip any pages; but we
* maintain next_not_all_visible_block anyway, so as to set up the
* all_visible_according_to_vm flag correctly for each page.
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 530,536
Page page;
OffsetNumber offnum,
maxoff;
! bool tupgone,
hastup;
int prev_d
Re: [HACKERS] Avoid endless futile table locks in vacuuming.
Jeff Janes writes: > If we are not doing a scan_all and we fail to acquire a clean-up lock on > the last block, and the last block reports that it needs freezing, then we > continue on to wait for the clean-up lock. But there is no need, we don't > really need to freeze the block, and we already know whether it has tuples > or not without the clean up lock. Couldn't we just set the flag based on > hastup, then 'continue'? Uh, isn't that what my patch is doing? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Avoid endless futile table locks in vacuuming.
On Mon, Dec 28, 2015 at 2:12 PM, Tom Lane wrote: > I wrote: >> Jeff Janes writes: >>> If a partially-active table develops a slug of stable all-visible, >>> non-empty pages at the end of it, then every autovacuum of that table >>> will skip the end pages on the forward scan, think they might be >>> truncatable, and take the access exclusive lock to do the truncation. >>> And then immediately fail when it sees the last page is not empty. >>> This can persist for an indefinite number of autovac rounds. >>> The simple solution is to always scan the last page of a table, so it >>> can be noticed that it is not empty and avoid the truncation attempt. > >> This seems like a reasonable proposal, but I find your implementation >> unconvincing: there are two places in lazy_scan_heap() that pay attention >> to scan_all, and you touched only one of them. > > After further investigation, there is another pre-existing bug: the short > circuit path for pages not requiring freezing doesn't bother to update > vacrelstats->nonempty_pages, causing the later logic to think that the > page is potentially truncatable even if we fix the second check of > scan_all! So this is pretty broken, and I almost think we should treat it > as a back-patchable bug fix. > > In the attached proposed patch, I added another refinement, which is to > not bother with forcing the last page to be scanned if we already know > that we're not going to attempt truncation, because we already found a > nonempty page too close to the end of the rel. I'm not quite sure this > is worthwhile, but it takes very little added logic, and saving an I/O > is probably worth the trouble. If we are not doing a scan_all and we fail to acquire a clean-up lock on the last block, and the last block reports that it needs freezing, then we continue on to wait for the clean-up lock. But there is no need, we don't really need to freeze the block, and we already know whether it has tuples or not without the clean up lock. Couldn't we just set the flag based on hastup, then 'continue'? Cheers, Jeff
Re: [HACKERS] Avoid endless futile table locks in vacuuming.
I wrote:
> Jeff Janes writes:
>> If a partially-active table develops a slug of stable all-visible,
>> non-empty pages at the end of it, then every autovacuum of that table
>> will skip the end pages on the forward scan, think they might be
>> truncatable, and take the access exclusive lock to do the truncation.
>> And then immediately fail when it sees the last page is not empty.
>> This can persist for an indefinite number of autovac rounds.
>> The simple solution is to always scan the last page of a table, so it
>> can be noticed that it is not empty and avoid the truncation attempt.
> This seems like a reasonable proposal, but I find your implementation
> unconvincing: there are two places in lazy_scan_heap() that pay attention
> to scan_all, and you touched only one of them.
After further investigation, there is another pre-existing bug: the short
circuit path for pages not requiring freezing doesn't bother to update
vacrelstats->nonempty_pages, causing the later logic to think that the
page is potentially truncatable even if we fix the second check of
scan_all! So this is pretty broken, and I almost think we should treat it
as a back-patchable bug fix.
In the attached proposed patch, I added another refinement, which is to
not bother with forcing the last page to be scanned if we already know
that we're not going to attempt truncation, because we already found a
nonempty page too close to the end of the rel. I'm not quite sure this
is worthwhile, but it takes very little added logic, and saving an I/O
is probably worth the trouble.
regards, tom lane
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 2429889..2833909 100644
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*** static BufferAccessStrategy vac_strategy
*** 138,144
static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
Relation *Irel, int nindexes, bool scan_all);
static void lazy_vacuum_heap(Relation onerel, LVRelStats *vacrelstats);
! static bool lazy_check_needs_freeze(Buffer buf);
static void lazy_vacuum_index(Relation indrel,
IndexBulkDeleteResult **stats,
LVRelStats *vacrelstats);
--- 138,144
static void lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
Relation *Irel, int nindexes, bool scan_all);
static void 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,
LVRelStats *vacrelstats);
*** static void lazy_cleanup_index(Relation
*** 147,152
--- 147,153
LVRelStats *vacrelstats);
static int lazy_vacuum_page(Relation onerel, BlockNumber blkno, Buffer buffer,
int tupindex, LVRelStats *vacrelstats, Buffer *vmbuffer);
+ static bool should_attempt_truncation(LVRelStats *vacrelstats);
static void lazy_truncate_heap(Relation onerel, LVRelStats *vacrelstats);
static BlockNumber count_nondeletable_pages(Relation onerel,
LVRelStats *vacrelstats);
*** lazy_vacuum_rel(Relation onerel, int opt
*** 175,181
LVRelStats *vacrelstats;
Relation *Irel;
int nindexes;
- BlockNumber possibly_freeable;
PGRUsage ru0;
TimestampTz starttime = 0;
long secs;
--- 176,181
*** lazy_vacuum_rel(Relation onerel, int opt
*** 263,276
/*
* Optionally truncate the relation.
- *
- * Don't even think about it unless we have a shot at releasing a goodly
- * number of pages. Otherwise, the time taken isn't worth it.
*/
! possibly_freeable = vacrelstats->rel_pages - vacrelstats->nonempty_pages;
! if (possibly_freeable > 0 &&
! (possibly_freeable >= REL_TRUNCATE_MINIMUM ||
! possibly_freeable >= vacrelstats->rel_pages / REL_TRUNCATE_FRACTION))
lazy_truncate_heap(onerel, vacrelstats);
/* Vacuum the Free Space Map */
--- 263,270
/*
* Optionally truncate the relation.
*/
! if (should_attempt_truncation(vacrelstats))
lazy_truncate_heap(onerel, vacrelstats);
/* Vacuum the Free Space Map */
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 498,503
--- 492,504
* started skipping blocks, we may as well skip everything up to the next
* not-all-visible block.
*
+ * We don't skip the last page, unless we've already determined that it's
+ * not worth trying to truncate the table. This avoids having every
+ * vacuum take access exclusive lock on the table to attempt a truncation
+ * that just fails immediately (if there are tuples in the last page).
+ * This is worth avoiding mainly because such a lock must be replayed on
+ * any hot standby, where it can be disruptive.
+ *
* Note: if scan_all is true, we won't actually skip any pages; but we
* maintain next_not_all_visible_block anyway, so as to set up th
Re: [HACKERS] Avoid endless futile table locks in vacuuming.
Jeff Janes writes: > If a partially-active table develops a slug of stable all-visible, > non-empty pages at the end of it, then every autovacuum of that table > will skip the end pages on the forward scan, think they might be > truncatable, and take the access exclusive lock to do the truncation. > And then immediately fail when it sees the last page is not empty. > This can persist for an indefinite number of autovac rounds. > This is not generally a problem, as the lock is taken conditionally. > However, the lock is also logged and passed over to any hot standbys, > where it must be replayed unconditionally. This can cause query > cancellations. > The simple solution is to always scan the last page of a table, so it > can be noticed that it is not empty and avoid the truncation attempt. This seems like a reasonable proposal, but I find your implementation unconvincing: there are two places in lazy_scan_heap() that pay attention to scan_all, and you touched only one of them. Thus, if we fail to acquire cleanup lock on the table's last page on the first try, the change is for naught. Shouldn't we be insisting on getting that lock? Or, if you intentionally didn't change that because it seems like too high a price to pay, why doesn't the comment reflect that? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Avoid endless futile table locks in vacuuming.
If a partially-active table develops a slug of stable all-visible,
non-empty pages at the end of it, then every autovacuum of that table
will skip the end pages on the forward scan, think they might be
truncatable, and take the access exclusive lock to do the truncation.
And then immediately fail when it sees the last page is not empty.
This can persist for an indefinite number of autovac rounds.
This is not generally a problem, as the lock is taken conditionally.
However, the lock is also logged and passed over to any hot standbys,
where it must be replayed unconditionally. This can cause query
cancellations.
The simple solution is to always scan the last page of a table, so it
can be noticed that it is not empty and avoid the truncation attempt.
We could add logic like doing this scan only if wal_level is
hot_standby or higher, or reproducing the REL_TRUNCATE_FRACTION logic
here to scan the last page only if truncation is eminent. But those
seem like needless complications to try to avoid sometimes scanning
one block.
Cheers,
Jeff
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
new file mode 100644
index 2429889..abc2e28
*** a/src/backend/commands/vacuumlazy.c
--- b/src/backend/commands/vacuumlazy.c
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 490,495
--- 490,501
* relfrozenxid, so we only want to do it if we can skip a goodly number
* of pages.
*
+ * We never skip the last page. This avoids having every vacuum take the
+ * access exclusive lock on the table to do a truncation which is doomed
+ * to fail in cases where a table has a stable series of all visible pages
+ * at the end. This is worth avoiding because the access exclusive lock
+ * must be replayed on any hot standby, where it can be disruptive.
+ *
* Before entering the main loop, establish the invariant that
* next_not_all_visible_block is the next block number >= blkno that's not
* all-visible according to the visibility map, or nblocks if there's no
*** lazy_scan_heap(Relation onerel, LVRelSta
*** 567,573
else
{
/* Current block is all-visible */
! if (skipping_all_visible_blocks && !scan_all)
continue;
all_visible_according_to_vm = true;
}
--- 573,579
else
{
/* Current block is all-visible */
! if (skipping_all_visible_blocks && !scan_all && blkno != nblocks-1)
continue;
all_visible_according_to_vm = true;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
