Date: Fri, 17 Dec 2021 14:04:19 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: pgsql-hackers@postgresql.org, Andres Freund <andres@anarazel.de>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: make tuplestore helper function
Message-ID: <20211217200419.GQ17618@telsasoft.com>
References: 
 <CAAKRu_azyd1Z3W_r7Ou4sorTjRCs+PxeHw1CWJeXKofkE6TuZg@mail.gmail.com>
 <20211102202353.GG17618@telsasoft.com>
 <CAAKRu_a-ZZTez9yn4EMDAQQgt1UFpFcTVr=MDxf-b75tZ9q8UQ@mail.gmail.com>
 <20211108201338.GU17618@telsasoft.com>
 <CAAKRu_b2rySeOC=zZDd-++N6AXTuL-uABpPKQgO8f2NV4j5dPw@mail.gmail.com>
 <20211118182430.GO17618@telsasoft.com>
 <CAAKRu_aQDmqRmDLE4qPSL-b0cPQA-ELkfkSBWtLgV6Y1Zp6yBQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_aQDmqRmDLE4qPSL-b0cPQA-ELkfkSBWtLgV6Y1Zp6yBQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211217200419.GQ17618%40telsasoft.com>
Precedence: bulk

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