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
signature.asc
Description: PGP signature