Re: make tuplestore helper function

Thanks for the detailed review!

On Fri, Dec 17, 2021 at 3:04 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> On Mon, Dec 13, 2021 at 05:27:52PM -0500, Melanie Plageman wrote:
> > Done in attached v4
>
> Thanks.
>
> I think these comments can be removed from the callers:
> /* it's a simple type, so don't use get_call_result_type */

Done in attached v5

> 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. 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.

> 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?

> It'd be nice if the "expected tuple format" error didn't need to be duplicated
> for each SRFs. I suppose the helper function could just take a boolean
> determining whether or not to throw an error (passing "expectedDesc==NULL").
> You'd have to call the helper before CreateTupleDescCopy() - which you're
> already doing in a couple places for similar reasons.

So, I don't think it makes sense to move that error message into
MakeFuncResultTuplestore() for the cases in which NULL is passed in for
the tuple descriptor. expectedDesc is not accessed at all in these cases
inside the function (since get_call_result_type()) is not called. For
the cases when a tuple descriptor is passed in, only two callers
actually check for expectedDesc -- each_worker_jsonb and each_worker.
Therefore, I don't think it makes sense to add another parameter just to
move that check inside for two callers.

> I noticed that tuplestore.h isn't included most places. I suppose it's
> included via execnodes.h. Your patch doesn't change that, arguably it
> should've always been included.

I've now included it in funcapi.c.

- Melanie