On Thu, Oct 24, 2024 at 7:11 PM Melanie Plageman <melanieplage...@gmail.com> wrote:
> Thanks for the reply, Dilip! > > On Thu, Oct 24, 2024 at 12:50 AM Dilip Kumar <dilipbal...@gmail.com> > wrote: > > > > On Thu, Oct 24, 2024 at 3:45 AM Melanie Plageman < > melanieplage...@gmail.com> wrote: > >> > >> HeapScanDescData->rs_cindex (the current index into the array of > >> visible tuples stored in the heap scan descriptor) is used for > >> multiple scan types, but for bitmap heap scans, AFAICT, it should > >> never be < 0. > > > > You are right it can never be < 0 in this case at least. > > Cool, thanks for confirming. I will commit this change then. > +1 > > > In fact you don't need to explicitly set it to 0 in initscan[1], because > before calling heapam_scan_bitmap_next_tuple() we must call > heapam_scan_bitmap_next_block() and this function is initializing this to 0 > (hscan->rs_cindex = 0;). Anyway no object even if you prefer to initialize > in initscan(), just the point is that we are already doing it for each > block before fetching tuples from the block. > > I am inclined to still initialize it to 0 in initscan(). I was > refactoring the bitmap heap scan code to use the read stream API and > after moving some things around, this field was no longer getting > initialized in heapam_scan_bitmap_next_block(). While that may not be > a good reason to change it in this patch, most of the other fields in > the heap scan descriptor (like rs_cbuf and rs_cblock) are > re-initialized in initscan(), so it seems okay to do that there even > though it isn't strictly necessary on master. > make sense -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
