Hi,
On 2025-07-11 11:22:36 +0900, Amit Langote wrote:
> On Fri, Jul 11, 2025 at 5:55 AM Andres Freund <and...@anarazel.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...
> > > > 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.
> >
> > It's a good question. I think I unfortunately found a problematic case,
> > ForeignNext().
>
> Ah, so we do have a culprit in the tree.
>
> > 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().
Greetings,
Andres Freund
