On Fri, Aug 9, 2024 at 4:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> However, after testing it I got less excited, because it caused
> quite a lot of regression test changes (which I didn't bother to
> include in 0001). Many of them seem like significant readability
> lossage, for example
>
> GroupAggregate
> - Group Key: pagg_tab1.y
> + Group Key: (NULL::integer)
> -> Sort
> - Sort Key: pagg_tab1.y
> + Sort Key: (NULL::integer)
> -> Result
> One-Time Filter: false
Yeah, it seems that the original tlist of a dummy Result is needed to
deparse upper plan nodes. With 0001, expressions that should be
matched to lower tlist might end up with all NULLs, which seems not
great.
> So I'm coming around to doing something like the quick hack you
> proposed. I don't like it too much because it seems like it could
> make other bugs in this area much harder to notice, but I don't have a
> better idea. We do need some work on the outdated comments though.
> Also, I think we should use "f%d" not "col%d" by analogy to the
> default field names for RowExprs and anonymous record types.
Agreed. Do you think it would be helpful to add some assertions to
verify the plan type? For example, if dpns->inner_plan is NULL, the
plan should be a Result node; otherwise, it must be a SubqueryScan
node (or in the RTE_CTE case, it must be a CteScan/WorkTableScan
node).
Thanks
Richard