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.