Re: make tuplestore helper function

On Tue, Feb 15, 2022 at 11:57:56PM -0600, Justin Pryzby wrote:
> On Tue, Jan 11, 2022 at 10:19:33AM -0500, Melanie Plageman wrote:
>> I'll wait to do that if preferred by committer.
>> Are you imagining that patch 0001 would only add the check for
>> expectedDesc that is missing from pg_config() and text_to_table()?

It took me some time to follow up on this point, but we clearly don't
expect a NULL expectedDesc in the context of these two code paths or
we finish with a NULL pointer dereference in CreateTupleDescCopy()
with a crash, so they had better be added. It also makes sense to me
to do that first and independently of the rest of the patch: it is a
separate issue.

> Yes, that's what I meant, but I haven't been able to determine if a NULL check
> should be added at all.

+ (errcode(ERRCODE_E_R_I_E_SRF_PROTOCOL_VIOLATED),
+ errmsg("expected tuple format not specified as required for "
+ "set-returning function.")))
errmsgs should be in a single line, to ease the grepping of similar
patterns.

There is something else that I think could be cleaned up, while we are
working on this area. Some of the callers of
MakeFuncResultTuplestore() (prepare.c, portalmem.c, one in genfile.c,
guc.c, misc.c) pass down a NULL tupdesc while building their TupleDesc
using CreateTemplateTupleDesc() and a set of TupleDescInitEntry() for
each attribute. This data exists already in pg_proc.dat, duplicating
the effort. Except if I am missing something, wouldn't it be better
to let get_call_result_type() do this work for us, passing a TupleDesc
pointer to the new MakeFuncResultTuplestore() rather than NULL?

Once you do that, there is something else that stands out: all the
callers of MakeFuncResultTuplestore() passing NULL for tupledesc *have
to* set expectedDesc, as far as I can see from the remaining callers.
This brings an interesting effect, couldn't we move the check on
ReturnSetInfo->expectedDesc being not NULL within
MakeFuncResultTuplestore(), checking after it only when tupdesc ==
NULL?

One would say that this changes the error ordering in code paths like
pg_config(), but, actually, in this case, we don't have any need for
the part "Check to make sure we have a reasonable tuple descriptor" as
of pg_proc.dat defining the tuple description this function uses, no?
And we could switch to a tuplestore in this case, as well, reducing by
one the numbers of callers with tupdesc=NULL for the new routine.
This could be a separate cleanup, separate from the rest, done first.

each_worker_jsonb() does not really need its check on expectedDesc
being NULL, does it? HEAD grabs the tuple descriptor with
get_call_result_type(), the patch uses MakeFuncResultTuplestore(), so
we make nowhere use of the expected TupleDesc saved by the SRF
executor.

Asserting that we are in the correct memory context in when calling
MakeFuncResultTuplestore() sounds rather sensible from here as per the
magics done in the various json functions. Still, it really feels
like we could do a more centralized consolidation of the whole.
--
Michael