Periodic FSM vacuum doesn't happen in one-pass strategy vacuum.
Hi,
With commit c120550edb86, If we got the cleanup lock on the page,
lazy_scan_prune() marks dead item IDs directly to LP_UNUSED. So the
following check with has_lpdead_items made the periodic FSM vacuum in
the one-pass strategy vacuum no longer being triggered:
if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
blkno);
next_fsm_block_to_vacuum = blkno;
}
Before c120550edb86, since we marked dead item IDs to LP_DEAD once
even in the one-pass strategy vacuum, we used to call
lazy_vacuum_heap_page() to vacuum the page and to call
FreeSpaceMapVacuum() periodically, so we had that check.
I've attached a patch to fix it.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Mon, Mar 31, 2025 at 6:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
With commit c120550edb86, If we got the cleanup lock on the page,
lazy_scan_prune() marks dead item IDs directly to LP_UNUSED. So the
following check with has_lpdead_items made the periodic FSM vacuum in
the one-pass strategy vacuum no longer being triggered:if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
blkno);
next_fsm_block_to_vacuum = blkno;
}Before c120550edb86, since we marked dead item IDs to LP_DEAD once
even in the one-pass strategy vacuum, we used to call
lazy_vacuum_heap_page() to vacuum the page and to call
FreeSpaceMapVacuum() periodically, so we had that check.
Whoops, yea, I had a feeling that something wasn't right here when I
saw that thread a couple weeks ago about FSM vacuuming taking forever
at the end of a vacuum and then I remember looking at the code and
thinking why is VACUUM_FSM_EVERY_PAGES named that if it only is used
when there are no indexes or a multi-pass vacuum.
I even made a comment in [1]/messages/by-id/CAAKRu_aXqoj2Vfqu3yzscsTX=2nPQ4y-aadCNz6nJP_o12GyxA@mail.gmail.com that we would only do FSM_EVERY_PAGES
when we have multi-pass vacuum -- which is even less after 17.
Isn't it ironic that I actually broke it.
I've attached a patch to fix it.
Looks like you forgot
- Melanie
[1]: /messages/by-id/CAAKRu_aXqoj2Vfqu3yzscsTX=2nPQ4y-aadCNz6nJP_o12GyxA@mail.gmail.com
On Mon, Mar 31, 2025 at 3:12 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Mon, Mar 31, 2025 at 6:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
With commit c120550edb86, If we got the cleanup lock on the page,
lazy_scan_prune() marks dead item IDs directly to LP_UNUSED. So the
following check with has_lpdead_items made the periodic FSM vacuum in
the one-pass strategy vacuum no longer being triggered:if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
blkno);
next_fsm_block_to_vacuum = blkno;
}Before c120550edb86, since we marked dead item IDs to LP_DEAD once
even in the one-pass strategy vacuum, we used to call
lazy_vacuum_heap_page() to vacuum the page and to call
FreeSpaceMapVacuum() periodically, so we had that check.Whoops, yea, I had a feeling that something wasn't right here when I
saw that thread a couple weeks ago about FSM vacuuming taking forever
at the end of a vacuum and then I remember looking at the code and
thinking why is VACUUM_FSM_EVERY_PAGES named that if it only is used
when there are no indexes or a multi-pass vacuum.I even made a comment in [1] that we would only do FSM_EVERY_PAGES
when we have multi-pass vacuum -- which is even less after 17.Isn't it ironic that I actually broke it.
I've attached a patch to fix it.
Looks like you forgot
Oops, attached now.
Looking the places related to VACUUM_FSM_EVERY_PAGES further, the
comment atop of vacuumlazy.c seems incorrect:
* In between phases, vacuum updates the freespace map (every
* VACUUM_FSM_EVERY_PAGES).
IIUC the context is two-pass strategy vacuum (i.e., the table has
indexes) and VACUUM_FSM_EVERY_PAGES is ignored in this case.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
fix_periodic_fsm_vacuum.patchapplication/octet-stream; name=fix_periodic_fsm_vacuum.patchDownload
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index f28326bad09..053252bfa69 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1475,7 +1475,7 @@ lazy_scan_heap(LVRelState *vacrel)
* table has indexes. There will only be newly-freed space if we
* held the cleanup lock and lazy_scan_prune() was called.
*/
- if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
+ if (got_cleanup_lock && vacrel->nindexes == 0
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
On Mon, Mar 31, 2025 at 3:29 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Mar 31, 2025 at 3:12 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:On Mon, Mar 31, 2025 at 6:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
With commit c120550edb86, If we got the cleanup lock on the page,
lazy_scan_prune() marks dead item IDs directly to LP_UNUSED. So the
following check with has_lpdead_items made the periodic FSM vacuum in
the one-pass strategy vacuum no longer being triggered:if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
blkno);
next_fsm_block_to_vacuum = blkno;
}Before c120550edb86, since we marked dead item IDs to LP_DEAD once
even in the one-pass strategy vacuum, we used to call
lazy_vacuum_heap_page() to vacuum the page and to call
FreeSpaceMapVacuum() periodically, so we had that check.Whoops, yea, I had a feeling that something wasn't right here when I
saw that thread a couple weeks ago about FSM vacuuming taking forever
at the end of a vacuum and then I remember looking at the code and
thinking why is VACUUM_FSM_EVERY_PAGES named that if it only is used
when there are no indexes or a multi-pass vacuum.I even made a comment in [1] that we would only do FSM_EVERY_PAGES
when we have multi-pass vacuum -- which is even less after 17.Isn't it ironic that I actually broke it.
I've attached a patch to fix it.
Looks like you forgot
Oops, attached now.
Looking the places related to VACUUM_FSM_EVERY_PAGES further, the
comment atop of vacuumlazy.c seems incorrect:* In between phases, vacuum updates the freespace map (every
* VACUUM_FSM_EVERY_PAGES).IIUC the context is two-pass strategy vacuum (i.e., the table has
indexes) and VACUUM_FSM_EVERY_PAGES is ignored in this case.
I've attached the patch with the commit message. I didn't include the
changes to the comment above in this patch so this is a pure bug
fixing patch.
I'm going to push this fix up to HEAD and v17 early next week, unless
there is no objection.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
0001-Fix-periodic-FSM-vacuum-not-triggering-in-one-pass-s.patchapplication/octet-stream; name=0001-Fix-periodic-FSM-vacuum-not-triggering-in-one-pass-s.patchDownload
From 3d88e92cc94766f9d30b819e59df6026dbffe4d6 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 4 Apr 2025 14:43:56 -0700
Subject: [PATCH] Fix periodic FSM vacuum not triggering in one-pass strategy
vacuum.
Since c120550edb86, we optimized the vacuuming of relations without
indexes by directly marking items as LP_UNUSED instead of LP_DEAD
during pruning. However, we continued to check has_lpdead_items in the
condition for periodic FSM vacuum (which occurs every
VACUUM_FSM_EVERY_PAGES).
This patch removes the has_lpdead_items check from that condition.
Backpatch to version 17, where we introduced the pruning optimization.
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAD21AoBL8m6B9GSzQfYxVaEgvD7-Kr3AJaS-hJPHC+avm-29zw@mail.gmail.com
Backpatch-through: 17
---
src/backend/access/heap/vacuumlazy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index f28326bad09..a163004da38 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1475,7 +1475,7 @@ lazy_scan_heap(LVRelState *vacrel)
* table has indexes. There will only be newly-freed space if we
* held the cleanup lock and lazy_scan_prune() was called.
*/
- if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
+ if (got_cleanup_lock && vacrel->nindexes == 0 &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
--
2.43.5
On Fri, Apr 4, 2025 at 6:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I'm going to push this fix up to HEAD and v17 early next week, unless
there is no objection.
I started studying this again looking back at the thread associated
with commit c120550edb86. There was actually a long discussion about
how this commit interacted with how often the freespace map was
vacuumed. [1]/messages/by-id/CA+TgmoYV5LFUgXS_g4S9P1AhQ63thPg6UJj8EsmsmEahFxLY_A@mail.gmail.com is one of the emails addressing that issue. If you grep
for FreeSpaceMapVacuumRange() on the thread, you can see the replies
to this topic, as they are interspersed with replies about updating
the FSM (as opposed to vacuuming it).
What I'm wondering is won't the patch you propose: simply removing
has_lpdead_items from
if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
lead to vacuuming the FSM when there is nothing to vacuum and thus to
wasting IO (when we didn't set anything to LP_UNUSED). It seems like
the logic we would want is to replace has_lpdead_items with something
about having set items lpunused.
Rereading that thread, it seems we discussed what the right logic for
this was extensively, but I don't quite understand how we ended up
with
if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
- Melanie
[1]: /messages/by-id/CA+TgmoYV5LFUgXS_g4S9P1AhQ63thPg6UJj8EsmsmEahFxLY_A@mail.gmail.com
On Mon, Apr 7, 2025 at 8:30 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Fri, Apr 4, 2025 at 6:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I'm going to push this fix up to HEAD and v17 early next week, unless
there is no objection.I started studying this again looking back at the thread associated
with commit c120550edb86. There was actually a long discussion about
how this commit interacted with how often the freespace map was
vacuumed. [1] is one of the emails addressing that issue. If you grep
for FreeSpaceMapVacuumRange() on the thread, you can see the replies
to this topic, as they are interspersed with replies about updating
the FSM (as opposed to vacuuming it).
Thank you for referring to the thread. I'll read through the discussion.
What I'm wondering is won't the patch you propose: simply removing
has_lpdead_items fromif (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)lead to vacuuming the FSM when there is nothing to vacuum and thus to
wasting IO (when we didn't set anything to LP_UNUSED). It seems like
the logic we would want is to replace has_lpdead_items with something
about having set items lpunused.
You're right. It would be better to replace it with something as you
suggested instead of just removing it. On the other hand, IIUC even if
there is no garbage on the table, we do FSM vacuum anyway at the end
of lazy_scan_heap(). Doing FSM vacuum periodically regardless of
presence of garbage might help load the disk I/O for FSM vacuum. But
we might think it is rather a bug that we do FSM vacuum even on an
all-frozen table.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Mon, Apr 7, 2025 at 11:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Mon, Apr 7, 2025 at 8:30 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:On Fri, Apr 4, 2025 at 6:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I'm going to push this fix up to HEAD and v17 early next week, unless
there is no objection.I started studying this again looking back at the thread associated
with commit c120550edb86. There was actually a long discussion about
how this commit interacted with how often the freespace map was
vacuumed. [1] is one of the emails addressing that issue. If you grep
for FreeSpaceMapVacuumRange() on the thread, you can see the replies
to this topic, as they are interspersed with replies about updating
the FSM (as opposed to vacuuming it).Thank you for referring to the thread. I'll read through the discussion.
What I'm wondering is won't the patch you propose: simply removing
has_lpdead_items fromif (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)lead to vacuuming the FSM when there is nothing to vacuum and thus to
wasting IO (when we didn't set anything to LP_UNUSED). It seems like
the logic we would want is to replace has_lpdead_items with something
about having set items lpunused.You're right. It would be better to replace it with something as you
suggested instead of just removing it. On the other hand, IIUC even if
there is no garbage on the table, we do FSM vacuum anyway at the end
of lazy_scan_heap(). Doing FSM vacuum periodically regardless of
presence of garbage might help load the disk I/O for FSM vacuum. But
we might think it is rather a bug that we do FSM vacuum even on an
all-frozen table.
I think that this issue presents since commit c120550edb86 but this
commit optimized the vacuum work for tables with no indexes and wasn't
intended to change the FSM vacuum behavior for such tables. Therefore,
I think we can fix the FSM vacuum to restore the original FSM vacuum
behavior for HEAD and v18, leaving aside the fact that we might want
to fix/improve VACUUM_FSM_EVERY_PAGES optimization . The original FSM
vacuum strategy for tables with no index was that we call
FreeSpaceMapVacuumRange() for every VACUUM_FSM_EVERY_PAGES, followed
by a final FreeSpaceMapVacuumRange() call for any remaining pages at
end of lazy_scan_heap():
/*
* Vacuum the remainder of the Free Space Map. We must do this whether or
* not there were indexes, and whether or not we bypassed index vacuuming.
* We can pass rel_pages here because we never skip scanning the last
* block of the relation.
*/
if (rel_pages > next_fsm_block_to_vacuum)
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
rel_pages);
Therefore, we do FSM vacuum for the entire table after all even if the
table has no garbage. As for VACUUM_FSM_EVERY_PAGES optimization, we
call FreeSpaceMapVacuumRange() only if we remove at least one tuple
from the page. I think these two points should have been maintained
post-commit. I've attached the patch implementing this idea.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
fix_periodic_fsm_vacuum_v2.patchapplication/octet-stream; name=fix_periodic_fsm_vacuum_v2.patchDownload
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 09416450af9..7c23eaf9a85 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -434,7 +434,8 @@ static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
static void lazy_scan_prune(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
Buffer vmbuffer, bool all_visible_according_to_vm,
- bool *has_lpdead_items, bool *vm_page_frozen);
+ bool *has_lpdead_items, bool *vm_page_frozen,
+ int *ndeleted);
static bool lazy_scan_noprune(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
bool *has_lpdead_items);
@@ -1245,6 +1246,7 @@ lazy_scan_heap(LVRelState *vacrel)
Buffer buf;
Page page;
uint8 blk_info = 0;
+ int ndeleted = 0;
bool has_lpdead_items;
void *per_buffer_data = NULL;
bool vm_page_frozen = false;
@@ -1390,7 +1392,7 @@ lazy_scan_heap(LVRelState *vacrel)
lazy_scan_prune(vacrel, buf, blkno, page,
vmbuffer,
blk_info & VAC_BLK_ALL_VISIBLE_ACCORDING_TO_VM,
- &has_lpdead_items, &vm_page_frozen);
+ &has_lpdead_items, &vm_page_frozen, &ndeleted);
/*
* Count an eagerly scanned page as a failure or a success.
@@ -1481,7 +1483,7 @@ lazy_scan_heap(LVRelState *vacrel)
* table has indexes. There will only be newly-freed space if we
* held the cleanup lock and lazy_scan_prune() was called.
*/
- if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
+ if (got_cleanup_lock && vacrel->nindexes == 0 && ndeleted > 0 &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
@@ -1946,6 +1948,8 @@ cmpOffsetNumbers(const void *a, const void *b)
* *vm_page_frozen is set to true if the page is newly set all-frozen in the
* VM. The caller currently only uses this for determining whether an eagerly
* scanned page was successfully set all-frozen.
+ *
+ * *ndeleted is the number of tuples deleted from the page during HOT pruning.
*/
static void
lazy_scan_prune(LVRelState *vacrel,
@@ -1955,7 +1959,8 @@ lazy_scan_prune(LVRelState *vacrel,
Buffer vmbuffer,
bool all_visible_according_to_vm,
bool *has_lpdead_items,
- bool *vm_page_frozen)
+ bool *vm_page_frozen,
+ int *ndeleted)
{
Relation rel = vacrel->rel;
PruneFreezeResult presult;
@@ -2063,6 +2068,9 @@ lazy_scan_prune(LVRelState *vacrel,
Assert(!presult.all_visible || !(*has_lpdead_items));
+ /* Remember the number of tuples deleted from the page for the caller */
+ *ndeleted = presult.ndeleted;
+
/*
* Handle setting visibility map bit based on information from the VM (as
* of last heap_vac_scan_next_block() call), and from all_visible and
On Mon, Jun 2, 2025 at 8:15 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I think that this issue presents since commit c120550edb86 but this
commit optimized the vacuum work for tables with no indexes and wasn't
intended to change the FSM vacuum behavior for such tables. Therefore,
I think we can fix the FSM vacuum to restore the original FSM vacuum
behavior for HEAD and v18, leaving aside the fact that we might want
to fix/improve VACUUM_FSM_EVERY_PAGES optimization . The original FSM
vacuum strategy for tables with no index was that we call
FreeSpaceMapVacuumRange() for every VACUUM_FSM_EVERY_PAGES, followed
by a final FreeSpaceMapVacuumRange() call for any remaining pages at
end of lazy_scan_heap():
Agreed. We should treat this as a bug fix and could separately reduce
unneeded FSM vacuuming.
Reviewing your patch, I think there might be an issue still. You
replaced has_lpdead_items with ndeleted. While ndeleted will count
those items we set LP_UNUSED (which is what we want), it also counts
LP_NORMAL items that vacuum sets LP_REDIRECT (see
heap_prune_record_redirect()).
Looking at PageGetHeapFreeSpace(), it seems like it only considers
LP_UNUSED items as freespace. So, if an item is set LP_REDIRECT, there
would be no need to update the FSM, I think.
I started to wonder why we counted setting an LP_NORMAL item
LP_REDIRECT as a "deleted" tuple. I refactored the code in
heap_prune_chain() in 17, adding heap_prune_record_redirect(), which
increments prstate->ndeleted if the root was LP_NORMAL.
This was the equivalent of the following heap_prune_chain() code in PG 16:
if (OffsetNumberIsValid(latestdead))
{
...
/*
* If the root entry had been a normal tuple, we are deleting it, so
* count it in the result. But changing a redirect (even to DEAD
* state) doesn't count.
*/
if (ItemIdIsNormal(rootlp))
ndeleted++;
/*
* If the DEAD tuple is at the end of the chain, the entire chain is
* dead and the root line pointer can be marked dead. Otherwise just
* redirect the root to the correct chain member.
*/
if (i >= nchain)
heap_prune_record_dead(prstate, rootoffnum);
else
heap_prune_record_redirect(prstate, rootoffnum, chainitems[i]);
}
So, I don't think I changed the behavior in 17 with my refactor. But I
don't know why we would want to count items set LP_REDIRECT as
"reclaimed".
I couldn't actually come up with a case where there was an LP_NORMAL
line pointer that vacuum set LP_REDIRECT and there were no intervening
line pointers in the chain that were being set LP_UNUSED. So, maybe it
is not a problem for this patch in practice.
I would like to understand why we count LP_NORMAL -> LP_REDIRECT items
in the deleted count -- even though that isn't the responsibility of
this patch.
- Melanie
On Mon, Jun 9, 2025 at 6:22 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
Reviewing your patch, I think there might be an issue still. You
replaced has_lpdead_items with ndeleted. While ndeleted will count
those items we set LP_UNUSED (which is what we want), it also counts
LP_NORMAL items that vacuum sets LP_REDIRECT (see
heap_prune_record_redirect()).Looking at PageGetHeapFreeSpace(), it seems like it only considers
LP_UNUSED items as freespace. So, if an item is set LP_REDIRECT, there
would be no need to update the FSM, I think.
I was wrong. Setting an item to LP_REDIRECT does free up space --
because there is no associated storage. PageGetFreeSpace() is
concerned with pd_upper and pd_lower. So after setting an item
LP_REDIRECT, we'll compactify_tuples() in PageRepairFragmentation(),
altering pd_upper/lower. Then future calls to PageGetHeapFreeSpace()
will report this updated number and we'll put that in the freespace
map. So, we can expect setting items LP_REDIRECT will result in new
freespace to report.
(I got confused by some code in PageGetHeapFreeSpace() that was
checking to make sure that, if there were >= MaxHeapTuplesPerPage line
pointers, that at least one of them is LP_UNUSED).
So, I think we should commit the fix you proposed.
The only question I have left is implementation: should we have
ndeleted as an output parameter of lazy_scan_prune() or have
lazy_scan_prune() return it (instead of void)?
In <= 16, heap_page_prune() returned the number of tuples deleted, so
I thought of maybe having lazy_scan_prune() do this. Though, maybe it
is confusing to have one result returned as the return value and the
others returned in output parameters unless there is something more
special about ndeleted. With heap_page_prune(), I think it was the
return value because that was kind of what heap_page_prune()
"accomplished".
- Melanie
On Tue, Jun 24, 2025 at 6:59 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
So, I think we should commit the fix you proposed.
The only question I have left is implementation: should we have
ndeleted as an output parameter of lazy_scan_prune() or have
lazy_scan_prune() return it (instead of void)?In <= 16, heap_page_prune() returned the number of tuples deleted, so
I thought of maybe having lazy_scan_prune() do this. Though, maybe it
is confusing to have one result returned as the return value and the
others returned in output parameters unless there is something more
special about ndeleted. With heap_page_prune(), I think it was the
return value because that was kind of what heap_page_prune()
"accomplished".
Hi Sawada-san,
Just checking what you thought about this. We probably want to get
this committed and backported relatively soon. I'm happy to help with
that if needed but just want to make sure we are on the same page
about the fix.
- Melanie
On Mon, Jun 30, 2025 at 10:20 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Jun 24, 2025 at 6:59 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:So, I think we should commit the fix you proposed.
The only question I have left is implementation: should we have
ndeleted as an output parameter of lazy_scan_prune() or have
lazy_scan_prune() return it (instead of void)?In <= 16, heap_page_prune() returned the number of tuples deleted, so
I thought of maybe having lazy_scan_prune() do this. Though, maybe it
is confusing to have one result returned as the return value and the
others returned in output parameters unless there is something more
special about ndeleted. With heap_page_prune(), I think it was the
return value because that was kind of what heap_page_prune()
"accomplished".Hi Sawada-san,
Just checking what you thought about this. We probably want to get
this committed and backported relatively soon. I'm happy to help with
that if needed but just want to make sure we are on the same page
about the fix.
Sorry for the late response, I was unable to work on this last week.
I'll check your reply and the solution tomorrow, and will get back to
you with my thoughts.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Mon, Jun 30, 2025 at 10:20 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Jun 24, 2025 at 6:59 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:So, I think we should commit the fix you proposed.
I agree with your analysis.
The only question I have left is implementation: should we have
ndeleted as an output parameter of lazy_scan_prune() or have
lazy_scan_prune() return it (instead of void)?In <= 16, heap_page_prune() returned the number of tuples deleted, so
I thought of maybe having lazy_scan_prune() do this. Though, maybe it
is confusing to have one result returned as the return value and the
others returned in output parameters unless there is something more
special about ndeleted. With heap_page_prune(), I think it was the
return value because that was kind of what heap_page_prune()
"accomplished".
Given your comment, I now think it makes sense to have
lazy_scan_prune() return the number of deleted tuples as one of the
main jobs of this function is to delete tuples by HOT pruning. I've
updated the patch accordingly.
Just checking what you thought about this. We probably want to get
this committed and backported relatively soon. I'm happy to help with
that if needed but just want to make sure we are on the same page
about the fix.
I've attached the updated patches for master and backbranches (v17 and
v18). Please review these patches.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
REL17_v3-0001-Fix-missing-FSM-vacuum-opportunities-on-tables-wi.patchapplication/octet-stream; name=REL17_v3-0001-Fix-missing-FSM-vacuum-opportunities-on-tables-wi.patchDownload
From b6f7ae27a2d7c8357758d9ae80823e1b3d5e0917 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 30 Jun 2025 20:29:52 -0700
Subject: [PATCH v3] Fix missing FSM vacuum opportunities on tables without
indexes.
Commit c120550edb86 optimized the vacuuming of relations without
indexes (a.k.a. one-pass strategy) by directly marking dead item IDs
as LP_UNUSED. However, the periodic FSM vacuum was still checking if
dead item IDs had been marked as LP_DEAD when attempting to vacuum the
FSM every VACUUM_FSM_EVERY_PAGES blocks. This condition was never met
due to the optimization, resulting in missed FSM vacuum
opportunities.
This commit modifies the periodic FSM vacuum condition to use the
number of tuples deleted during HOT pruning. This count includes items
marked as either LP_UNUSED or LP_REDIRECT, both of which are expected
to result in new free space to report.
Back-patch to v17 where the vacuum optimization for tables with no
indexes was introduced.
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAD21AoBL8m6B9GSzQfYxVaEgvD7-Kr3AJaS-hJPHC+avm-29zw@mail.gmail.com
Backpatch-through: 17
---
src/backend/access/heap/vacuumlazy.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index b22604e9600..426ebd37906 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -235,7 +235,7 @@ static void find_next_unskippable_block(LVRelState *vacrel, bool *skipsallvis);
static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
bool sharelock, Buffer vmbuffer);
-static void lazy_scan_prune(LVRelState *vacrel, Buffer buf,
+static int lazy_scan_prune(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
Buffer vmbuffer, bool all_visible_according_to_vm,
bool *has_lpdead_items);
@@ -844,6 +844,7 @@ lazy_scan_heap(LVRelState *vacrel)
{
Buffer buf;
Page page;
+ int ndeleted = 0;
bool has_lpdead_items;
bool got_cleanup_lock = false;
@@ -973,9 +974,9 @@ lazy_scan_heap(LVRelState *vacrel)
* line pointers previously marked LP_DEAD.
*/
if (got_cleanup_lock)
- lazy_scan_prune(vacrel, buf, blkno, page,
- vmbuffer, all_visible_according_to_vm,
- &has_lpdead_items);
+ ndeleted = lazy_scan_prune(vacrel, buf, blkno, page,
+ vmbuffer, all_visible_according_to_vm,
+ &has_lpdead_items);
/*
* Now drop the buffer lock and, potentially, update the FSM.
@@ -1011,7 +1012,7 @@ lazy_scan_heap(LVRelState *vacrel)
* table has indexes. There will only be newly-freed space if we
* held the cleanup lock and lazy_scan_prune() was called.
*/
- if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
+ if (got_cleanup_lock && vacrel->nindexes == 0 && ndeleted > 0 &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
@@ -1393,6 +1394,8 @@ cmpOffsetNumbers(const void *a, const void *b)
/*
* lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
*
+ * Return the number of tuples deleted from the page during HOT pruning.
+ *
* Caller must hold pin and buffer cleanup lock on the buffer.
*
* vmbuffer is the buffer containing the VM block with visibility information
@@ -1403,7 +1406,7 @@ cmpOffsetNumbers(const void *a, const void *b)
* *has_lpdead_items is set to true or false depending on whether, upon return
* from this function, any LP_DEAD items are still present on the page.
*/
-static void
+static int
lazy_scan_prune(LVRelState *vacrel,
Buffer buf,
BlockNumber blkno,
@@ -1623,6 +1626,8 @@ lazy_scan_prune(LVRelState *vacrel,
VISIBILITYMAP_ALL_VISIBLE |
VISIBILITYMAP_ALL_FROZEN);
}
+
+ return presult.ndeleted;
}
/*
--
2.43.5
master_v3-0001-Fix-missing-FSM-vacuum-opportunities-on-tables-wi.patchapplication/octet-stream; name=master_v3-0001-Fix-missing-FSM-vacuum-opportunities-on-tables-wi.patchDownload
From 1362d6eb2a0c51c57544656eb4009e9063c6de20 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 30 Jun 2025 20:29:52 -0700
Subject: [PATCH v3] Fix missing FSM vacuum opportunities on tables without
indexes.
Commit c120550edb86 optimized the vacuuming of relations without
indexes (a.k.a. one-pass strategy) by directly marking dead item IDs
as LP_UNUSED. However, the periodic FSM vacuum was still checking if
dead item IDs had been marked as LP_DEAD when attempting to vacuum the
FSM every VACUUM_FSM_EVERY_PAGES blocks. This condition was never met
due to the optimization, resulting in missed FSM vacuum
opportunities.
This commit modifies the periodic FSM vacuum condition to use the
number of tuples deleted during HOT pruning. This count includes items
marked as either LP_UNUSED or LP_REDIRECT, both of which are expected
to result in new free space to report.
Back-patch to v17 where the vacuum optimization for tables with no
indexes was introduced.
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAD21AoBL8m6B9GSzQfYxVaEgvD7-Kr3AJaS-hJPHC+avm-29zw@mail.gmail.com
Backpatch-through: 17
---
src/backend/access/heap/vacuumlazy.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 75979530897..c7f7089bf39 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -431,7 +431,7 @@ static void find_next_unskippable_block(LVRelState *vacrel, bool *skipsallvis);
static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
bool sharelock, Buffer vmbuffer);
-static void lazy_scan_prune(LVRelState *vacrel, Buffer buf,
+static int lazy_scan_prune(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
Buffer vmbuffer, bool all_visible_according_to_vm,
bool *has_lpdead_items, bool *vm_page_frozen);
@@ -1245,6 +1245,7 @@ lazy_scan_heap(LVRelState *vacrel)
Buffer buf;
Page page;
uint8 blk_info = 0;
+ int ndeleted = 0;
bool has_lpdead_items;
void *per_buffer_data = NULL;
bool vm_page_frozen = false;
@@ -1387,10 +1388,10 @@ lazy_scan_heap(LVRelState *vacrel)
* line pointers previously marked LP_DEAD.
*/
if (got_cleanup_lock)
- lazy_scan_prune(vacrel, buf, blkno, page,
- vmbuffer,
- blk_info & VAC_BLK_ALL_VISIBLE_ACCORDING_TO_VM,
- &has_lpdead_items, &vm_page_frozen);
+ ndeleted = lazy_scan_prune(vacrel, buf, blkno, page,
+ vmbuffer,
+ blk_info & VAC_BLK_ALL_VISIBLE_ACCORDING_TO_VM,
+ &has_lpdead_items, &vm_page_frozen);
/*
* Count an eagerly scanned page as a failure or a success.
@@ -1481,7 +1482,7 @@ lazy_scan_heap(LVRelState *vacrel)
* table has indexes. There will only be newly-freed space if we
* held the cleanup lock and lazy_scan_prune() was called.
*/
- if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
+ if (got_cleanup_lock && vacrel->nindexes == 0 && ndeleted > 0 &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
@@ -1923,6 +1924,8 @@ cmpOffsetNumbers(const void *a, const void *b)
/*
* lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
*
+ * Return the number of tuples deleted from the page during HOT pruning.
+ *
* Caller must hold pin and buffer cleanup lock on the buffer.
*
* vmbuffer is the buffer containing the VM block with visibility information
@@ -1937,7 +1940,7 @@ cmpOffsetNumbers(const void *a, const void *b)
* VM. The caller currently only uses this for determining whether an eagerly
* scanned page was successfully set all-frozen.
*/
-static void
+static int
lazy_scan_prune(LVRelState *vacrel,
Buffer buf,
BlockNumber blkno,
@@ -2208,6 +2211,8 @@ lazy_scan_prune(LVRelState *vacrel,
*vm_page_frozen = true;
}
}
+
+ return presult.ndeleted;
}
/*
--
2.43.5
REL18_v3-0001-Fix-missing-FSM-vacuum-opportunities-on-tables-wi.patchapplication/octet-stream; name=REL18_v3-0001-Fix-missing-FSM-vacuum-opportunities-on-tables-wi.patchDownload
From 1007518e206d8d4828ebbc430b81eca8611a86f3 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Mon, 30 Jun 2025 20:29:52 -0700
Subject: [PATCH v3] Fix missing FSM vacuum opportunities on tables without
indexes.
Commit c120550edb86 optimized the vacuuming of relations without
indexes (a.k.a. one-pass strategy) by directly marking dead item IDs
as LP_UNUSED. However, the periodic FSM vacuum was still checking if
dead item IDs had been marked as LP_DEAD when attempting to vacuum the
FSM every VACUUM_FSM_EVERY_PAGES blocks. This condition was never met
due to the optimization, resulting in missed FSM vacuum
opportunities.
This commit modifies the periodic FSM vacuum condition to use the
number of tuples deleted during HOT pruning. This count includes items
marked as either LP_UNUSED or LP_REDIRECT, both of which are expected
to result in new free space to report.
Back-patch to v17 where the vacuum optimization for tables with no
indexes was introduced.
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Discussion: https://postgr.es/m/CAD21AoBL8m6B9GSzQfYxVaEgvD7-Kr3AJaS-hJPHC+avm-29zw@mail.gmail.com
Backpatch-through: 17
---
src/backend/access/heap/vacuumlazy.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 4111a8996b5..2945d04fbb2 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -431,7 +431,7 @@ static void find_next_unskippable_block(LVRelState *vacrel, bool *skipsallvis);
static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
bool sharelock, Buffer vmbuffer);
-static void lazy_scan_prune(LVRelState *vacrel, Buffer buf,
+static int lazy_scan_prune(LVRelState *vacrel, Buffer buf,
BlockNumber blkno, Page page,
Buffer vmbuffer, bool all_visible_according_to_vm,
bool *has_lpdead_items, bool *vm_page_frozen);
@@ -1245,6 +1245,7 @@ lazy_scan_heap(LVRelState *vacrel)
Buffer buf;
Page page;
uint8 blk_info = 0;
+ int ndeleted = 0;
bool has_lpdead_items;
void *per_buffer_data = NULL;
bool vm_page_frozen = false;
@@ -1387,10 +1388,10 @@ lazy_scan_heap(LVRelState *vacrel)
* line pointers previously marked LP_DEAD.
*/
if (got_cleanup_lock)
- lazy_scan_prune(vacrel, buf, blkno, page,
- vmbuffer,
- blk_info & VAC_BLK_ALL_VISIBLE_ACCORDING_TO_VM,
- &has_lpdead_items, &vm_page_frozen);
+ ndeleted = lazy_scan_prune(vacrel, buf, blkno, page,
+ vmbuffer,
+ blk_info & VAC_BLK_ALL_VISIBLE_ACCORDING_TO_VM,
+ &has_lpdead_items, &vm_page_frozen);
/*
* Count an eagerly scanned page as a failure or a success.
@@ -1481,7 +1482,7 @@ lazy_scan_heap(LVRelState *vacrel)
* table has indexes. There will only be newly-freed space if we
* held the cleanup lock and lazy_scan_prune() was called.
*/
- if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
+ if (got_cleanup_lock && vacrel->nindexes == 0 && ndeleted > 0 &&
blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
{
FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
@@ -1923,6 +1924,8 @@ cmpOffsetNumbers(const void *a, const void *b)
/*
* lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
*
+ * Return the number of tuples deleted from the page during HOT pruning.
+ *
* Caller must hold pin and buffer cleanup lock on the buffer.
*
* vmbuffer is the buffer containing the VM block with visibility information
@@ -1937,7 +1940,7 @@ cmpOffsetNumbers(const void *a, const void *b)
* VM. The caller currently only uses this for determining whether an eagerly
* scanned page was successfully set all-frozen.
*/
-static void
+static int
lazy_scan_prune(LVRelState *vacrel,
Buffer buf,
BlockNumber blkno,
@@ -2208,6 +2211,8 @@ lazy_scan_prune(LVRelState *vacrel,
*vm_page_frozen = true;
}
}
+
+ return presult.ndeleted;
}
/*
--
2.43.5
On Tue, Jul 1, 2025 at 12:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached the updated patches for master and backbranches (v17 and
v18). Please review these patches.
All look good to me. One nitpick that is up to you if you want to
change: the comment
* Return the number of tuples deleted from the page during HOT pruning.
is at the top of the function block comment for lazy_scan_prune() in
your patch. Most of the function block comments in vacuumlazy.c list
the return value last after describing the other parameters and use
"Returns" as opposed to the imperative conjugation "Return".
Thanks so much for finding and fixing my bug!
- Melanie
On Wed, Jul 2, 2025 at 3:34 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Jul 1, 2025 at 12:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
I've attached the updated patches for master and backbranches (v17 and
v18). Please review these patches.All look good to me. One nitpick that is up to you if you want to
change: the comment* Return the number of tuples deleted from the page during HOT pruning.
is at the top of the function block comment for lazy_scan_prune() in
your patch. Most of the function block comments in vacuumlazy.c list
the return value last after describing the other parameters and use
"Returns" as opposed to the imperative conjugation "Return".
I've pushed the fix after incorporating your comments. Thank you for
reviewing the patch!
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com