On Wed, Jan 31, 2024 at 5:12 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Richard Guo <guofengli...@gmail.com> writes: > > On Wed, Jan 17, 2024 at 5:01 PM Richard Guo <guofengli...@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
v11-0001-Avoid-reparameterizing-Paths-when-it-s-not-suitable.patch
Description: Binary data
