Re: Oversight in reparameterize_path_by_child leading to executor crash

On Wed, Jan 31, 2024 at 11:21 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> > On Wed, Jan 31, 2024 at 5:12 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> * Why is it okay to just use pull_varnos on a tablesample expression,
> >> when contain_references_to() does something different?
>
> > Hmm, the main reason is that the expression_tree_walker() function does
> > not handle T_RestrictInfo nodes. So we cannot just use pull_varnos or
> > pull_var_clause on a restrictinfo list.
>
> Right, the extract_actual_clauses step is not wanted for the
> tablesample expression. But my point is that surely the handling of
> Vars and PlaceHolderVars should be the same, which it's not, and your
> v11 makes it even less so. How can it be OK to ignore Vars in the
> restrictinfo case?

Hmm, in this specific scenario it seems that it's not possible to have
Vars in the restrictinfo list that laterally reference the outer join
relation; otherwise the clause containing such Vars would not be a
restriction clause but rather a join clause. So in v11 I did not check
Vars in contain_references_to().

But I agree that we'd better to handle Vars and PlaceHolderVars in the
same way.

> I think the code structure we need to end up with is a routine that
> takes a RestrictInfo-free node tree (and is called directly in the
> tablesample case) with a wrapper routine that strips the RestrictInfos
> (for use on restriction lists).

How about the attached v12 patch? But I'm a little concerned about
omitting PVC_RECURSE_AGGREGATES and PVC_RECURSE_WINDOWFUNCS in this
case. Is it possible that aggregates or window functions appear in the
tablesample expression?

Thanks
Richard