Re: Using read stream in autoprewarm

Hi,

On Tue, 1 Apr 2025 at 05:14, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>
> On Mon, Mar 31, 2025 at 3:45 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> >
> > Whoops, this isn't right. It does work. I'm going to draft a version
> > suggesting slightly different variable naming and a couple comments to
> > make this more clear.
>
> Okay, so after further study, I think there are multiple issues still
> with the code. We could end up comparing a blocknumber to nblocks
> calculated from a different fork. To address this, you'll need to keep
> track of the last fork_number. At that point, you kind of have to
> bring back old_blk -- because that is what we are recreating with
> multiple local variables.

Yes, I realized the same.

> But, I think, overall, what we actually want to do is actually be
> really explicit about fast-forwarding in the failure cases (when we
> want to skip ahead because a relation is invalid or a fork is
> invalid). We were trying to use the main loop control and just add
> special cases to allow us to do this fast-forwarding. But, I think
> instead, we want to just go to a function or loop somewhere and fast
> forward through those bad blocks until we get to the next run of
> blocks from a different relation or fork.
>
> I've sketched out an idea like this in the attached. I don't think it
> is 100% correct. It does pass tests, but I think we might incorrectly
> advance pos twice after skipping a run of blocks belonging to a bad
> fork or relation -- and thus skip the first good block after some bad
> blocks.

The test actually does not pass, it prewarms 4 of 211 blocks. It
prewarms all 211 blocks in the master.
From 'pg_prewarm/001_basic/log/001_basic_main.log':
'autoprewarm successfully prewarmed 4 of 211 previously-loaded blocks'

> It also needs some more refactoring.
>
> maybe instead of having the skip code like this
> skip_forknumber:;
> while ((blk = next_record(block_info, &i)) != NULL &&
> blk->database == database && blk->filenumber == filenumber &&
> blk->forknum == forknum);
>
> we make it a function? to avoid the back-to-back while loop conditions
> (because of the outer do while loop).

+1 for using the functions. I think it is hard to follow / maintain
this with the do-while loops and goto statements.

> But the explicit looping for skipping the bad blocks and the nested
> loops for rel and fork -- I think these are less error prone.

One question in my mind is, the outermost loop stops when the database
changes, we do not check if it is changed from the database oid = 0.
Handling this might require some structural changes.

> What do you think?

I think what you said is right but the current version of the patch
looks more complicated to me. I may be biased because I do not like
do-while loops and goto statements.

--
Regards,
Nazir Bilal Yavuz
Microsoft