Hi Daniil,
On Thu, Oct 30, 2025 at 9:12 PM Daniil Davydov <[email protected]> wrote: > On Wed, Oct 29, 2025 at 9:23 AM Amit Langote <[email protected]> wrote: > > > > Hi Daniil, > > > > On Tue, Oct 28, 2025 at 11:32 PM Daniil Davydov <[email protected]> > > 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
