Re: Batching in executor

Hi Daniil,


On Thu, Oct 30, 2025 at 9:12 PM Daniil Davydov <3danissimo@gmail.com> wrote:
> On Wed, Oct 29, 2025 at 9:23 AM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > Hi Daniil,
> >
> > On Tue, Oct 28, 2025 at 11:32 PM Daniil Davydov <3danissimo@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > As far as I understand, this work partially overlaps with what we did in the
> > > thread [1] (in short - we introduce support for batching within the ModifyTable
> > > node). Am I correct?
> >
> > There might be some relation, but not much overlap. The thread you
> > mention seems to focus on batching in the write path (for INSERT,
> > etc.), while this work targets batching in the read path via Table AM
> > scan callbacks. I think they can be developed independently, though
> > I'm happy to take a look.
>
> Oh, I got it. Thanks!
>
> I looked at 0001-0003 patches and got some comments :
> 1)
> I noticed that some Nodes may set SO_ALLOW_PAGEMODE flag to 'false'
> during ExecReScan. heap_getnextslot works carefully with it - checks whether
> pagemode is allowed at every call. If not - it just uses tuple-at-a-time mode.
> At the same time, heap_getnextbatch always expects that pagemode is enabled.
> I didn't find any code paths which can lead to an assertion [1] fail.
> If such a code
> path is unreachable under any circumstances, maybe we should add a comment
> why?
>
> 2)
> heapgettup_pagemode_batch : Do we really need to compute lineindex variable
> in this way? :
> ***
>             lineindex = scan->rs_cindex + dir;
>             if (ScanDirectionIsForward(dir))
>                 linesleft = (lineindex <= (uint32) scan->rs_ntuples) ?
>                     (scan->rs_ntuples - lineindex) : 0;
> ***
>
> As far as I understand, this is enough :
> ***
>         lineindex = scan->rs_cindex + dir;
>         if (ScanDirectionIsForward(dir))
>             linesleft = scan->rs_ntuples - lineindex;
> ***
>
> 3)
> Is this code inside heapgettup_pagemode_batch necessary? :
> ***
> ScanDirectionIsForward(dir) ? 0 : 0
> ***
>
> 4)
> heapgettup_pagemode has this change :
> HeapTuple    tuple = &(scan->rs_ctup) ---> HeapTuple tuple = &scan->rs_ctup
> I guess it was changed accidentally.
>
> 5)
> I apologize for the tediousness, but these braces are not in the
> postgres style :
> ***
> static const TupleBatchOps TupleBatchHeapOps = {
>     .materialize_all = heap_materialize_batch_all
> };
> ***
>
> [1] heap_getnextbatch : Assert(sscan->rs_flags & SO_ALLOW_PAGEMODE)

Thanks for the review and apologies for getting to them so late.

I think I've addressed your comments in v4 that I just posted.

--
Thanks, Amit Langote