Re: BitmapHeapScan streaming read user and prelim refactoring

On Sat, Apr 06, 2024 at 12:04:23PM -0400, Melanie Plageman wrote:
> On Sat, Apr 06, 2024 at 04:57:51PM +0200, Tomas Vondra wrote:
> > On 4/6/24 15:40, Melanie Plageman wrote:
> > > On Sat, Apr 06, 2024 at 02:51:45AM +0200, Tomas Vondra wrote:
> > >>
> > >>
> > >> On 4/6/24 01:53, Melanie Plageman wrote:
> > >>> On Fri, Apr 05, 2024 at 04:06:34AM -0400, Melanie Plageman wrote:
> > >>>> On Thu, Apr 04, 2024 at 04:35:45PM +0200, Tomas Vondra wrote:
> > >>>>> On 4/4/24 00:57, Melanie Plageman wrote:
> > >>>>>> On Sun, Mar 31, 2024 at 11:45:51AM -0400, Melanie Plageman wrote:
> > >>>>> I'd focus on the first ~8-9 commits or so for now, we can commit more if
> > >>>>> things go reasonably well.
> > >>>>
> > >>>> Sounds good. I will spend cleanup time on 0010-0013 tomorrow but would
> > >>>> love to know if you agree with the direction before I spend more time.
> > >>>
> > >>> In attached v16, I've split out 0010-0013 into 0011-0017. I think it is
> > >>> much easier to understand.
> > >>>
> > >>
> > >> Anyway, I've attached it as .tgz in order to not confuse cfbot. All the
> > >> review comments are marked with XXX, so grep for that in the patches.
> > >> There's two separate patches - the first one suggests a code change, so
> > >> it was better to not merge that with your code. The second has just a
> > >> couple XXX comments, I'm not sure why I kept it separate.
> > >>
> > >> A couple review comments:
> > >>
> > >> * I think 0001-0009 are 99% ready to. I reworded some of the commit
> > >> messages a bit - I realize it's a bit bold, considering you're native
> > >> speaker and I'm not. If you could check I didn't make it worse, that
> > >> would be great.
> > > 
> > > Attached v17 has *only* patches 0001-0009 with these changes. I will
> > > work on applying the remaining patches, addressing feedback, and adding
> > > comments next.
> > > 
> > > I have reviewed and incorporated all of your feedback on these patches.
> > > Attached v17 is your exact patches with 1 or 2 *very* slight tweaks to
> > > commit messages (comma splice removal and single word adjustments) as
> > > well as the changes listed below:
> > > 
> > > I have open questions on the following:
> > > 
> > > - 0003: should it be SO_NEED_TUPLES and need_tuples (instead of
> > > 	SO_NEED_TUPLE and need_tuple)?
> > > 
> > 
> > I think SO_NEED_TUPLES is more accurate, as we need all tuples from the
> > block. But either would work.
> 
> Attached v18 changes it to TUPLES/tuples
> 
> > 
> > > - 0009 (your 0010)
> > > 	- Should I mention in the commit message that we added blockno and
> > > 		pfblockno in the BitmapHeapScanState only for validation or is that
> > > 		too specific?
> > > 
> > 
> > For the commit message I'd say it's too specific, I'd put it in the
> > comment before the struct.
> 
> It is in the comment for the struct
> 
> > > 	- I'm worried this comment is vague and or potentially not totally
> > > 		correct. Should we remove it? I don't think we have conclusive proof
> > > 		that this is true.
> > >   /*
> > >    * Adjusting the prefetch iterator before invoking
> > >    * table_scan_bitmap_next_block() keeps prefetch distance higher across
> > >    * the parallel workers.
> > >    */
> > > 
> > 
> > TBH it's not clear to me what "higher across parallel workers" means.
> > But it sure shouldn't claim things that we think may not be correct. I
> > don't have a good idea how to reword it, though.
> 
> I realized it makes more sense to add a FIXME (I used XXX. I'm not when
> to use what) with a link to the message where Andres describes why he
> thinks it is a bug. If we plan on fixing it, it is good to have a record
> of that. And it makes it easier to put a clear and accurate comment.
> Done in 0009.
> 
> > OK, thanks. If think 0001-0008 are ready to go, with some minor tweaks
> > per above (tuple vs. tuples etc.), and the question about the recheck
> > flag. If you can do these tweaks, I'll get that committed today and we
> > can try to get a couple more patches in tomorrow.

Attached v19 rebases the rest of the commits from v17 over the first
nine patches from v18. All patches 0001-0009 are unchanged from v18. I
have made updates and done cleanup on 0010-0021.

- Melanie