Re: Assert("vacrel->eager_scan_remaining_successes > 0")

On Thu, May 22, 2025 at 7:27 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Wed, May 21, 2025 at 6:11 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > > if (vacrel->eager_scan_remaining_successes > 0)
> > > vacrel->eager_scan_remaining_successes--;
> >
> > I've attached a patch that uses this idea. Feedback is very welcome.
>
> Thanks for writing the patch!
>
> I actually think we have the same situation with
> eager_scan_remaining_fails.

Good catch.

> Since the extra pages that are eagerly
> scanned could either fail or succeed to be frozen, so we probably also
> need to change the assert in the failure case into a guard as well:
>
> else
> {
> Assert(vacrel->eager_scan_remaining_fails > 0);
> vacrel->eager_scan_remaining_fails--;
> }
>
> ->
>
> else if (vacrel->eager_scan_remaining_fails > 0)
> vacrel->eager_scan_remaining_fails--;
>
> In the comment you wrote, I would probably just change one thing
>
> + /*
> + * Report only once that we disabled eager scanning. This
> + * check is required because we might have eagerly read
> + * more blocks and we could reach here even after
> + * disabling eager scanning.
> + */
>
> I would emphasize that we read ahead these blocks before executing the
> code trying to freeze them. So, I might instead say something like:
> "Report only once that we disabled eager scanning. We may eagerly read
> ahead blocks in excess of the success or failure caps before
> attempting to freeze them, so we could reach here even after disabling
> additional eager scanning"
>
> And then probably avoid repeating the whole comment above the
> remaining fails guard.

Agreed. I've updated the patch. Does this address your comments?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com