Обсуждение: Re: Simplify VM counters in vacuum code
On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> Hi,
>
> In dc6acfd910b8, I added some counters to track and log in
> autovacuum/vacuum output the number of pages newly set
> all-visible/frozen. Taking another look at the code recently, I
> realized the conditions for setting the counters could be simplified
> because of what we know to be true about the state of the heap page
> and VM at the time we are doing the counting.
>
Thank you for the patch! I could not understand the following change:
+ /* We know the page should not have been all-visible */
+ Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
+ (void) old_vmbits; /* Silence compiler */
+
+ /* Count the newly set VM page for logging */
+ if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0)
{
vacrel->vm_new_visible_pages++;
if (all_frozen)
vacrel->vm_new_visible_frozen_pages++;
}
The flags is initialized as:
uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
so the new if-condition is always true.
> Further explanation is in the attached patch. This code is only in 18/master.
The patch removes if statements and adds some assertions, which seems
to be a refactoring to me rather than a fix. I think we need to
consider whether it's really okay to apply it to v18.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Hi,
On Tue, 24 Jun 2025 at 07:13, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
Thank you for working on this!
> On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > Hi,
> >
> > In dc6acfd910b8, I added some counters to track and log in
> > autovacuum/vacuum output the number of pages newly set
> > all-visible/frozen. Taking another look at the code recently, I
> > realized the conditions for setting the counters could be simplified
> > because of what we know to be true about the state of the heap page
> > and VM at the time we are doing the counting.
> >
>
> Thank you for the patch! I could not understand the following change:
>
> + /* We know the page should not have been all-visible */
> + Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
> + (void) old_vmbits; /* Silence compiler */
> +
> + /* Count the newly set VM page for logging */
> + if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0)
> {
> vacrel->vm_new_visible_pages++;
> if (all_frozen)
> vacrel->vm_new_visible_frozen_pages++;
> }
>
> The flags is initialized as:
>
> uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
>
> so the new if-condition is always true.
I think we do not need to check visibility of the page here, as we
already know that page was not all-visible due to LP_DEAD items. We
can simply increment the vacrel->vm_new_visible_pages and check
whether the page is frozen.
--
Regards,
Nazir Bilal Yavuz
Microsoft
Thanks for the review! On Tue, Jun 24, 2025 at 12:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Tue, Jun 24, 2025 at 4:21 AM Melanie Plageman > <melanieplageman@gmail.com> wrote: > > The flags is initialized as: > > uint8 flags = VISIBILITYMAP_ALL_VISIBLE; > > so the new if-condition is always true. Yep, this was a mistake. I pulled this patch out of a larger set in which I moved setting these counters outside of the heap_page_is_all_visible() == true branch. Attached v2 fixes this. > The patch removes if statements and adds some assertions, which seems > to be a refactoring to me rather than a fix. I think we need to > consider whether it's really okay to apply it to v18. The reason I consider it a fix is that the if statement is confusing -- it makes the reader think it is possible that the VM page was already all-visible/frozen. In the other cases where we set the VM counters, that is true. But in the case of lazy_vacuum_heap_page(), it would not be correct for the page to have been all-visible because it contained LP_DEAD items. And in the case of an empty page where PD_ALL_VISIBLE was clear, the VM bits cannot have been set (because the page bit must be set if the VM bit is set). We could remove the asserts, as we rely on other code to enforce these invariants. So, here the asserts would only really be protecting from code changes that make it so the counters are no longer correctly counting newly all-visible pages -- which isn't critical to get right. - Melanie
Вложения
On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> > Thank you for the patch! I could not understand the following change:
> >
> > + /* We know the page should not have been all-visible */
> > + Assert((old_vmbits & VISIBILITYMAP_VALID_BITS) == 0);
> > + (void) old_vmbits; /* Silence compiler */
> > +
> > + /* Count the newly set VM page for logging */
> > + if ((flags & VISIBILITYMAP_ALL_VISIBLE) != 0)
> > {
> > vacrel->vm_new_visible_pages++;
> > if (all_frozen)
> > vacrel->vm_new_visible_frozen_pages++;
> > }
> >
> > The flags is initialized as:
> >
> > uint8 flags = VISIBILITYMAP_ALL_VISIBLE;
> >
> > so the new if-condition is always true.
Yep, I fixed that in the v2 patch I just sent.
> I think we do not need to check visibility of the page here, as we
> already know that page was not all-visible due to LP_DEAD items. We
> can simply increment the vacrel->vm_new_visible_pages and check
> whether the page is frozen.
My idea with the assert was sort of to codify the expectation that the
page couldn't have been all-visible because of the dead items. But
perhaps that is obvious. But you are right that the if statement is
not needed. Perhaps I ought to remove the asserts as they may be more
confusing than helpful.
- Melanie
On Tue, Jun 24, 2025 at 9:17 AM Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > > > I think we do not need to check visibility of the page here, as we > > already know that page was not all-visible due to LP_DEAD items. We > > can simply increment the vacrel->vm_new_visible_pages and check > > whether the page is frozen. > > My idea with the assert was sort of to codify the expectation that the > page couldn't have been all-visible because of the dead items. But > perhaps that is obvious. But you are right that the if statement is > not needed. Perhaps I ought to remove the asserts as they may be more > confusing than helpful. Thinking about this more, I think it is better without the asserts. I've done this in attached v3. - Melanie
Вложения
Hi, On Tue, 24 Jun 2025 at 18:12, Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Tue, Jun 24, 2025 at 9:17 AM Melanie Plageman > <melanieplageman@gmail.com> wrote: > > > > On Tue, Jun 24, 2025 at 4:01 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > > > > > I think we do not need to check visibility of the page here, as we > > > already know that page was not all-visible due to LP_DEAD items. We > > > can simply increment the vacrel->vm_new_visible_pages and check > > > whether the page is frozen. > > > > My idea with the assert was sort of to codify the expectation that the > > page couldn't have been all-visible because of the dead items. But > > perhaps that is obvious. But you are right that the if statement is > > not needed. Perhaps I ought to remove the asserts as they may be more > > confusing than helpful. > > Thinking about this more, I think it is better without the asserts. > I've done this in attached v3. I liked this version more. I agree that the asserts were causing some confusion. nitpick: + /* Count the newly all-frozen pages for logging. */ AFAIK, we do not use periods in the one line comments. Other than that, the patch looks good to me. -- Regards, Nazir Bilal Yavuz Microsoft
On Wed, Jun 25, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > > I liked this version more. I agree that the asserts were causing some confusion. Thanks for the review! Sawada-san, do you have any objection to this being merged in master/18? I know you had said changing the if statements to asserts did not feel like a bug fix. However, now that the asserts have been removed, do you still feel this way? I feel simplifying the counters is a clarity improvement and would prefer we have it before branching, but I would hold off if you still felt this way even with the asserts removed. (I had originally misunderstood and didn't realize that the page bit must be set if the VM bit is set, so that is why I had a guard in the empty page case. And for lazy_vacuum_heap_page(), I was being overly cautious at the expense of clarity.) > nitpick: > + /* Count the newly all-frozen pages for logging. */ > > AFAIK, we do not use periods in the one line comments. Other than > that, the patch looks good to me. Will fix. Thanks - Melanie
On Thu, Jun 26, 2025 at 10:01 PM Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Wed, Jun 25, 2025 at 2:59 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > > > > I liked this version more. I agree that the asserts were causing some confusion. > > Thanks for the review! > > Sawada-san, do you have any objection to this being merged in > master/18? I know you had said changing the if statements to asserts > did not feel like a bug fix. However, now that the asserts have been > removed, do you still feel this way? > > I feel simplifying the counters is a clarity improvement and would > prefer we have it before branching, but I would hold off if you still > felt this way even with the asserts removed. > > (I had originally misunderstood and didn't realize that the page bit > must be set if the VM bit is set, so that is why I had a guard in the > empty page case. And for lazy_vacuum_heap_page(), I was being overly > cautious at the expense of clarity.) Thank you for updating the patch! The changes in v3 appear straightforward; the patch eliminates unnecessary codes that were introduced in the original commit due to some misunderstandings. And I agreed with your answer[1] to my question. So it looks okay to me to push it to master/18. Regards, [1] https://www.postgresql.org/message-id/CAAKRu_ZFGLXCWkLZv2BT4SLbu%3D3zHPkfx5EeR%2BRupe%3DxCxLaPA%40mail.gmail.com -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On Thu, Jun 26, 2025 at 10:25 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > Thank you for updating the patch! The changes in v3 appear > straightforward; the patch eliminates unnecessary codes that were > introduced in the original commit due to some misunderstandings. And I > agreed with your answer[1] to my question. So it looks okay to me to > push it to master/18. Thanks so much for the reply. I've now pushed this. - Melanie