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(at)sss(dot)pgh(dot)pa(dot)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