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

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. 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.

- Melanie