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). 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? * 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. 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.) 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. regards, tom lane