Re: Some ExecSeqScan optimizations

Hi,

On Fri, Jul 11, 2025 at 11:34 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2025-07-11 11:22:36 +0900, Amit Langote wrote:
> > On Fri, Jul 11, 2025 at 5:55 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2025-07-10 17:28:50 +0900, Amit Langote wrote:
> > > > Thanks for the patch.
> > > >
> > > > + /*
> > > > + * Use pg_assume() for != NULL tests to make the compiler realize no
> > > > + * runtime check for the field is needed in ExecScanExtended().
> > > > + */
> > > >
> > > > I propose changing "to make the compiler realize no runtime check" to
> > > > "so the compiler can optimize away the runtime check", assuming that
> > > > is what it means.
> > >
> > > It does. I don't really see a meaningful difference between the comments?
> >
> > Maybe not. I just had to pause for a moment to be sure that was what
> > it actually meant when I first read it. I'm fine leaving it as is if
> > you prefer.
>
> To me my version makes a bit more sense, by explaining that we tell the
> compiler information that it otherwise doesn't have, which results in the
> optimization...

Ok, that does make sense.

> > > > > I have a separate question as well - do we need to call ResetExprContext() if
> > > > > we neither qual, projection nor epq? I see a small gain by avoiding that.
> > > >
> > > > You're referring to this block, I assume:
> > > >
> > > > /*
> > > > * If we have neither a qual to check nor a projection to do, just skip
> > > > * all the overhead and return the raw scan tuple.
> > > > */
> > > > if (!qual && !projInfo)
> > > > {
> > > > ResetExprContext(econtext);
> > > > return ExecScanFetch(node, epqstate, accessMtd, recheckMtd);
> > > > }
> > >
> > > Yep.
> > >
> > >
> > > > Yeah, I think it's fine to remove ResetExprContext() here. When I
> > > > looked at it before, I left it in because I was unsure whether
> > > > accessMtd() might leak memory into the per-tuple context.
> > >
> > > I wonder if we instead can MemoryContextReset cheaper, by avoiding a function
> > > call for the common case that no reset is needed. Right now we can't just
> > > check ->isReset in an inline function, because we also delete children. I
> > > wonder if we could define isReset so that creating a child context unsets
> > > isReset?
> >
> > Were you thinking ResetExprContext() could become something like:
> >
> > #define ResetExprContext(econtext) \
> > do { \
> > if (!((econtext)->ecxt_per_tuple_memory)->isReset) \
> > MemoryContextReset((econtext)->ecxt_per_tuple_memory); \
> > } while (0)
> >
> > that is, once isReset also accounts for whether any child context exists?
>
> Nearly - I was thinking we'd do that in MemoryContextReset(), rather than
> ResetExprContext().

Ah, ok -- I was confused about which function you meant ("can't just
check ->isReset in an inline function" should have been a clue). I
thought you were referring to avoiding the call to
MemoryContextReset() itself from ExecScanExtended() by checking
isReset.

But it sounds like you meant optimizing within MemoryContextReset() --
specifically, skipping MemoryContextDeleteChildren() when isReset is
already true, so it becomes:

if (context->isReset)
return;
MemoryContextDeleteChildren(context);
MemoryContextResetOnly(context);

Just out of curiosity, I tried making that change locally, and meson
test (check-world) passed. I assume that's just because nothing
notices leaked child contexts -- there's no mechanism asserting that
everything under a context gets reset if we skip
MemoryContextDeleteChildren().

That’s not to say we don't need MemoryContextCreate() to clear isReset
in the parent when adding a child. :-)

--
Thanks, Amit Langote