On Wed, Jan 5, 2022 at 7:57 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> 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:
> > > 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.
I'll wait to do that if preferred by committer.
Are you imagining that patch 0001 would only add the check for
expectedDesc that is missing from pg_config() and text_to_table()?
> > > 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)
Yes, I mean the local variable, tupstore.
> > 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 :)
oops. I've attached v6 with the newline.
- Melanie
