MIME-Version: 1.0
References: 
 <CA+HiwqFfAY_ZFqN8wcAEMw71T9hM_kA8UtyHaZZEZtuT3UyogA@mail.gmail.com>
 <68f3771f-91f5-4cb7-b1de-74d9abbf0b96@vondra.me>
 <CA+HiwqGqeS_94wxiYa8VpymiR_OtFPKDSpX+Me=MYWO45f5yig@mail.gmail.com>
 <dbce8def-e817-4e70-a9cb-e62574724d95@vondra.me>
 <CA+HiwqGM0ZTeVHicSkGnCp-2U-jvU-KBQCkPJ0N7nAj_c2LjZg@mail.gmail.com>
 <CAJDiXgh7vJAu39QkEk4TiJexPVsM6X6RXeNy7dNX9KjhX_5=9A@mail.gmail.com>
 <CA+HiwqED=0D1jQsYoXGuATxKr=Pxhm24Kwh01212KBAJ-bK9Bg@mail.gmail.com>
In-Reply-To: 
 <CA+HiwqED=0D1jQsYoXGuATxKr=Pxhm24Kwh01212KBAJ-bK9Bg@mail.gmail.com>
From: Daniil Davydov <3danissimo@gmail.com>
Date: Thu, 30 Oct 2025 19:12:03 +0700
Message-ID: 
 <CAJDiXgh6-JS3Z4L+HPhrvbp6v7hOrBOhJQrvfPG4g5f5dXZZ+w@mail.gmail.com>
Subject: Re: Batching in executor
To: Amit Langote <amitlangote09@gmail.com>
Cc: Tomas Vondra <tomas@vondra.me>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJDiXgh6-JS3Z4L%2BHPhrvbp6v7hOrBOhJQrvfPG4g5f5dXZZ%2Bw%40mail.gmail.com>
Precedence: bulk

Hi,

On Wed, Oct 29, 2025 at 9:23=E2=80=AFAM Amit Langote <amitlangote09@gmail.c=
om> wrote:
>
> Hi Daniil,
>
> On Tue, Oct 28, 2025 at 11:32=E2=80=AFPM Daniil Davydov <3danissimo@gmail=
.com> wrote:
> >
> > Hi,
> >
> > As far as I understand, this work partially overlaps with what we did i=
n the
> > thread [1] (in short - we introduce support for batching within the Mod=
ifyTable
> > 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 whethe=
r
pagemode is allowed at every call. If not - it just uses tuple-at-a-time mo=
de.
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 =3D scan->rs_cindex + dir;
            if (ScanDirectionIsForward(dir))
                linesleft =3D (lineindex <=3D (uint32) scan->rs_ntuples) ?
                    (scan->rs_ntuples - lineindex) : 0;
***

As far as I understand, this is enough :
***
        lineindex =3D scan->rs_cindex + dir;
        if (ScanDirectionIsForward(dir))
            linesleft =3D 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 =3D &(scan->rs_ctup) ---> HeapTuple tuple =3D &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 =3D {
    .materialize_all =3D heap_materialize_batch_all
};
***

[1] heap_getnextbatch : Assert(sscan->rs_flags & SO_ALLOW_PAGEMODE)

--
Best regards,
Daniil Davydov