Hi,
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)
--
Best regards,
Daniil Davydov
