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 */
You removed one call to tuplestore_donestoring(), but not the others.
I guess you could remove them all (optionally as a separate patch).
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).
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()
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.
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.
--
Justin
