public inbox for [email protected]  

help / color / mirror / Atom feed

From: Amit Langote <[email protected]>
To: Daniil Davydov <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Batching in executor
Date: Sat, 20 Dec 2025 23:36:13 +0900
Message-ID: <CA+HiwqFGo-QHirV8dkYWA+tkRDujnGetMLWzLen9B7+MtsW-Xg@mail.gmail.com> (raw)
In-Reply-To: <CAJDiXgh6-JS3Z4L+HPhrvbp6v7hOrBOhJQrvfPG4g5f5dXZZ+w@mail.gmail.com>
References: <CA+HiwqFfAY_ZFqN8wcAEMw71T9hM_kA8UtyHaZZEZtuT3UyogA@mail.gmail.com>
	<[email protected]>
	<CA+HiwqGqeS_94wxiYa8VpymiR_OtFPKDSpX+Me=MYWO45f5yig@mail.gmail.com>
	<[email protected]>
	<CA+HiwqGM0ZTeVHicSkGnCp-2U-jvU-KBQCkPJ0N7nAj_c2LjZg@mail.gmail.com>
	<CAJDiXgh7vJAu39QkEk4TiJexPVsM6X6RXeNy7dNX9KjhX_5=9A@mail.gmail.com>
	<CA+HiwqED=0D1jQsYoXGuATxKr=Pxhm24Kwh01212KBAJ-bK9Bg@mail.gmail.com>
	<CAJDiXgh6-JS3Z4L+HPhrvbp6v7hOrBOhJQrvfPG4g5f5dXZZ+w@mail.gmail.com>

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

view thread (22+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Batching in executor
  In-Reply-To: <CA+HiwqFGo-QHirV8dkYWA+tkRDujnGetMLWzLen9B7+MtsW-Xg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox