Date: Tue, 2 Nov 2021 15:17:05 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>,
	Andres Freund <andres@anarazel.de>
Subject: Re: make tuplestore helper function
Message-ID: <202111021817.ngdvsnnsmkq5@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAKRu_azyd1Z3W_r7Ou4sorTjRCs+PxeHw1CWJeXKofkE6TuZg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202111021817.ngdvsnnsmkq5%40alvherre.pgsql>
Precedence: bulk

On 2021-Nov-02, Melanie Plageman wrote:

> Attached is a patch to create a helper function which creates a
> tuplestore to be used by FUNCAPI-callable functions.

Looks good, and given the amount of code being removed, it seems a
no-brainer that some helper(s) is/are needed.

I think the name is a bit too generic.  How about
MakeFuncResultTuplestore()?

I think the function should not modify rsinfo -- having that as
(undocumented) side effect is weird.  I would suggest to just return the
tuplestore, and have the caller stash it in rsinfo->setResult and modify
the other struct members alongside that.  It's a couple more lines per
caller, but the code is clearer that way.

> There are a few places with very similar code to the new helper
> (MakeTuplestore()) but which, for example, don't expect the return type
> to be a row (pg_ls_dir(), pg_tablespace_databases(), text_to_table()). I
> wasn't sure if it was worth modifying it for their benefit.

Is this just because of the tupdesc?  If that's the case, then maybe the
tupdesc can be optionally passed in by caller, and when it's not, the
helper does get_call_result_type() and the tupdesc is used as output
argument.

> I wasn't sure if the elog() check for call result type should be changed
> to an ereport().

I don't see a reason for that: it's still an internal (not user-facing) error.

> All callers of MakeTuplestore() pass work_mem as the third parameter, so
> I could just omit it and hard-code it in, but I wasn't sure that felt
> right in a utility/helper function.

No opinion.  I would have used the global in the function instead of
passing it in.

> I inlined deflist_to_tuplestore() in foreign.c since it was billed as a
> helper function but wasn't used elsewhere and it made it harder to use
> MakeTuplestore() in this location.

This is a bit strange.  Does it work to pass rsinfo->expectedDesc?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."