On Thu, May 22, 2025 at 7:27 AM Melanie Plageman <melanieplage...@gmail.com> wrote: > > On Wed, May 21, 2025 at 6:11 PM Masahiko Sawada <sawada.m...@gmail.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
v2-0001-Fix-assertion-when-decrementing-eager-scanning-su.patch
Description: Binary data
