Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF

On Wed, Apr 10, 2024 at 3:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I think that's mostly a kluge.  It seems to me that the actual problem
> is that 2ed8f9a01 missed some places that need fixing.  The policy
> that that commit intended to institute is "if a RangeTblFunction has a
> coldeflist, then the function return type is certainly RECORD, and we
> don't need to try to dig the column specifications out of the function
> expression" (which dodges the problem that the function expression
> might not produce what we're expecting).  However, expandRTE didn't
> get that memo, and would produce the wrong tupdesc in an example such
> as this one.  IMO, it ought to produce the query-specified columns in
> all cases, even when the function expression doesn't match.


+1.  This seems a more principled fix.


> I dug around looking at other callers of get_expr_result_type and
> get_expr_result_tupdesc, and found a couple other places that aren't
> actively buggy but can be made faster and more robust by adding
> this same rule.


I wonder why the v2 patch does not apply this same rule in
process_function_rte_ref(), by checking if the 'rtfunc' has a coldeflist
before it calls get_expr_result_tupdesc.


> BTW, I wondered why the test cases we added for 2ed8f9a01 didn't
> catch this, because they sure look superficially similar.  The
> reason seems to be that we fail because this query produces a join
> plan in which we invoke build_physical_tlist for the FunctionScan,
> and that's what's calling expandRTE and hitting the assertion.
> The older test cases get simplified sufficiently that there's no
> join but just a FunctionScan, and that plan node will be required
> to produce the query-specified tlist not a physical tlist, so we
> accidentally avoid reaching the assertion.


Exactly.

Thanks
Richard