On Thu, 2 Jan 2025 at 20:29, Matheus Alcantara <matheusssilv97@gmail.com> wrote:
>
> Hi,
>
> While reviewing some other patches implementing stream API for core subsystems,
> I noticed that the amcheck extension could also benefit from that.
>
> Notice the refactor when handling the "skip" parameter; The logic was moved to
> the heapam_read_stream_next_block callback so that verify_heapam don't need to
> touch any private field of heapam_read_stream_next_block_private struct.
>
> One other think to mention is that the test cases of "skip" parameter
> that I've seen just test when the first page is corrupted, so I think
> that a carefully review on callback logic would be good to ensure that
> we don't accidentally skip a page when doing p->current_blocknum++;
>
> This patch doesn't show any performance improvements (or regression)
> but I think that it would be good to replace the ReadBufferExtended
> usage with the read stream API, so in the future it could be benefit
> from the AIO project.
>
> --
> Matheus Alcantara
Hi!
+1 on idea
However, this:
>- if (skip_option == SKIP_PAGES_ALL_FROZEN)
>- {
>- if ((mapbits & VISIBILITYMAP_ALL_FROZEN) != 0)
>- continue;
>- }
>-
>- if (skip_option == SKIP_PAGES_ALL_VISIBLE)
>- {
>- if ((mapbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
>- continue;
>- }
changes to this
(in heapam_read_stream_next_block)
>+
>+ if ((p->skip_option == SKIP_PAGES_ALL_FROZEN && (mapbts & VISIBILITYMAP_ALL_FROZEN) != 0) ||
>+ (p->skip_option == SKIP_PAGES_ALL_VISIBLE && (mapbts & VISIBILITYMAP_ALL_VISIBLE) != 0))
> + continue;
I don't understand this change. The patch aims to be purely applying
streaming API, not refactoring. And if we refactor this code, this is
less readable than it was.
Other than that, LGTM.
--
Best regards,
Kirill Reshke
