From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: pgsql-hackers@postgresql.org, Andres Freund <andres@anarazel.de>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: Re: make tuplestore helper function
Date: Fri, 17 Dec 2021 14:04:19 -0600
Message-ID: <20211217200419.GQ17618@telsasoft.com> (raw)
In-Reply-To: <CAAKRu_aQDmqRmDLE4qPSL-b0cPQA-ELkfkSBWtLgV6Y1Zp6yBQ@mail.gmail.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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, melanieplageman@gmail.com, andres@anarazel.de, alvherre@alvh.no-ip.org
Subject: Re: make tuplestore helper function
In-Reply-To: <20211217200419.GQ17618@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox