Re: make tuplestore helper function

On Wed, Jan 05, 2022 at 12:09:16PM -0500, Melanie Plageman wrote:
> On Fri, Dec 17, 2021 at 3:04 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > The rest of these are questions more than comments, and I'm not necessarily
> > suggesting to change the patch:
> >
> > This isn't new in your patch, but for me, it's more clear if the callers have a
> > separate boolean for randomAccess, rather than having the long expression in
> > the function call:
> >
> > +       tupstore = MakeFuncResultTuplestore(fcinfo, NULL,
> > +               rsinfo->allowedModes & SFRM_Materialize_Random);
> >
> > vs
> >
> >         randomAccess = (rsinfo->allowedModes & SFRM_Materialize_Random) != 0;
> > -       tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> > +       tupstore = MakeFuncResultTuplestore(fcinfo, NULL, randomAccess);
> >
> > One could also argue for passing rsinfo->allowedModes, instead of
> > (rsinfo->allowedModes & SFRM_Materialize_Random).
> 
> I've changed the patch to have MakeFuncResultTuplestore() check
> rsinfo->allowedModes and pass in the resulting boolean to
> tuplestore_begin_heap(). Because I modified the
> MakeFuncResultTuplestore() function signature to accommodate this, I had
> to collapse the two patches into one, as I couldn't maintain the call
> sites which passed in a constant.

> > There's a couples places that you're checking expectedDesc where it wasn't
> > being checked before.  Is that some kind of live bug ?
> > pg_config() text_to_table()
> 
> Yes, expectedDesc was accessed in these locations without checking that
> it is not NULL. Should that be a separate patch?

I don't know.  The patch is already easy to review, since it's mostly limited
to removing code and fixing inconsistencies (NULL check) and possible
inefficiencies (randomAccess).

If the expectedDesc NULL check were an 0001 patch, then 0002 (the main patch)
would be even easier to review.  Only foreign.c is different.

> > You removed one call to tuplestore_donestoring(), but not the others.
> > I guess you could remove them all (optionally as a separate patch).
> 
> I removed it in that one location because I wanted to get rid of the
> local variable it used.

What local variable ?  I see now that logicalfuncs.c never had a local variable
called tupstore.  I'm sure it was intended since 2014 (b89e15105) to say
tupestore_donestoring(p->tupstore).  But donestoring(typo) causes no error,
since the define is a NOP.

src/include/utils/tuplestore.h:#define tuplestore_donestoring(state) ((void) 0)

> I am fine with removing the other occurrences,
> but I thought there might be some reason why instead of removing it,
> they made it into a no-op.

I assume Tom left it (actually, added it back in dd04e958c) to avoid breaking
extensions for no reason.  And maybe to preserve the possbility of at some
point in the future doing something again during the "done" step.

I'd leave it for input from a committer about those:

 - remove tuplestore_donestoring() calls ?
 - split expectedDesc NULL check to an 0001 patch ?
 - anything other opened questions ?

I'm marking this as RFC, with the caveat that the newline before
MakeFuncResultTuplestore went missing again :)

-- 
Justin