Re: Oversight in reparameterize_path_by_child leading to executor crash

On Wed, Jan 31, 2024 at 5:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Richard Guo <guofenglinux@gmail.com> writes:
> > On Wed, Jan 17, 2024 at 5:01 PM Richard Guo <guofenglinux@gmail.com>
> wrote:
> >> Sure, here it is:
> >> v10-0001-Avoid-reparameterizing-Paths-when-it-s-not-suitable.patch
>
> > I forgot to mention that this patch applies on v16 not master.
>
> I spent some time looking at this patch (which seems more urgent than
> the patch for master, given that we have back-branch releases coming
> up).


Thanks for looking at this patch!


> There are two things I'm not persuaded about:
>
> * 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.  So contain_references_to()
calls extract_actual_clauses() first before it calls pull_var_clause().


> * Is contain_references_to() doing the right thing by specifying
> PVC_RECURSE_PLACEHOLDERS?  That causes it to totally ignore a
> PlaceHolderVar's ph_eval_at, and I'm not convinced that's correct.


I was thinking that we should recurse into the PHV's contents to see if
there are any lateral references to the other join relation.  But now I
realize that checking phinfo->ph_lateral, as you suggested, might be a
better way to do that.

But I'm not sure about checking phinfo->ph_eval_at.  It seems to me that
the ph_eval_at could not overlap the other join relation; otherwise the
clause would not be a restriction clause but rather a join clause.


> Ideally it seems to me that we want to reject a PlaceHolderVar
> if either its ph_eval_at or ph_lateral overlap the other join
> relation; if it was coded that way then we'd not need to recurse
> into the PHV's contents.   pull_varnos isn't directly amenable
> to this, but I think we could use pull_var_clause with
> PVC_INCLUDE_PLACEHOLDERS and then iterate through the resulting
> list manually.  (If this patch were meant for HEAD, I'd think
> about extending the var.c code to support this usage more directly.
> But as things stand, this is a one-off so I think we should just do
> what we must in reparameterize_path_by_child.)


Thanks for the suggestion.  I've coded it this way in the v11 patch, and
left a XXX comment about checking phinfo->ph_eval_at.


> BTW, it shouldn't be necessary to write either PVC_RECURSE_AGGREGATES
> or PVC_RECURSE_WINDOWFUNCS, because neither kind of node should ever
> appear in a scan-level expression.  I'd leave those out so that we
> get an error if something unexpected happens.


Good point.

Thanks
Richard